linux-kernel - Re: [PATCH] perf jevents: Don't stop at the first matched pmu when searching a events table

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zqo8kVXkN_UaTp6f@x1>
Date: Wed, 31 Jul 2024 10:30:57 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: kan.liang@...ux.intel.com, namhyung@...nel.org, adrian.hunter@...el.com,
	peterz@...radead.org, mingo@...nel.org,
	linux-kernel@...r.kernel.org,
	kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH] perf jevents: Don't stop at the first matched pmu when
 searching a events table

On Tue, Jul 16, 2024 at 09:30:01AM -0700, Ian Rogers wrote:
> On Tue, Jul 16, 2024 at 6:29 AM <kan.liang@...ux.intel.com> wrote:
> >
> > From: Kan Liang <kan.liang@...ux.intel.com>
> >
> > The "perf all PMU test" fails on a Coffee Lake machine.
> >
> > The failure is caused by the below change in the commit e2641db83f18
> > ("perf vendor events: Add/update skylake events/metrics").
> >
> > +    {
> > +        "BriefDescription": "This 48-bit fixed counter counts the UCLK cycles",
> > +        "Counter": "FIXED",
> > +        "EventCode": "0xff",
> > +        "EventName": "UNC_CLOCK.SOCKET",
> > +        "PerPkg": "1",
> > +        "PublicDescription": "This 48-bit fixed counter counts the UCLK cycles.",
> > +        "Unit": "cbox_0"
> >      }
> >
> > The other cbox events have the unit name "CBOX", while the fixed counter
> > has a unit name "cbox_0". So the events_table will maintain separate
> > entries for cbox and cbox_0.
> >
> > The perf_pmus__print_pmu_events() calculates the total number of events,
> > allocate an aliases buffer, store all the events into the buffer, sort,
> > and print all the aliases one by one.
> >
> > The problem is that the calculated total number of events doesn't match
> > the stored events in the aliases buffer.
> >
> > The perf_pmu__num_events() is used to calculate the number of events. It
> > invokes the pmu_events_table__num_events() to go through the entire
> > events_table to find all events. Because of the
> > pmu_uncore_alias_match(), the suffix of uncore PMU will be ignored. So
> > the events for cbox and cbox_0 are all counted.
> >
> > When storing events into the aliases buffer, the
> > perf_pmu__for_each_event() only process the events for cbox.
> >
> > Since a bigger buffer was allocated, the last entry are all 0.
> > When printing all the aliases, null will be outputted, and trigger the
> > failure.
> >
> > The mismatch was introduced from the commit e3edd6cf6399 ("perf
> > pmu-events: Reduce processed events by passing PMU"). The
> > pmu_events_table__for_each_event() stops immediately once a pmu is set.
> > But for uncore, especially this case, the method is wrong and mismatch
> > what perf does in the perf_pmu__num_events().
> >
> > With the patch,
> > $ perf list pmu | grep -A 1 clock.socket
> >    unc_clock.socket
> >         [This 48-bit fixed counter counts the UCLK cycles. Unit: uncore_cbox_0
> > $ perf test "perf all PMU test"
> >   107: perf all PMU test                                               : Ok
> >
> > Reported-by: kernel test robot <oliver.sang@...el.com>
> > Closes: https://lore.kernel.org/all/202407101021.2c8baddb-oliver.sang@intel.com/
> > Fixes: e3edd6cf6399 ("perf pmu-events: Reduce processed events by passing PMU")
> > Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>
> 
> Awesome sauce, thanks!
> 
> Reviewed-by: Ian Rogers <irogers@...gle.com>

Thanks, applied to tmp.perf-tools-next,

- Arnaldo