* [PATCH] perf parse-events: Use strcmp to compare the PMU name
@ 2020-04-30 0:36 Jin Yao
2020-04-30 8:45 ` Jiri Olsa
0 siblings, 1 reply; 13+ messages in thread
From: Jin Yao @ 2020-04-30 0:36 UTC (permalink / raw)
To: acme, jolsa, peterz, mingo, alexander.shishkin
Cc: Linux-kernel, ak, kan.liang, yao.jin, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-04-30 0:36 [PATCH] perf parse-events: Use strcmp to compare the PMU name Jin Yao
@ 2020-04-30 8:45 ` Jiri Olsa
2020-04-30 8:54 ` John Garry
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Jiri Olsa @ 2020-04-30 8:45 UTC (permalink / raw)
To: Jin Yao
Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin, John Garry
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-04-30 8:45 ` Jiri Olsa
@ 2020-04-30 8:54 ` John Garry
2020-04-30 11:15 ` Jiri Olsa
2020-04-30 13:45 ` Jin, Yao
2020-05-07 16:44 ` Arnaldo Carvalho de Melo
2 siblings, 1 reply; 13+ messages in thread
From: John Garry @ 2020-04-30 8:54 UTC (permalink / raw)
To: Jiri Olsa, Jin Yao
Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin
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
>>
>
> .
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-04-30 8:54 ` John Garry
@ 2020-04-30 11:15 ` Jiri Olsa
2020-04-30 11:48 ` John Garry
0 siblings, 1 reply; 13+ messages in thread
From: Jiri Olsa @ 2020-04-30 11:15 UTC (permalink / raw)
To: John Garry
Cc: Jin Yao, acme, jolsa, peterz, mingo, alexander.shishkin,
Linux-kernel, ak, kan.liang, yao.jin
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
> > >
> >
> > .
> >
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-04-30 11:15 ` Jiri Olsa
@ 2020-04-30 11:48 ` John Garry
2020-04-30 13:38 ` Jin, Yao
0 siblings, 1 reply; 13+ messages in thread
From: John Garry @ 2020-04-30 11:48 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jin Yao, acme, jolsa, peterz, mingo, alexander.shishkin,
Linux-kernel, ak, kan.liang, yao.jin, irogers
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
>>>>
>>>
>>> .
>>>
>>
>
> .
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-04-30 11:48 ` John Garry
@ 2020-04-30 13:38 ` Jin, Yao
2020-05-07 16:46 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 13+ messages in thread
From: Jin, Yao @ 2020-04-30 13:38 UTC (permalink / raw)
To: John Garry, Jiri Olsa
Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin, irogers
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
>>>>>
>>>>
>>>> .
>>>>
>>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-04-30 8:45 ` Jiri Olsa
2020-04-30 8:54 ` John Garry
@ 2020-04-30 13:45 ` Jin, Yao
2020-04-30 15:32 ` Jiri Olsa
2020-05-07 16:44 ` Arnaldo Carvalho de Melo
2 siblings, 1 reply; 13+ messages in thread
From: Jin, Yao @ 2020-04-30 13:45 UTC (permalink / raw)
To: Jiri Olsa
Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin, John Garry
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
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-04-30 13:45 ` Jin, Yao
@ 2020-04-30 15:32 ` Jiri Olsa
2020-05-06 22:45 ` Ian Rogers
0 siblings, 1 reply; 13+ messages in thread
From: Jiri Olsa @ 2020-04-30 15:32 UTC (permalink / raw)
To: Jin, Yao
Cc: acme, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
kan.liang, yao.jin, John Garry
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-04-30 15:32 ` Jiri Olsa
@ 2020-05-06 22:45 ` Ian Rogers
2020-05-06 23:46 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 13+ messages in thread
From: Ian Rogers @ 2020-05-06 22:45 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jin, Yao, Arnaldo Carvalho de Melo, Jiri Olsa, Peter Zijlstra,
Ingo Molnar, Alexander Shishkin, LKML, Andi Kleen, kan.liang,
yao.jin, John Garry
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-05-06 22:45 ` Ian Rogers
@ 2020-05-06 23:46 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-06 23:46 UTC (permalink / raw)
To: Ian Rogers
Cc: Jiri Olsa, Jin, Yao, Jiri Olsa, Peter Zijlstra, Ingo Molnar,
Alexander Shishkin, LKML, Andi Kleen, kan.liang, yao.jin,
John Garry
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-04-30 8:45 ` Jiri Olsa
2020-04-30 8:54 ` John Garry
2020-04-30 13:45 ` Jin, Yao
@ 2020-05-07 16:44 ` Arnaldo Carvalho de Melo
2 siblings, 0 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-07 16:44 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jin Yao, jolsa, peterz, mingo, alexander.shishkin, Linux-kernel,
ak, kan.liang, yao.jin, John Garry
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-04-30 13:38 ` Jin, Yao
@ 2020-05-07 16:46 ` Arnaldo Carvalho de Melo
2020-05-07 17:24 ` Ian Rogers
0 siblings, 1 reply; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-07 16:46 UTC (permalink / raw)
To: Jin, Yao
Cc: John Garry, Jiri Olsa, jolsa, peterz, mingo, alexander.shishkin,
Linux-kernel, ak, kan.liang, yao.jin, irogers
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name
2020-05-07 16:46 ` Arnaldo Carvalho de Melo
@ 2020-05-07 17:24 ` Ian Rogers
0 siblings, 0 replies; 13+ messages in thread
From: Ian Rogers @ 2020-05-07 17:24 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jin, Yao, John Garry, Jiri Olsa, Jiri Olsa, Peter Zijlstra,
Ingo Molnar, Alexander Shishkin, LKML, Andi Kleen, kan.liang,
yao.jin
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-05-07 17:24 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-30 0:36 [PATCH] perf parse-events: Use strcmp to compare the PMU name Jin Yao
2020-04-30 8:45 ` Jiri Olsa
2020-04-30 8:54 ` John Garry
2020-04-30 11:15 ` Jiri Olsa
2020-04-30 11:48 ` John Garry
2020-04-30 13:38 ` Jin, Yao
2020-05-07 16:46 ` Arnaldo Carvalho de Melo
2020-05-07 17:24 ` Ian Rogers
2020-04-30 13:45 ` Jin, Yao
2020-04-30 15:32 ` Jiri Olsa
2020-05-06 22:45 ` Ian Rogers
2020-05-06 23:46 ` Arnaldo Carvalho de Melo
2020-05-07 16:44 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).