Messages in this thread | | | Subject | Re: [PATCH] perf parse-events: Use strcmp to compare the PMU name | From | "Jin, Yao" <> | Date | Thu, 30 Apr 2020 21:38:34 +0800 |
| |
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 >>>>> >>>> >>>> . >>>> >>> >> >> . >> >
| |