| Message ID | 20220324121734.21531-2-quic_jinlmao@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Coresight: Add support for TPDM and TPDA | expand |
On Thu, Mar 24, 2022 at 08:17:25PM +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 | 75 +++++++------------- > 1 file changed, 26 insertions(+), 49 deletions(-) Your subject line is odd. Please put back the driver subsystem in the subject line so that it makes more sense. And how have you measured "more efficient"? thanks, greg k-h
Hi Greg, Thanks for your review. On 3/24/2022 8:26 PM, Greg Kroah-Hartman wrote: > On Thu, Mar 24, 2022 at 08:17:25PM +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 | 75 +++++++------------- >> 1 file changed, 26 insertions(+), 49 deletions(-) > Your subject line is odd. Please put back the driver subsystem in the > subject line so that it makes more sense. I will update the subject in next version. > > And how have you measured "more efficient"? Using IDR would be better than doing a sequential search as there will be much more device in future. > > thanks, > > greg k-h Thanks Jinlong Mao
On Thu, Mar 24, 2022 at 10:23:19PM +0800, Jinlong Mao wrote: > Hi Greg, > > Thanks for your review. > > On 3/24/2022 8:26 PM, Greg Kroah-Hartman wrote: > > On Thu, Mar 24, 2022 at 08:17:25PM +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 | 75 +++++++------------- > > > 1 file changed, 26 insertions(+), 49 deletions(-) > > Your subject line is odd. Please put back the driver subsystem in the > > subject line so that it makes more sense. > I will update the subject in next version. > > > > And how have you measured "more efficient"? > > Using IDR would be better than doing a sequential search as there will be > much more device in future. How many "more"? Where does the trade off of speed for complexity help? How much faster is this really? You can't claim performance improvements without any proof :) thanks, greg k-h
On 24/03/2022 14:23, Jinlong Mao wrote: > Hi Greg, > > Thanks for your review. > > On 3/24/2022 8:26 PM, Greg Kroah-Hartman wrote: >> On Thu, Mar 24, 2022 at 08:17:25PM +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 | 75 +++++++------------- >>> 1 file changed, 26 insertions(+), 49 deletions(-) >> Your subject line is odd. Please put back the driver subsystem in the >> subject line so that it makes more sense. > I will update the subject in next version. >> >> And how have you measured "more efficient"? > > Using IDR would be better than doing a sequential search as there will > be much more device in future. Where do we use sequential search now ? For non-CPU bound sources, yes we may need something. But CPU case is straight forward, and could be retained as it is. i.e., per-cpu list of paths. Cheers Suzuki > >> >> thanks, >> >> greg k-h > > Thanks > > Jinlong Mao >
Hi Greg, On 3/25/2022 1:06 AM, Greg Kroah-Hartman wrote: > On Thu, Mar 24, 2022 at 10:23:19PM +0800, Jinlong Mao wrote: >> Hi Greg, >> >> Thanks for your review. >> >> On 3/24/2022 8:26 PM, Greg Kroah-Hartman wrote: >>> On Thu, Mar 24, 2022 at 08:17:25PM +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 | 75 +++++++------------- >>>> 1 file changed, 26 insertions(+), 49 deletions(-) >>> Your subject line is odd. Please put back the driver subsystem in the >>> subject line so that it makes more sense. >> I will update the subject in next version. >>> And how have you measured "more efficient"? >> Using IDR would be better than doing a sequential search as there will be >> much more device in future. > How many "more"? Where does the trade off of speed for complexity help? > How much faster is this really? You can't claim performance > improvements without any proof :) There is about 40 trace sources in our internal device. I believe there will be more cpu cores, then there will be more etm sources. IDR here is used to store the path of both etm sources and other sources which aren't associated with CPU. Use IDR is not more complicated than using list. It will also save the time of searching the path when coresight_disable. I tested in internal device. The test case is that enable all the sources, disable the source one by one to check the search time. Use list to store paths: sh-7687 [005] .... 342.113099: __coresight_disable: ====search path start==== source_0 sh-7687 [005] .... 342.113127: __coresight_disable: ====search path end==== source_0 sh-7693 [005] .... 342.542216: __coresight_disable: ====search path start==== source_1 sh-7693 [005] .... 342.542244: __coresight_disable: ====search path end==== source_1 sh-7699 [005] .... 342.929083: __coresight_disable: ====search path start==== source_2 sh-7699 [005] .... 342.929106: __coresight_disable: ====search path end==== source_2 sh-7711 [005] .... 343.760688: __coresight_disable: ====search path start==== source_3 sh-7711 [005] .... 343.760713: __coresight_disable: ====search path end==== source_3 sh-7717 [005] .... 344.172353: __coresight_disable: ====search path start==== source_4 sh-7717 [005] .... 344.172381: __coresight_disable: ====search path end==== source_4 Use IDR to store paths: sh-7156 [006] .... 223.294228: __coresight_disable: ====search path start==== source_0 sh-7156 [006] .... 223.294237: __coresight_disable: ====search path end==== source_0 sh-7162 [006] .... 223.690153: __coresight_disable: ====search path start==== source_1 sh-7162 [006] .... 223.690163: __coresight_disable: ====search path end==== source_1 sh-7168 [006] .... 224.110670: __coresight_disable: ====search path start==== source_2 sh-7168 [006] .... 224.110679: __coresight_disable: ====search path end==== source_2 <...>-7180 [006] .... 224.929315: __coresight_disable: ====search path start==== source_3 <...>-7180 [006] .... 224.929324: __coresight_disable: ====search path end==== source_3 <...>-7186 [006] .... 225.343617: __coresight_disable: ====search path start==== source_4 <...>-7186 [006] .... 225.343626: __coresight_disable: ====search path end==== source_4 From the log, Searching the path from the IDR takes about 9us for each source. Searching the path from the list takes about 23 ~ 28us for the source. Use IDR saves much time. Thanks Jinlong Mao > > thanks, > > greg k-h
Hi Suzuki, On 3/28/2022 4:33 PM, Suzuki K Poulose wrote: > On 24/03/2022 14:23, Jinlong Mao wrote: >> Hi Greg, >> >> Thanks for your review. >> >> On 3/24/2022 8:26 PM, Greg Kroah-Hartman wrote: >>> On Thu, Mar 24, 2022 at 08:17:25PM +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 | 75 >>>> +++++++------------- >>>> 1 file changed, 26 insertions(+), 49 deletions(-) >>> Your subject line is odd. Please put back the driver subsystem in the >>> subject line so that it makes more sense. >> I will update the subject in next version. >>> >>> And how have you measured "more efficient"? >> >> Using IDR would be better than doing a sequential search as there >> will be much more device in future. > > Where do we use sequential search now ? For non-CPU bound sources, yes > we may need something. But CPU case is straight forward, and could be > retained as it is. i.e., per-cpu list of paths. > We use list to store the paths for both ETM and non-CPU bound sources in patch below. “[PATCH 01/10] coresight: add support to enable more coresight paths” According to Mathieu's comments, IDR is used now. So i added "Using IDR will be more efficient than using the list" this message in my commit message. I think we need to use one mechanism to store ETM and non-CPU bound sources. Mathieu's comments: So many TPDM and many ETMs... That is definitely a reason to do better than a sequential search. If an IDR (or some other kind of mechanism) is used then we can use that to store paths associated with ETMs as well. That way everything works the same way and access time is constant for any kind of source. Thanks Jinlong Mao > Cheers > Suzuki > > >> >>> >>> thanks, >>> >>> greg k-h >> >> Thanks >> >> Jinlong Mao >> >
On Tue, 29 Mar 2022 at 07:56, Jinlong Mao <quic_jinlmao@quicinc.com> wrote: > > Hi Suzuki, > > On 3/28/2022 4:33 PM, Suzuki K Poulose wrote: > > On 24/03/2022 14:23, Jinlong Mao wrote: > >> Hi Greg, > >> > >> Thanks for your review. > >> > >> On 3/24/2022 8:26 PM, Greg Kroah-Hartman wrote: > >>> On Thu, Mar 24, 2022 at 08:17:25PM +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 | 75 > >>>> +++++++------------- > >>>> 1 file changed, 26 insertions(+), 49 deletions(-) > >>> Your subject line is odd. Please put back the driver subsystem in the > >>> subject line so that it makes more sense. > >> I will update the subject in next version. > >>> > >>> And how have you measured "more efficient"? > >> > >> Using IDR would be better than doing a sequential search as there > >> will be much more device in future. > > > > Where do we use sequential search now ? For non-CPU bound sources, yes > > we may need something. But CPU case is straight forward, and could be > > retained as it is. i.e., per-cpu list of paths. > > > We use list to store the paths for both ETM and non-CPU bound sources in > patch below. > > “[PATCH 01/10] coresight: add support to enable more coresight paths” > > According to Mathieu's comments, IDR is used now. So i added "Using IDR > will be more efficient than using > the list" this message in my commit message. I think we need to use one > mechanism to store ETM and > non-CPU bound sources. > > > Mathieu's comments: > > So many TPDM and many ETMs... That is definitely a reason to do better than a > sequential search. > > If an IDR (or some other kind of mechanism) is used then we can use that to > store paths associated with ETMs as well. That way everything works the same > way and access time is constant for any kind of source. As per my last sentence above, the goal of my comment was to simplify things so that we don't have two different ways of managing sources. But if that ends up causing more trouble than benefit then it should be avoided. > > Thanks > > Jinlong Mao > > > Cheers > > Suzuki > > > > > >> > >>> > >>> thanks, > >>> > >>> greg k-h > >> > >> Thanks > >> > >> Jinlong Mao > >> > >
On 30/03/2022 03:10, Jinlong Mao wrote: > > On 3/29/2022 10:36 PM, Mathieu Poirier wrote: >> On Tue, 29 Mar 2022 at 07:56, Jinlong Mao<quic_jinlmao@quicinc.com> wrote: >>> Hi Suzuki, >>> >>> On 3/28/2022 4:33 PM, Suzuki K Poulose wrote: >>>> On 24/03/2022 14:23, Jinlong Mao wrote: >>>>> Hi Greg, >>>>> >>>>> Thanks for your review. >>>>> >>>>> On 3/24/2022 8:26 PM, Greg Kroah-Hartman wrote: >>>>>> On Thu, Mar 24, 2022 at 08:17:25PM +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 | 75 >>>>>>> +++++++------------- >>>>>>> 1 file changed, 26 insertions(+), 49 deletions(-) >>>>>> Your subject line is odd. Please put back the driver subsystem in the >>>>>> subject line so that it makes more sense. >>>>> I will update the subject in next version. >>>>>> And how have you measured "more efficient"? >>>>> Using IDR would be better than doing a sequential search as there >>>>> will be much more device in future. >>>> Where do we use sequential search now ? For non-CPU bound sources, yes >>>> we may need something. But CPU case is straight forward, and could be >>>> retained as it is. i.e., per-cpu list of paths. >>>> >>> We use list to store the paths for both ETM and non-CPU bound sources in >>> patch below. >>> >>> “[PATCH 01/10] coresight: add support to enable more coresight paths” >>> >>> According to Mathieu's comments, IDR is used now. So i added "Using IDR >>> will be more efficient than using >>> the list" this message in my commit message. I think we need to use one >>> mechanism to store ETM and >>> non-CPU bound sources. >>> >>> >>> Mathieu's comments: >>> >>> So many TPDM and many ETMs... That is definitely a reason to do better than a >>> sequential search. >>> >>> If an IDR (or some other kind of mechanism) is used then we can use that to >>> store paths associated with ETMs as well. That way everything works the same >>> way and access time is constant for any kind of source. >> As per my last sentence above, the goal of my comment was to simplify >> things so that we don't have two different ways of managing sources. >> But if that ends up causing more trouble than benefit then it should >> be avoided. > > Hi Mathieu, > > I didn't see any disadvantage to use IDR to store both ETM source and > non-CPU bound sources. > > Benefits: > > * Only need to maintain one way of managing sources. > * Less time to search the path My preference is to keep the ETM source paths per-CPU. For the reasons below : - It is straight forward for an ETM. per_cpu(paths, cpu) - It is faster than the IDR. - Makes the debugging easier. Simply lookup the per_cpu variable. I agree that the IDR is required for the non ETM sources. And I am fine with that. Suzuki > > Thanks > Jinlong Mao >>> Thanks >>> >>> Jinlong Mao >>> >>>> Cheers >>>> Suzuki >>>> >>>> >>>>>> thanks, >>>>>> >>>>>> greg k-h >>>>> Thanks >>>>> >>>>> Jinlong Mao >>>>> >> _______________________________________________ >> CoreSight mailing list --coresight@lists.linaro.org >> To unsubscribe send an email tocoresight-leave@lists.linaro.org
On 3/30/2022 5:05 PM, Suzuki K Poulose wrote: > On 30/03/2022 03:10, Jinlong Mao wrote: >> >> On 3/29/2022 10:36 PM, Mathieu Poirier wrote: >>> On Tue, 29 Mar 2022 at 07:56, Jinlong Mao<quic_jinlmao@quicinc.com> >>> wrote: >>>> Hi Suzuki, >>>> >>>> On 3/28/2022 4:33 PM, Suzuki K Poulose wrote: >>>>> On 24/03/2022 14:23, Jinlong Mao wrote: >>>>>> Hi Greg, >>>>>> >>>>>> Thanks for your review. >>>>>> >>>>>> On 3/24/2022 8:26 PM, Greg Kroah-Hartman wrote: >>>>>>> On Thu, Mar 24, 2022 at 08:17:25PM +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 | 75 >>>>>>>> +++++++------------- >>>>>>>> 1 file changed, 26 insertions(+), 49 deletions(-) >>>>>>> Your subject line is odd. Please put back the driver subsystem >>>>>>> in the >>>>>>> subject line so that it makes more sense. >>>>>> I will update the subject in next version. >>>>>>> And how have you measured "more efficient"? >>>>>> Using IDR would be better than doing a sequential search as there >>>>>> will be much more device in future. >>>>> Where do we use sequential search now ? For non-CPU bound sources, >>>>> yes >>>>> we may need something. But CPU case is straight forward, and could be >>>>> retained as it is. i.e., per-cpu list of paths. >>>>> >>>> We use list to store the paths for both ETM and non-CPU bound >>>> sources in >>>> patch below. >>>> >>>> “[PATCH 01/10] coresight: add support to enable more coresight paths” >>>> >>>> According to Mathieu's comments, IDR is used now. So i added >>>> "Using IDR >>>> will be more efficient than using >>>> the list" this message in my commit message. I think we need to use >>>> one >>>> mechanism to store ETM and >>>> non-CPU bound sources. >>>> >>>> >>>> Mathieu's comments: >>>> >>>> So many TPDM and many ETMs... That is definitely a reason to do >>>> better than a >>>> sequential search. >>>> >>>> If an IDR (or some other kind of mechanism) is used then we can use >>>> that to >>>> store paths associated with ETMs as well. That way everything >>>> works the same >>>> way and access time is constant for any kind of source. >>> As per my last sentence above, the goal of my comment was to simplify >>> things so that we don't have two different ways of managing sources. >>> But if that ends up causing more trouble than benefit then it should >>> be avoided. >> >> Hi Mathieu, >> >> I didn't see any disadvantage to use IDR to store both ETM source and >> non-CPU bound sources. >> >> Benefits: >> >> * Only need to maintain one way of managing sources. >> * Less time to search the path > > My preference is to keep the ETM source paths per-CPU. For the reasons > below : > - It is straight forward for an ETM. per_cpu(paths, cpu) > - It is faster than the IDR. > - Makes the debugging easier. Simply lookup the per_cpu variable. > > I agree that the IDR is required for the non ETM sources. And I am fine > with that. > > Suzuki Hi Suzuki, I will address your comments in next version. Could you please help to review other patches ? Thanks Jinlong Mao > >> >> Thanks >> Jinlong Mao >>>> Thanks >>>> >>>> Jinlong Mao >>>> >>>>> Cheers >>>>> Suzuki >>>>> >>>>> >>>>>>> thanks, >>>>>>> >>>>>>> greg k-h >>>>>> Thanks >>>>>> >>>>>> Jinlong Mao >>>>>> >>> _______________________________________________ >>> CoreSight mailing list --coresight@lists.linaro.org >>> To unsubscribe send an email tocoresight-leave@lists.linaro.org >
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index cd426569185e..8ff94d020893 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> @@ -27,6 +28,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. @@ -37,20 +44,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; - /* * Set up a global trace ID map. * We may need a per sink ID map in future for larger / multi sink systems. @@ -1057,10 +1050,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; @@ -1102,26 +1096,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 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; @@ -1137,8 +1119,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); @@ -1149,21 +1132,15 @@ 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. */ + path = idr_find(&path_idr, hash); + if (path == NULL) { + pr_err("Path is not found for %s\n", dev_name(&csdev->dev)); + goto out; } + idr_remove(&path_idr, hash); coresight_disable_path(path); coresight_release_path(path);
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 | 75 +++++++------------- 1 file changed, 26 insertions(+), 49 deletions(-)