| Message ID | 20211209141543.21314-2-quic_jinlmao@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/9] Use IDR to maintain all the enabled sources' paths. | expand |
Hi Mao, On Thu, Dec 09, 2021 at 10:15:35PM +0800, Mao Jinlong wrote: > Use hash length of the source's device name to map to the pointer > of the enabled path. Using IDR will be more efficient than using > the list. And there could be other sources except STM and CPU etms > in the new HWs. It is better to maintain all the paths together. > > Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com> > --- > drivers/hwtracing/coresight/coresight-core.c | 76 +++++++------------- > 1 file changed, 26 insertions(+), 50 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 8a18c71df37a..cc6b6cabf85f 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -7,6 +7,7 @@ > #include <linux/init.h> > #include <linux/types.h> > #include <linux/device.h> > +#include <linux/idr.h> > #include <linux/io.h> > #include <linux/err.h> > #include <linux/export.h> > @@ -26,6 +27,12 @@ > static DEFINE_MUTEX(coresight_mutex); > static DEFINE_PER_CPU(struct coresight_device *, csdev_sink); > > +/* > + * Use IDR to map the hash length of the source's device name > + * to the pointer of path for the source > + */ > +static DEFINE_IDR(path_idr); > + > /** > * struct coresight_node - elements of a path, from source to sink > * @csdev: Address of an element. > @@ -36,20 +43,6 @@ struct coresight_node { > struct list_head link; > }; > > -/* > - * When operating Coresight drivers from the sysFS interface, only a single > - * path can exist from a tracer (associated to a CPU) to a sink. > - */ > -static DEFINE_PER_CPU(struct list_head *, tracer_path); > - > -/* > - * As of this writing only a single STM can be found in CS topologies. Since > - * there is no way to know if we'll ever see more and what kind of > - * configuration they will enact, for the time being only define a single path > - * for STM. > - */ > -static struct list_head *stm_path; > - > /* > * When losing synchronisation a new barrier packet needs to be inserted at the > * beginning of the data collected in a buffer. That way the decoder knows that > @@ -1088,10 +1081,11 @@ static int coresight_validate_source(struct coresight_device *csdev, > > int coresight_enable(struct coresight_device *csdev) > { > - int cpu, ret = 0; > + int ret = 0; > struct coresight_device *sink; > struct list_head *path; > enum coresight_dev_subtype_source subtype; > + u32 hash; > > subtype = csdev->subtype.source_subtype; > > @@ -1133,26 +1127,14 @@ int coresight_enable(struct coresight_device *csdev) > if (ret) > goto err_source; > > - switch (subtype) { > - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: > - /* > - * When working from sysFS it is important to keep track > - * of the paths that were created so that they can be > - * undone in 'coresight_disable()'. Since there can only > - * be a single session per tracer (when working from sysFS) > - * a per-cpu variable will do just fine. > - */ > - cpu = source_ops(csdev)->cpu_id(csdev); > - per_cpu(tracer_path, cpu) = path; > - break; > - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE: > - stm_path = path; > - break; > - default: > - /* We can't be here */ > - break; > - } > - > + /* > + * Use the hash length of source's device name as ID > + * and map the ID to the pointer of the path. > + */ > + hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev))); > + ret = idr_alloc_u32(&path_idr, path, &hash, hash, GFP_KERNEL); > + if (ret) > + goto err_source; > out: > mutex_unlock(&coresight_mutex); > return ret; > @@ -1168,8 +1150,9 @@ EXPORT_SYMBOL_GPL(coresight_enable); > > void coresight_disable(struct coresight_device *csdev) > { > - int cpu, ret; > + int ret; > struct list_head *path = NULL; > + u32 hash; > > mutex_lock(&coresight_mutex); > > @@ -1180,21 +1163,13 @@ void coresight_disable(struct coresight_device *csdev) > if (!csdev->enable || !coresight_disable_source(csdev)) > goto out; > > - switch (csdev->subtype.source_subtype) { > - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: > - cpu = source_ops(csdev)->cpu_id(csdev); > - path = per_cpu(tracer_path, cpu); > - per_cpu(tracer_path, cpu) = NULL; > - break; > - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE: > - path = stm_path; > - stm_path = NULL; > - break; > - default: > - /* We can't be here */ > - break; > - } > + hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev))); > + /* Find the path by the hash length. */ > + path = idr_find(&path_idr, hash); > + if (path == NULL) > + return; Please add a dev_err() here as this should really not be happening. > > + idr_remove(&path_idr, hash); > coresight_disable_path(path); > coresight_release_path(path); > > @@ -1779,6 +1754,7 @@ static int __init coresight_init(void) > > static void __exit coresight_exit(void) > { > + idr_destroy(&path_idr); As far as I can tell this isn't needed. > cscfg_exit(); > etm_perf_exit(); > bus_unregister(&coresight_bustype); > -- > 2.17.1 >
Hi Mathieu, Thanks for the review. On 12/15/2021 2:30 AM, Mathieu Poirier wrote: > Hi Mao, > > On Thu, Dec 09, 2021 at 10:15:35PM +0800, Mao Jinlong wrote: >> Use hash length of the source's device name to map to the pointer >> of the enabled path. Using IDR will be more efficient than using >> the list. And there could be other sources except STM and CPU etms >> in the new HWs. It is better to maintain all the paths together. >> >> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com> >> --- >> drivers/hwtracing/coresight/coresight-core.c | 76 +++++++------------- >> 1 file changed, 26 insertions(+), 50 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c >> index 8a18c71df37a..cc6b6cabf85f 100644 >> --- a/drivers/hwtracing/coresight/coresight-core.c >> +++ b/drivers/hwtracing/coresight/coresight-core.c >> @@ -7,6 +7,7 @@ >> #include <linux/init.h> >> #include <linux/types.h> >> #include <linux/device.h> >> +#include <linux/idr.h> >> #include <linux/io.h> >> #include <linux/err.h> >> #include <linux/export.h> >> @@ -26,6 +27,12 @@ >> static DEFINE_MUTEX(coresight_mutex); >> static DEFINE_PER_CPU(struct coresight_device *, csdev_sink); >> >> +/* >> + * Use IDR to map the hash length of the source's device name >> + * to the pointer of path for the source >> + */ >> +static DEFINE_IDR(path_idr); >> + >> /** >> * struct coresight_node - elements of a path, from source to sink >> * @csdev: Address of an element. >> @@ -36,20 +43,6 @@ struct coresight_node { >> struct list_head link; >> }; >> >> -/* >> - * When operating Coresight drivers from the sysFS interface, only a single >> - * path can exist from a tracer (associated to a CPU) to a sink. >> - */ >> -static DEFINE_PER_CPU(struct list_head *, tracer_path); >> - >> -/* >> - * As of this writing only a single STM can be found in CS topologies. Since >> - * there is no way to know if we'll ever see more and what kind of >> - * configuration they will enact, for the time being only define a single path >> - * for STM. >> - */ >> -static struct list_head *stm_path; >> - >> /* >> * When losing synchronisation a new barrier packet needs to be inserted at the >> * beginning of the data collected in a buffer. That way the decoder knows that >> @@ -1088,10 +1081,11 @@ static int coresight_validate_source(struct coresight_device *csdev, >> >> int coresight_enable(struct coresight_device *csdev) >> { >> - int cpu, ret = 0; >> + int ret = 0; >> struct coresight_device *sink; >> struct list_head *path; >> enum coresight_dev_subtype_source subtype; >> + u32 hash; >> >> subtype = csdev->subtype.source_subtype; >> >> @@ -1133,26 +1127,14 @@ int coresight_enable(struct coresight_device *csdev) >> if (ret) >> goto err_source; >> >> - switch (subtype) { >> - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: >> - /* >> - * When working from sysFS it is important to keep track >> - * of the paths that were created so that they can be >> - * undone in 'coresight_disable()'. Since there can only >> - * be a single session per tracer (when working from sysFS) >> - * a per-cpu variable will do just fine. >> - */ >> - cpu = source_ops(csdev)->cpu_id(csdev); >> - per_cpu(tracer_path, cpu) = path; >> - break; >> - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE: >> - stm_path = path; >> - break; >> - default: >> - /* We can't be here */ >> - break; >> - } >> - >> + /* >> + * Use the hash length of source's device name as ID >> + * and map the ID to the pointer of the path. >> + */ >> + hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev))); >> + ret = idr_alloc_u32(&path_idr, path, &hash, hash, GFP_KERNEL); >> + if (ret) >> + goto err_source; >> out: >> mutex_unlock(&coresight_mutex); >> return ret; >> @@ -1168,8 +1150,9 @@ EXPORT_SYMBOL_GPL(coresight_enable); >> >> void coresight_disable(struct coresight_device *csdev) >> { >> - int cpu, ret; >> + int ret; >> struct list_head *path = NULL; >> + u32 hash; >> >> mutex_lock(&coresight_mutex); >> >> @@ -1180,21 +1163,13 @@ void coresight_disable(struct coresight_device *csdev) >> if (!csdev->enable || !coresight_disable_source(csdev)) >> goto out; >> >> - switch (csdev->subtype.source_subtype) { >> - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: >> - cpu = source_ops(csdev)->cpu_id(csdev); >> - path = per_cpu(tracer_path, cpu); >> - per_cpu(tracer_path, cpu) = NULL; >> - break; >> - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE: >> - path = stm_path; >> - stm_path = NULL; >> - break; >> - default: >> - /* We can't be here */ >> - break; >> - } >> + hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev))); >> + /* Find the path by the hash length. */ >> + path = idr_find(&path_idr, hash); >> + if (path == NULL) >> + return; > Please add a dev_err() here as this should really not be happening. I will add the dev_err(). > >> >> + idr_remove(&path_idr, hash); >> coresight_disable_path(path); >> coresight_release_path(path); >> >> @@ -1779,6 +1754,7 @@ static int __init coresight_init(void) >> >> static void __exit coresight_exit(void) >> { >> + idr_destroy(&path_idr); > As far as I can tell this isn't needed. I will remove this. > >> cscfg_exit(); >> etm_perf_exit(); >> bus_unregister(&coresight_bustype); >> -- >> 2.17.1 >>
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 8a18c71df37a..cc6b6cabf85f 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -7,6 +7,7 @@ #include <linux/init.h> #include <linux/types.h> #include <linux/device.h> +#include <linux/idr.h> #include <linux/io.h> #include <linux/err.h> #include <linux/export.h> @@ -26,6 +27,12 @@ static DEFINE_MUTEX(coresight_mutex); static DEFINE_PER_CPU(struct coresight_device *, csdev_sink); +/* + * Use IDR to map the hash length of the source's device name + * to the pointer of path for the source + */ +static DEFINE_IDR(path_idr); + /** * struct coresight_node - elements of a path, from source to sink * @csdev: Address of an element. @@ -36,20 +43,6 @@ struct coresight_node { struct list_head link; }; -/* - * When operating Coresight drivers from the sysFS interface, only a single - * path can exist from a tracer (associated to a CPU) to a sink. - */ -static DEFINE_PER_CPU(struct list_head *, tracer_path); - -/* - * As of this writing only a single STM can be found in CS topologies. Since - * there is no way to know if we'll ever see more and what kind of - * configuration they will enact, for the time being only define a single path - * for STM. - */ -static struct list_head *stm_path; - /* * When losing synchronisation a new barrier packet needs to be inserted at the * beginning of the data collected in a buffer. That way the decoder knows that @@ -1088,10 +1081,11 @@ static int coresight_validate_source(struct coresight_device *csdev, int coresight_enable(struct coresight_device *csdev) { - int cpu, ret = 0; + int ret = 0; struct coresight_device *sink; struct list_head *path; enum coresight_dev_subtype_source subtype; + u32 hash; subtype = csdev->subtype.source_subtype; @@ -1133,26 +1127,14 @@ int coresight_enable(struct coresight_device *csdev) if (ret) goto err_source; - switch (subtype) { - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: - /* - * When working from sysFS it is important to keep track - * of the paths that were created so that they can be - * undone in 'coresight_disable()'. Since there can only - * be a single session per tracer (when working from sysFS) - * a per-cpu variable will do just fine. - */ - cpu = source_ops(csdev)->cpu_id(csdev); - per_cpu(tracer_path, cpu) = path; - break; - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE: - stm_path = path; - break; - default: - /* We can't be here */ - break; - } - + /* + * Use the hash length of source's device name as ID + * and map the ID to the pointer of the path. + */ + hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev))); + ret = idr_alloc_u32(&path_idr, path, &hash, hash, GFP_KERNEL); + if (ret) + goto err_source; out: mutex_unlock(&coresight_mutex); return ret; @@ -1168,8 +1150,9 @@ EXPORT_SYMBOL_GPL(coresight_enable); void coresight_disable(struct coresight_device *csdev) { - int cpu, ret; + int ret; struct list_head *path = NULL; + u32 hash; mutex_lock(&coresight_mutex); @@ -1180,21 +1163,13 @@ void coresight_disable(struct coresight_device *csdev) if (!csdev->enable || !coresight_disable_source(csdev)) goto out; - switch (csdev->subtype.source_subtype) { - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC: - cpu = source_ops(csdev)->cpu_id(csdev); - path = per_cpu(tracer_path, cpu); - per_cpu(tracer_path, cpu) = NULL; - break; - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE: - path = stm_path; - stm_path = NULL; - break; - default: - /* We can't be here */ - break; - } + hash = hashlen_hash(hashlen_string(NULL, dev_name(&csdev->dev))); + /* Find the path by the hash length. */ + path = idr_find(&path_idr, hash); + if (path == NULL) + return; + idr_remove(&path_idr, hash); coresight_disable_path(path); coresight_release_path(path); @@ -1779,6 +1754,7 @@ static int __init coresight_init(void) static void __exit coresight_exit(void) { + idr_destroy(&path_idr); cscfg_exit(); etm_perf_exit(); bus_unregister(&coresight_bustype);
Use hash length of the source's device name to map to the pointer of the enabled path. Using IDR will be more efficient than using the list. And there could be other sources except STM and CPU etms in the new HWs. It is better to maintain all the paths together. Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com> --- drivers/hwtracing/coresight/coresight-core.c | 76 +++++++------------- 1 file changed, 26 insertions(+), 50 deletions(-)