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
