Re: [PATCH v4 01/10] Use IDR to maintain all the enabled sources' paths.

From: Jinlong Mao <quic_jinlmao@quicinc.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Mike Leach <mike.leach@linaro.org>, Leo Yan <leo.yan@linaro.org>,
	<coresight@lists.linaro.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>,
	Tingwei Zhang <quic_tingweiz@quicinc.com>,
	Yuanfang Zhang <quic_yuanfang@quicinc.com>,
	Tao Zhang <quic_taozha@quicinc.com>,
	Trilok Soni <quic_tsoni@quicinc.com>,
	Hao Zhang <quic_hazha@quicinc.com>,
	<linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH v4 01/10] Use IDR to maintain all the enabled sources' paths.
Date: Tue, 29 Mar 2022 21:47:18 +0800	[thread overview]
Message-ID: <7c0bed05-a071-82f3-3162-64b88bbe7dc2@quicinc.com> (raw)
In-Reply-To: <YjylKhBslN+5sKRx@kroah.com>

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