* [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 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
* 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
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).