[PATCH] perf parse-events: Use strcmp to compare the PMU name

[PATCH] perf parse-events: Use strcmp to compare the PMU name

[Jin Yao]

A big uncore event group is split into multiple small groups which
only include the uncore events from the same PMU. This has been
supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
uncore event aliases in small groups properly").

If the event's PMU name starts to repeat, it must be a new event.
That can be used to distinguish the leader from other members.
But now it only compares the pointer of pmu_name
(leader->pmu_name == evsel->pmu_name).

If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
the event list is:

evsel->name					evsel->pmu_name
---------------------------------------------------------------
unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_4 (as leader)
unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_2
unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_0
unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_5
unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_3
unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_1
unc_iio_data_req_of_cpu.mem_write.part1		uncore_iio_4
......

For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
"uncore_iio_4", it should be the event from PMU "uncore_iio_4".
It's not a new leader for this PMU.

But if we use "(leader->pmu_name == evsel->pmu_name)", the check
would be failed and the event is stored to leaders[] as a new
PMU leader.

So this patch uses strcmp to compare the PMU name between events.

Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in small groups properly")
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/parse-events.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 10107747b361..786eddb6a097 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1629,12 +1629,11 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
 		 * event. That can be used to distinguish the leader from
 		 * other members, even they have the same event name.
 		 */
-		if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) {
+		if ((leader != evsel) &&
+		    !strcmp(leader->pmu_name, evsel->pmu_name)) {
 			is_leader = false;
 			continue;
 		}
-		/* The name is always alias name */
-		WARN_ON(strcmp(leader->name, evsel->name));
 
 		/* Store the leader event for each PMU */
 		leaders[nr_pmu++] = (uintptr_t) evsel;
-- 
2.17.1

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[Jiri Olsa]

On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
> A big uncore event group is split into multiple small groups which
> only include the uncore events from the same PMU. This has been
> supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
> uncore event aliases in small groups properly").
> 
> If the event's PMU name starts to repeat, it must be a new event.
> That can be used to distinguish the leader from other members.
> But now it only compares the pointer of pmu_name
> (leader->pmu_name == evsel->pmu_name).
> 
> If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
> the event list is:
> 
> evsel->name					evsel->pmu_name
> ---------------------------------------------------------------
> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_4 (as leader)
> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_2
> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_0
> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_5
> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_3
> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_1
> unc_iio_data_req_of_cpu.mem_write.part1		uncore_iio_4
> ......
> 
> For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
> "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
> It's not a new leader for this PMU.
> 
> But if we use "(leader->pmu_name == evsel->pmu_name)", the check
> would be failed and the event is stored to leaders[] as a new
> PMU leader.
> 
> So this patch uses strcmp to compare the PMU name between events.
> 
> Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in small groups properly")
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>

looks good, any chance we could have automated test
for this uncore leader setup logic? like maybe the way
John did the pmu-events tests? like test will trigger
only when there's the pmu/events in the system

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka


> ---
>  tools/perf/util/parse-events.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 10107747b361..786eddb6a097 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1629,12 +1629,11 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
>  		 * event. That can be used to distinguish the leader from
>  		 * other members, even they have the same event name.
>  		 */
> -		if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) {
> +		if ((leader != evsel) &&
> +		    !strcmp(leader->pmu_name, evsel->pmu_name)) {
>  			is_leader = false;
>  			continue;
>  		}
> -		/* The name is always alias name */
> -		WARN_ON(strcmp(leader->name, evsel->name));
>  
>  		/* Store the leader event for each PMU */
>  		leaders[nr_pmu++] = (uintptr_t) evsel;
> -- 
> 2.17.1
> 

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[John Garry]

On 30/04/2020 09:45, Jiri Olsa wrote:
> On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
>> A big uncore event group is split into multiple small groups which
>> only include the uncore events from the same PMU. This has been
>> supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
>> uncore event aliases in small groups properly").
>>
>> If the event's PMU name starts to repeat, it must be a new event.
>> That can be used to distinguish the leader from other members.
>> But now it only compares the pointer of pmu_name
>> (leader->pmu_name == evsel->pmu_name).
>>
>> If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
>> the event list is:
>>
>> evsel->name					evsel->pmu_name
>> ---------------------------------------------------------------
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_4 (as leader)
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_2
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_0
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_5
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_3
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_1
>> unc_iio_data_req_of_cpu.mem_write.part1		uncore_iio_4
>> ......
>>
>> For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
>> "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
>> It's not a new leader for this PMU.
>>
>> But if we use "(leader->pmu_name == evsel->pmu_name)", the check
>> would be failed and the event is stored to leaders[] as a new
>> PMU leader.
>>
>> So this patch uses strcmp to compare the PMU name between events.
>>
>> Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in small groups properly")
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> 
> looks good, any chance we could have automated test
> for this uncore leader setup logic? like maybe the way
> John did the pmu-events tests? like test will trigger
> only when there's the pmu/events in the system
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>
> 
> thanks,
> jirka

Hi jirka,

JFYI, this is effectively the same patch as I mentioned to you, which 
was a fix for the same WARN:

https://lore.kernel.org/linux-arm-kernel/1587120084-18990-2-git-send-email-john.garry@huawei.com/

but I found that it "fixed" d4953f7ef1a2 ("perf parse-events: Fix 3 use 
after frees found with clang ASANutil/parse-events.c"), based on bisect 
breakage

cheers

> 
> 
>> ---
>>   tools/perf/util/parse-events.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 10107747b361..786eddb6a097 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1629,12 +1629,11 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
>>   		 * event. That can be used to distinguish the leader from
>>   		 * other members, even they have the same event name.
>>   		 */
>> -		if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) {
>> +		if ((leader != evsel) &&
>> +		    !strcmp(leader->pmu_name, evsel->pmu_name)) {
>>   			is_leader = false;
>>   			continue;
>>   		}
>> -		/* The name is always alias name */
>> -		WARN_ON(strcmp(leader->name, evsel->name));
>>   
>>   		/* Store the leader event for each PMU */
>>   		leaders[nr_pmu++] = (uintptr_t) evsel;
>> -- 
>> 2.17.1
>>
> 
> .
> 

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[Jiri Olsa]

On Thu, Apr 30, 2020 at 09:54:18AM +0100, John Garry wrote:
> On 30/04/2020 09:45, Jiri Olsa wrote:
> > On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
> > > A big uncore event group is split into multiple small groups which
> > > only include the uncore events from the same PMU. This has been
> > > supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
> > > uncore event aliases in small groups properly").
> > > 
> > > If the event's PMU name starts to repeat, it must be a new event.
> > > That can be used to distinguish the leader from other members.
> > > But now it only compares the pointer of pmu_name
> > > (leader->pmu_name == evsel->pmu_name).
> > > 
> > > If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
> > > the event list is:
> > > 
> > > evsel->name					evsel->pmu_name
> > > ---------------------------------------------------------------
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_4 (as leader)
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_2
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_0
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_5
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_3
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_1
> > > unc_iio_data_req_of_cpu.mem_write.part1		uncore_iio_4
> > > ......
> > > 
> > > For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
> > > "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
> > > It's not a new leader for this PMU.
> > > 
> > > But if we use "(leader->pmu_name == evsel->pmu_name)", the check
> > > would be failed and the event is stored to leaders[] as a new
> > > PMU leader.
> > > 
> > > So this patch uses strcmp to compare the PMU name between events.
> > > 
> > > Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in small groups properly")
> > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> > 
> > looks good, any chance we could have automated test
> > for this uncore leader setup logic? like maybe the way
> > John did the pmu-events tests? like test will trigger
> > only when there's the pmu/events in the system
> > 
> > Acked-by: Jiri Olsa <jolsa@redhat.com>
> > 
> > thanks,
> > jirka
> 
> Hi jirka,
> 
> JFYI, this is effectively the same patch as I mentioned to you, which was a
> fix for the same WARN:
> 
> https://lore.kernel.org/linux-arm-kernel/1587120084-18990-2-git-send-email-john.garry@huawei.com/
> 
> but I found that it "fixed" d4953f7ef1a2 ("perf parse-events: Fix 3 use
> after frees found with clang ASANutil/parse-events.c"), based on bisect
> breakage

hum right.. sorry I did not recalled that, there's
also the warn removal in here, could you guys also
plz sync on the fixes clauses?

thanks,
jirka

> 
> cheers
> 
> > 
> > 
> > > ---
> > >   tools/perf/util/parse-events.c | 5 ++---
> > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > > index 10107747b361..786eddb6a097 100644
> > > --- a/tools/perf/util/parse-events.c
> > > +++ b/tools/perf/util/parse-events.c
> > > @@ -1629,12 +1629,11 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
> > >   		 * event. That can be used to distinguish the leader from
> > >   		 * other members, even they have the same event name.
> > >   		 */
> > > -		if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) {
> > > +		if ((leader != evsel) &&
> > > +		    !strcmp(leader->pmu_name, evsel->pmu_name)) {
> > >   			is_leader = false;
> > >   			continue;
> > >   		}
> > > -		/* The name is always alias name */
> > > -		WARN_ON(strcmp(leader->name, evsel->name));
> > >   		/* Store the leader event for each PMU */
> > >   		leaders[nr_pmu++] = (uintptr_t) evsel;
> > > -- 
> > > 2.17.1
> > > 
> > 
> > .
> > 
> 

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[John Garry]

On 30/04/2020 12:15, Jiri Olsa wrote:

+

> On Thu, Apr 30, 2020 at 09:54:18AM +0100, John Garry wrote:
>> On 30/04/2020 09:45, Jiri Olsa wrote:
>>> On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
>>>> A big uncore event group is split into multiple small groups which
>>>> only include the uncore events from the same PMU. This has been
>>>> supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
>>>> uncore event aliases in small groups properly").
>>>>
>>>> If the event's PMU name starts to repeat, it must be a new event.
>>>> That can be used to distinguish the leader from other members.
>>>> But now it only compares the pointer of pmu_name
>>>> (leader->pmu_name == evsel->pmu_name).
>>>>
>>>> If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
>>>> the event list is:
>>>>
>>>> evsel->name					evsel->pmu_name
>>>> ---------------------------------------------------------------
>>>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_4 (as leader)
>>>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_2
>>>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_0
>>>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_5
>>>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_3
>>>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_1
>>>> unc_iio_data_req_of_cpu.mem_write.part1		uncore_iio_4
>>>> ......
>>>>
>>>> For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
>>>> "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
>>>> It's not a new leader for this PMU.
>>>>
>>>> But if we use "(leader->pmu_name == evsel->pmu_name)", the check
>>>> would be failed and the event is stored to leaders[] as a new
>>>> PMU leader.
>>>>
>>>> So this patch uses strcmp to compare the PMU name between events.
>>>>
>>>> Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in small groups properly")
>>>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>>>
>>> looks good, any chance we could have automated test
>>> for this uncore leader setup logic? like maybe the way
>>> John did the pmu-events tests? like test will trigger
>>> only when there's the pmu/events in the system
>>>
>>> Acked-by: Jiri Olsa <jolsa@redhat.com>
>>>
>>> thanks,
>>> jirka
>>
>> Hi jirka,
>>
>> JFYI, this is effectively the same patch as I mentioned to you, which was a
>> fix for the same WARN:
>>
>> https://lore.kernel.org/linux-arm-kernel/1587120084-18990-2-git-send-email-john.garry@huawei.com/
>>
>> but I found that it "fixed" d4953f7ef1a2 ("perf parse-events: Fix 3 use
>> after frees found with clang ASANutil/parse-events.c"), based on bisect
>> breakage
> 
> hum right.. sorry I did not recalled that, there's
> also the warn removal in here, could you guys also
> plz sync on the fixes clauses?

I just thought that it fixes d4953f7ef1a2, as I found that the pointer 
equality fails from that commit. I assume the parse-events code was 
sound before then (in that regard). That's all I know :)

Thanks!

> 
> thanks,
> jirka
> 
>>
>> cheers
>>
>>>
>>>
>>>> ---
>>>>    tools/perf/util/parse-events.c | 5 ++---
>>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>>> index 10107747b361..786eddb6a097 100644
>>>> --- a/tools/perf/util/parse-events.c
>>>> +++ b/tools/perf/util/parse-events.c
>>>> @@ -1629,12 +1629,11 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
>>>>    		 * event. That can be used to distinguish the leader from
>>>>    		 * other members, even they have the same event name.
>>>>    		 */
>>>> -		if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) {
>>>> +		if ((leader != evsel) &&
>>>> +		    !strcmp(leader->pmu_name, evsel->pmu_name)) {
>>>>    			is_leader = false;
>>>>    			continue;
>>>>    		}
>>>> -		/* The name is always alias name */
>>>> -		WARN_ON(strcmp(leader->name, evsel->name));
>>>>    		/* Store the leader event for each PMU */
>>>>    		leaders[nr_pmu++] = (uintptr_t) evsel;
>>>> -- 
>>>> 2.17.1
>>>>
>>>
>>> .
>>>
>>
> 
> .
> 

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[Jin, Yao]

Hi John, Jiri,

On 4/30/2020 7:48 PM, John Garry wrote:
> On 30/04/2020 12:15, Jiri Olsa wrote:
> 
> +
> 
>> On Thu, Apr 30, 2020 at 09:54:18AM +0100, John Garry wrote:
>>> On 30/04/2020 09:45, Jiri Olsa wrote:
>>>> On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
>>>>> A big uncore event group is split into multiple small groups which
>>>>> only include the uncore events from the same PMU. This has been
>>>>> supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
>>>>> uncore event aliases in small groups properly").
>>>>>
>>>>> If the event's PMU name starts to repeat, it must be a new event.
>>>>> That can be used to distinguish the leader from other members.
>>>>> But now it only compares the pointer of pmu_name
>>>>> (leader->pmu_name == evsel->pmu_name).
>>>>>
>>>>> If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
>>>>> the event list is:
>>>>>
>>>>> evsel->name                    evsel->pmu_name
>>>>> ---------------------------------------------------------------
>>>>> unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_4 (as leader)
>>>>> unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_2
>>>>> unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_0
>>>>> unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_5
>>>>> unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_3
>>>>> unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_1
>>>>> unc_iio_data_req_of_cpu.mem_write.part1        uncore_iio_4
>>>>> ......
>>>>>
>>>>> For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
>>>>> "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
>>>>> It's not a new leader for this PMU.
>>>>>
>>>>> But if we use "(leader->pmu_name == evsel->pmu_name)", the check
>>>>> would be failed and the event is stored to leaders[] as a new
>>>>> PMU leader.
>>>>>
>>>>> So this patch uses strcmp to compare the PMU name between events.
>>>>>
>>>>> Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in 
>>>>> small groups properly")
>>>>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>>>>
>>>> looks good, any chance we could have automated test
>>>> for this uncore leader setup logic? like maybe the way
>>>> John did the pmu-events tests? like test will trigger
>>>> only when there's the pmu/events in the system
>>>>
>>>> Acked-by: Jiri Olsa <jolsa@redhat.com>
>>>>
>>>> thanks,
>>>> jirka
>>>
>>> Hi jirka,
>>>
>>> JFYI, this is effectively the same patch as I mentioned to you, which was a
>>> fix for the same WARN:
>>>
>>> https://lore.kernel.org/linux-arm-kernel/1587120084-18990-2-git-send-email-john.garry@huawei.com/ 
>>>
>>>
>>> but I found that it "fixed" d4953f7ef1a2 ("perf parse-events: Fix 3 use
>>> after frees found with clang ASANutil/parse-events.c"), based on bisect
>>> breakage
>>
>> hum right.. sorry I did not recalled that, there's
>> also the warn removal in here, could you guys also
>> plz sync on the fixes clauses?
> 
> I just thought that it fixes d4953f7ef1a2, as I found that the pointer equality 
> fails from that commit. I assume the parse-events code was sound before then (in 
> that regard). That's all I know :)
> 
> Thanks!
> 

For 3cdc5c2cb924a, I just think it should use strcmp for pmu_name comparison 
rather than using pointer. But I'm OK to add d4953f7ef1a2 in fixes clauses. :)

Thanks
Jin Yao

>>
>> thanks,
>> jirka
>>
>>>
>>> cheers
>>>
>>>>
>>>>
>>>>> ---
>>>>>    tools/perf/util/parse-events.c | 5 ++---
>>>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>>>> index 10107747b361..786eddb6a097 100644
>>>>> --- a/tools/perf/util/parse-events.c
>>>>> +++ b/tools/perf/util/parse-events.c
>>>>> @@ -1629,12 +1629,11 @@ parse_events__set_leader_for_uncore_aliase(char 
>>>>> *name, struct list_head *list,
>>>>>             * event. That can be used to distinguish the leader from
>>>>>             * other members, even they have the same event name.
>>>>>             */
>>>>> -        if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) {
>>>>> +        if ((leader != evsel) &&
>>>>> +            !strcmp(leader->pmu_name, evsel->pmu_name)) {
>>>>>                is_leader = false;
>>>>>                continue;
>>>>>            }
>>>>> -        /* The name is always alias name */
>>>>> -        WARN_ON(strcmp(leader->name, evsel->name));
>>>>>            /* Store the leader event for each PMU */
>>>>>            leaders[nr_pmu++] = (uintptr_t) evsel;
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>
>>>> .
>>>>
>>>
>>
>> .
>>
> 

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[Jin, Yao]

Hi Jiri,

On 4/30/2020 4:45 PM, Jiri Olsa wrote:
> On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
>> A big uncore event group is split into multiple small groups which
>> only include the uncore events from the same PMU. This has been
>> supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
>> uncore event aliases in small groups properly").
>>
>> If the event's PMU name starts to repeat, it must be a new event.
>> That can be used to distinguish the leader from other members.
>> But now it only compares the pointer of pmu_name
>> (leader->pmu_name == evsel->pmu_name).
>>
>> If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
>> the event list is:
>>
>> evsel->name					evsel->pmu_name
>> ---------------------------------------------------------------
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_4 (as leader)
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_2
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_0
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_5
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_3
>> unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_1
>> unc_iio_data_req_of_cpu.mem_write.part1		uncore_iio_4
>> ......
>>
>> For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
>> "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
>> It's not a new leader for this PMU.
>>
>> But if we use "(leader->pmu_name == evsel->pmu_name)", the check
>> would be failed and the event is stored to leaders[] as a new
>> PMU leader.
>>
>> So this patch uses strcmp to compare the PMU name between events.
>>
>> Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in small groups properly")
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> 
> looks good, any chance we could have automated test
> for this uncore leader setup logic? like maybe the way
> John did the pmu-events tests? like test will trigger
> only when there's the pmu/events in the system
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>
> 
> thanks,
> jirka
> 
> 

I'm considering to use LKP to do the sanity tests for all perf events 
(core/uncore) and perf metrics periodically. It may help us to find the 
regressions on time.

Thanks
Jin Yao

>> ---
>>   tools/perf/util/parse-events.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 10107747b361..786eddb6a097 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1629,12 +1629,11 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
>>   		 * event. That can be used to distinguish the leader from
>>   		 * other members, even they have the same event name.
>>   		 */
>> -		if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) {
>> +		if ((leader != evsel) &&
>> +		    !strcmp(leader->pmu_name, evsel->pmu_name)) {
>>   			is_leader = false;
>>   			continue;
>>   		}
>> -		/* The name is always alias name */
>> -		WARN_ON(strcmp(leader->name, evsel->name));
>>   
>>   		/* Store the leader event for each PMU */
>>   		leaders[nr_pmu++] = (uintptr_t) evsel;
>> -- 
>> 2.17.1
>>
> 

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[Jiri Olsa]

On Thu, Apr 30, 2020 at 09:45:14PM +0800, Jin, Yao wrote:
> Hi Jiri,
> 
> On 4/30/2020 4:45 PM, Jiri Olsa wrote:
> > On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
> > > A big uncore event group is split into multiple small groups which
> > > only include the uncore events from the same PMU. This has been
> > > supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
> > > uncore event aliases in small groups properly").
> > > 
> > > If the event's PMU name starts to repeat, it must be a new event.
> > > That can be used to distinguish the leader from other members.
> > > But now it only compares the pointer of pmu_name
> > > (leader->pmu_name == evsel->pmu_name).
> > > 
> > > If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
> > > the event list is:
> > > 
> > > evsel->name					evsel->pmu_name
> > > ---------------------------------------------------------------
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_4 (as leader)
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_2
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_0
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_5
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_3
> > > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_1
> > > unc_iio_data_req_of_cpu.mem_write.part1		uncore_iio_4
> > > ......
> > > 
> > > For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
> > > "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
> > > It's not a new leader for this PMU.
> > > 
> > > But if we use "(leader->pmu_name == evsel->pmu_name)", the check
> > > would be failed and the event is stored to leaders[] as a new
> > > PMU leader.
> > > 
> > > So this patch uses strcmp to compare the PMU name between events.
> > > 
> > > Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in small groups properly")
> > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> > 
> > looks good, any chance we could have automated test
> > for this uncore leader setup logic? like maybe the way
> > John did the pmu-events tests? like test will trigger
> > only when there's the pmu/events in the system
> > 
> > Acked-by: Jiri Olsa <jolsa@redhat.com>
> > 
> > thanks,
> > jirka
> > 
> > 
> 
> I'm considering to use LKP to do the sanity tests for all perf events
> (core/uncore) and perf metrics periodically. It may help us to find the
> regressions on time.

sounds good ;) thanks

jirka

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[Ian Rogers]

On Thu, Apr 30, 2020 at 8:33 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Apr 30, 2020 at 09:45:14PM +0800, Jin, Yao wrote:
> > Hi Jiri,
> >
> > On 4/30/2020 4:45 PM, Jiri Olsa wrote:
> > > On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
> > > > A big uncore event group is split into multiple small groups which
> > > > only include the uncore events from the same PMU. This has been
> > > > supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
> > > > uncore event aliases in small groups properly").
> > > >
> > > > If the event's PMU name starts to repeat, it must be a new event.
> > > > That can be used to distinguish the leader from other members.
> > > > But now it only compares the pointer of pmu_name
> > > > (leader->pmu_name == evsel->pmu_name).
> > > >
> > > > If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
> > > > the event list is:
> > > >
> > > > evsel->name                                       evsel->pmu_name
> > > > ---------------------------------------------------------------
> > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_4 (as leader)
> > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_2
> > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_0
> > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_5
> > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_3
> > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_1
> > > > unc_iio_data_req_of_cpu.mem_write.part1           uncore_iio_4
> > > > ......
> > > >
> > > > For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
> > > > "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
> > > > It's not a new leader for this PMU.
> > > >
> > > > But if we use "(leader->pmu_name == evsel->pmu_name)", the check
> > > > would be failed and the event is stored to leaders[] as a new
> > > > PMU leader.
> > > >
> > > > So this patch uses strcmp to compare the PMU name between events.
> > > >
> > > > Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in small groups properly")
> > > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> > >
> > > looks good, any chance we could have automated test
> > > for this uncore leader setup logic? like maybe the way
> > > John did the pmu-events tests? like test will trigger
> > > only when there's the pmu/events in the system
> > >
> > > Acked-by: Jiri Olsa <jolsa@redhat.com>
> > >
> > > thanks,
> > > jirka
> > >
> > >
> >
> > I'm considering to use LKP to do the sanity tests for all perf events
> > (core/uncore) and perf metrics periodically. It may help us to find the
> > regressions on time.
>
> sounds good ;) thanks
>
> jirka

I've tested this and would be happy to see this merged. John's bisect
found a memory leak fix of mine as the culprit.

Wrt testing, libbpf is using github/travis CI:
https://github.com/libbpf/libbpf
https://travis-ci.org/libbpf/libbpf
Perhaps that kind of set up can automate testing and lower the
reviewer burden - but there's upfront cost in setting it up.

Thanks,
Ian

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[Arnaldo Carvalho de Melo]

Em Wed, May 06, 2020 at 03:45:59PM -0700, Ian Rogers escreveu:
> On Thu, Apr 30, 2020 at 8:33 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Thu, Apr 30, 2020 at 09:45:14PM +0800, Jin, Yao wrote:
> > > Hi Jiri,
> > >
> > > On 4/30/2020 4:45 PM, Jiri Olsa wrote:
> > > > On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
> > > > > A big uncore event group is split into multiple small groups which
> > > > > only include the uncore events from the same PMU. This has been
> > > > > supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
> > > > > uncore event aliases in small groups properly").
> > > > >
> > > > > If the event's PMU name starts to repeat, it must be a new event.
> > > > > That can be used to distinguish the leader from other members.
> > > > > But now it only compares the pointer of pmu_name
> > > > > (leader->pmu_name == evsel->pmu_name).
> > > > >
> > > > > If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
> > > > > the event list is:
> > > > >
> > > > > evsel->name                                       evsel->pmu_name
> > > > > ---------------------------------------------------------------
> > > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_4 (as leader)
> > > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_2
> > > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_0
> > > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_5
> > > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_3
> > > > > unc_iio_data_req_of_cpu.mem_write.part0           uncore_iio_1
> > > > > unc_iio_data_req_of_cpu.mem_write.part1           uncore_iio_4
> > > > > ......
> > > > >
> > > > > For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
> > > > > "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
> > > > > It's not a new leader for this PMU.
> > > > >
> > > > > But if we use "(leader->pmu_name == evsel->pmu_name)", the check
> > > > > would be failed and the event is stored to leaders[] as a new
> > > > > PMU leader.
> > > > >
> > > > > So this patch uses strcmp to compare the PMU name between events.
> > > > >
> > > > > Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in small groups properly")
> > > > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> > > >
> > > > looks good, any chance we could have automated test
> > > > for this uncore leader setup logic? like maybe the way
> > > > John did the pmu-events tests? like test will trigger
> > > > only when there's the pmu/events in the system
> > > >
> > > > Acked-by: Jiri Olsa <jolsa@redhat.com>
> > > >
> > > > thanks,
> > > > jirka
> > > >
> > > >
> > >
> > > I'm considering to use LKP to do the sanity tests for all perf events
> > > (core/uncore) and perf metrics periodically. It may help us to find the
> > > regressions on time.
> >
> > sounds good ;) thanks
> >
> > jirka
> 
> I've tested this and would be happy to see this merged. John's bisect
> found a memory leak fix of mine as the culprit.
> 
> Wrt testing, libbpf is using github/travis CI:
> https://github.com/libbpf/libbpf
> https://travis-ci.org/libbpf/libbpf
> Perhaps that kind of set up can automate testing and lower the
> reviewer burden - but there's upfront cost in setting it up.

Well, if someone wants to bear this upfront cost, I can provide all the
Dockerfiles + scripts I have to build those images, etc, I just don't
have the time right now to invest in learning about travis, etc.

That would be awesome.

But if people run:

make -C tools/perf build-test

And:

perf test

Before sending pull requests, that would as well be fantastic :)

- Arnaldo

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[Arnaldo Carvalho de Melo]

Em Thu, Apr 30, 2020 at 10:45:29AM +0200, Jiri Olsa escreveu:
> On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
> > A big uncore event group is split into multiple small groups which
> > only include the uncore events from the same PMU. This has been
> > supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
> > uncore event aliases in small groups properly").
> > 
> > If the event's PMU name starts to repeat, it must be a new event.
> > That can be used to distinguish the leader from other members.
> > But now it only compares the pointer of pmu_name
> > (leader->pmu_name == evsel->pmu_name).
> > 
> > If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
> > the event list is:
> > 
> > evsel->name					evsel->pmu_name
> > ---------------------------------------------------------------
> > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_4 (as leader)
> > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_2
> > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_0
> > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_5
> > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_3
> > unc_iio_data_req_of_cpu.mem_write.part0		uncore_iio_1
> > unc_iio_data_req_of_cpu.mem_write.part1		uncore_iio_4
> > ......
> > 
> > For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
> > "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
> > It's not a new leader for this PMU.
> > 
> > But if we use "(leader->pmu_name == evsel->pmu_name)", the check
> > would be failed and the event is stored to leaders[] as a new
> > PMU leader.
> > 
> > So this patch uses strcmp to compare the PMU name between events.
> > 
> > Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore event aliases in small groups properly")
> > Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> 
> looks good, any chance we could have automated test
> for this uncore leader setup logic? like maybe the way
> John did the pmu-events tests? like test will trigger
> only when there's the pmu/events in the system
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks, applied.

- Arnaldo

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[Arnaldo Carvalho de Melo]

Em Thu, Apr 30, 2020 at 09:38:34PM +0800, Jin, Yao escreveu:
> Hi John, Jiri,
> 
> On 4/30/2020 7:48 PM, John Garry wrote:
> > On 30/04/2020 12:15, Jiri Olsa wrote:
> > 
> > +
> > 
> > > On Thu, Apr 30, 2020 at 09:54:18AM +0100, John Garry wrote:
> > > > On 30/04/2020 09:45, Jiri Olsa wrote:
> > > > > On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
> > > > > > A big uncore event group is split into multiple small groups which
> > > > > > only include the uncore events from the same PMU. This has been
> > > > > > supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
> > > > > > uncore event aliases in small groups properly").
> > > > > > 
> > > > > > If the event's PMU name starts to repeat, it must be a new event.
> > > > > > That can be used to distinguish the leader from other members.
> > > > > > But now it only compares the pointer of pmu_name
> > > > > > (leader->pmu_name == evsel->pmu_name).
> > > > > > 
> > > > > > If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
> > > > > > the event list is:
> > > > > > 
> > > > > > evsel->name                    evsel->pmu_name
> > > > > > ---------------------------------------------------------------
> > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_4 (as leader)
> > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_2
> > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_0
> > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_5
> > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_3
> > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_1
> > > > > > unc_iio_data_req_of_cpu.mem_write.part1        uncore_iio_4
> > > > > > ......
> > > > > > 
> > > > > > For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
> > > > > > "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
> > > > > > It's not a new leader for this PMU.
> > > > > > 
> > > > > > But if we use "(leader->pmu_name == evsel->pmu_name)", the check
> > > > > > would be failed and the event is stored to leaders[] as a new
> > > > > > PMU leader.
> > > > > > 
> > > > > > So this patch uses strcmp to compare the PMU name between events.
> > > > > > 
> > > > > > Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore
> > > > > > event aliases in small groups properly")
> > > > > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> > > > > 
> > > > > looks good, any chance we could have automated test
> > > > > for this uncore leader setup logic? like maybe the way
> > > > > John did the pmu-events tests? like test will trigger
> > > > > only when there's the pmu/events in the system
> > > > > 
> > > > > Acked-by: Jiri Olsa <jolsa@redhat.com>
> > > > > 
> > > > > thanks,
> > > > > jirka
> > > > 
> > > > Hi jirka,
> > > > 
> > > > JFYI, this is effectively the same patch as I mentioned to you, which was a
> > > > fix for the same WARN:
> > > > 
> > > > https://lore.kernel.org/linux-arm-kernel/1587120084-18990-2-git-send-email-john.garry@huawei.com/
> > > > 
> > > > 
> > > > but I found that it "fixed" d4953f7ef1a2 ("perf parse-events: Fix 3 use
> > > > after frees found with clang ASANutil/parse-events.c"), based on bisect
> > > > breakage
> > > 
> > > hum right.. sorry I did not recalled that, there's
> > > also the warn removal in here, could you guys also
> > > plz sync on the fixes clauses?
> > 
> > I just thought that it fixes d4953f7ef1a2, as I found that the pointer
> > equality fails from that commit. I assume the parse-events code was
> > sound before then (in that regard). That's all I know :)
> > 
> > Thanks!
> > 
> 
> For 3cdc5c2cb924a, I just think it should use strcmp for pmu_name comparison
> rather than using pointer. But I'm OK to add d4953f7ef1a2 in fixes clauses.
> :)

So, I'm keeping Ian's patch, as I just applied it, and replaced the
fixes clause to:

Fixes: d4953f7ef1a2 ("perf parse-events: Fix 3 use after frees found with clang ASAN")


Would this be ok? Or does John's fix has something else in it (I haven't
checked).

- Arnaldo

Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name

[Ian Rogers]

On Thu, May 7, 2020 at 9:46 AM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Thu, Apr 30, 2020 at 09:38:34PM +0800, Jin, Yao escreveu:
> > Hi John, Jiri,
> >
> > On 4/30/2020 7:48 PM, John Garry wrote:
> > > On 30/04/2020 12:15, Jiri Olsa wrote:
> > >
> > > +
> > >
> > > > On Thu, Apr 30, 2020 at 09:54:18AM +0100, John Garry wrote:
> > > > > On 30/04/2020 09:45, Jiri Olsa wrote:
> > > > > > On Thu, Apr 30, 2020 at 08:36:18AM +0800, Jin Yao wrote:
> > > > > > > A big uncore event group is split into multiple small groups which
> > > > > > > only include the uncore events from the same PMU. This has been
> > > > > > > supported in the commit 3cdc5c2cb924a ("perf parse-events: Handle
> > > > > > > uncore event aliases in small groups properly").
> > > > > > >
> > > > > > > If the event's PMU name starts to repeat, it must be a new event.
> > > > > > > That can be used to distinguish the leader from other members.
> > > > > > > But now it only compares the pointer of pmu_name
> > > > > > > (leader->pmu_name == evsel->pmu_name).
> > > > > > >
> > > > > > > If we use "perf stat -M LLC_MISSES.PCIE_WRITE -a" on cascadelakex,
> > > > > > > the event list is:
> > > > > > >
> > > > > > > evsel->name                    evsel->pmu_name
> > > > > > > ---------------------------------------------------------------
> > > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_4 (as leader)
> > > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_2
> > > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_0
> > > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_5
> > > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_3
> > > > > > > unc_iio_data_req_of_cpu.mem_write.part0        uncore_iio_1
> > > > > > > unc_iio_data_req_of_cpu.mem_write.part1        uncore_iio_4
> > > > > > > ......
> > > > > > >
> > > > > > > For the event "unc_iio_data_req_of_cpu.mem_write.part1" with
> > > > > > > "uncore_iio_4", it should be the event from PMU "uncore_iio_4".
> > > > > > > It's not a new leader for this PMU.
> > > > > > >
> > > > > > > But if we use "(leader->pmu_name == evsel->pmu_name)", the check
> > > > > > > would be failed and the event is stored to leaders[] as a new
> > > > > > > PMU leader.
> > > > > > >
> > > > > > > So this patch uses strcmp to compare the PMU name between events.
> > > > > > >
> > > > > > > Fixes: 3cdc5c2cb924a ("perf parse-events: Handle uncore
> > > > > > > event aliases in small groups properly")
> > > > > > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> > > > > >
> > > > > > looks good, any chance we could have automated test
> > > > > > for this uncore leader setup logic? like maybe the way
> > > > > > John did the pmu-events tests? like test will trigger
> > > > > > only when there's the pmu/events in the system
> > > > > >
> > > > > > Acked-by: Jiri Olsa <jolsa@redhat.com>
> > > > > >
> > > > > > thanks,
> > > > > > jirka
> > > > >
> > > > > Hi jirka,
> > > > >
> > > > > JFYI, this is effectively the same patch as I mentioned to you, which was a
> > > > > fix for the same WARN:
> > > > >
> > > > > https://lore.kernel.org/linux-arm-kernel/1587120084-18990-2-git-send-email-john.garry@huawei.com/
> > > > >
> > > > >
> > > > > but I found that it "fixed" d4953f7ef1a2 ("perf parse-events: Fix 3 use
> > > > > after frees found with clang ASANutil/parse-events.c"), based on bisect
> > > > > breakage
> > > >
> > > > hum right.. sorry I did not recalled that, there's
> > > > also the warn removal in here, could you guys also
> > > > plz sync on the fixes clauses?
> > >
> > > I just thought that it fixes d4953f7ef1a2, as I found that the pointer
> > > equality fails from that commit. I assume the parse-events code was
> > > sound before then (in that regard). That's all I know :)
> > >
> > > Thanks!
> > >
> >
> > For 3cdc5c2cb924a, I just think it should use strcmp for pmu_name comparison
> > rather than using pointer. But I'm OK to add d4953f7ef1a2 in fixes clauses.
> > :)
>
> So, I'm keeping Ian's patch, as I just applied it, and replaced the
> fixes clause to:
>
> Fixes: d4953f7ef1a2 ("perf parse-events: Fix 3 use after frees found with clang ASAN")
>
>
> Would this be ok? Or does John's fix has something else in it (I haven't
> checked).

It is great! This patch is the same as John's except it also removes a
warning that can now no longer happen. As such this patch is the
better and the Fixes is correct.

Thanks,
Ian

> - Arnaldo