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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
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
>
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 <[email protected]>
>
> 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 <[email protected]>
>
> 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/[email protected]/
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
>>
>
> .
>
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 <[email protected]>
> >
> > 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 <[email protected]>
> >
> > 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/[email protected]/
>
> 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
> > >
> >
> > .
> >
>
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>>
>>> 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/[email protected]/
>>
>> 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
>>>>
>>>
>>> .
>>>
>>
>
> .
>
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>>
>>>> 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/[email protected]/
>>>
>>>
>>> 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
>>>>>
>>>>
>>>> .
>>>>
>>>
>>
>> .
>>
>
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 <[email protected]>
>
> 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 <[email protected]>
>
> 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
>>
>
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 <[email protected]>
> >
> > 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 <[email protected]>
> >
> > 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
On Thu, Apr 30, 2020 at 8:33 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > >
> > > 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
Em Wed, May 06, 2020 at 03:45:59PM -0700, Ian Rogers escreveu:
> On Thu, Apr 30, 2020 at 8:33 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > >
> > > > 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
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 <[email protected]>
>
> 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 <[email protected]>
Thanks, applied.
- Arnaldo
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 <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > >
> > > > > 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/[email protected]/
> > > >
> > > >
> > > > 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
On Thu, May 7, 2020 at 9:46 AM Arnaldo Carvalho de Melo
<[email protected]> 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 <[email protected]>
> > > > > >
> > > > > > 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 <[email protected]>
> > > > > >
> > > > > > 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/[email protected]/
> > > > >
> > > > >
> > > > > 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