linux-kernel - RE: [RFC PATCH v6 2/5] perf stat: Fork and launch perf record when perf stat needs to get retire latency value for a metric.

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]
Date: Wed, 15 May 2024 05:57:36 +0000
From: "Wang, Weilin" <weilin.wang@...el.com>
To: Namhyung Kim <namhyung@...nel.org>
CC: Ian Rogers <irogers@...gle.com>, Arnaldo Carvalho de Melo
	<acme@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar
	<mingo@...hat.com>, Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>, "Hunter, Adrian" <adrian.hunter@...el.com>, Kan
 Liang <kan.liang@...ux.intel.com>, "linux-perf-users@...r.kernel.org"
	<linux-perf-users@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Taylor, Perry" <perry.taylor@...el.com>,
	"Alt, Samantha" <samantha.alt@...el.com>, "Biggers, Caleb"
	<caleb.biggers@...el.com>
Subject: RE: [RFC PATCH v6 2/5] perf stat: Fork and launch perf record when
 perf stat needs to get retire latency value for a metric.



> -----Original Message-----
> From: Namhyung Kim <namhyung@...nel.org>
> Sent: Wednesday, April 24, 2024 11:50 AM
> To: Wang, Weilin <weilin.wang@...el.com>
> Cc: Ian Rogers <irogers@...gle.com>; Arnaldo Carvalho de Melo
> <acme@...nel.org>; Peter Zijlstra <peterz@...radead.org>; Ingo Molnar
> <mingo@...hat.com>; Alexander Shishkin
> <alexander.shishkin@...ux.intel.com>; Jiri Olsa <jolsa@...nel.org>; Hunter,
> Adrian <adrian.hunter@...el.com>; Kan Liang <kan.liang@...ux.intel.com>;
> linux-perf-users@...r.kernel.org; linux-kernel@...r.kernel.org; Taylor, Perry
> <perry.taylor@...el.com>; Alt, Samantha <samantha.alt@...el.com>; Biggers,
> Caleb <caleb.biggers@...el.com>
> Subject: Re: [RFC PATCH v6 2/5] perf stat: Fork and launch perf record when
> perf stat needs to get retire latency value for a metric.
> 
> On Wed, Apr 24, 2024 at 10:08 AM Wang, Weilin <weilin.wang@...el.com>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Namhyung Kim <namhyung@...nel.org>
> > > Sent: Tuesday, April 23, 2024 4:06 PM
> > > To: Wang, Weilin <weilin.wang@...el.com>
> > > Cc: Ian Rogers <irogers@...gle.com>; Arnaldo Carvalho de Melo
> > > <acme@...nel.org>; Peter Zijlstra <peterz@...radead.org>; Ingo Molnar
> > > <mingo@...hat.com>; Alexander Shishkin
> > > <alexander.shishkin@...ux.intel.com>; Jiri Olsa <jolsa@...nel.org>; Hunter,
> > > Adrian <adrian.hunter@...el.com>; Kan Liang <kan.liang@...ux.intel.com>;
> > > linux-perf-users@...r.kernel.org; linux-kernel@...r.kernel.org; Taylor,
> Perry
> > > <perry.taylor@...el.com>; Alt, Samantha <samantha.alt@...el.com>;
> Biggers,
> > > Caleb <caleb.biggers@...el.com>
> > > Subject: Re: [RFC PATCH v6 2/5] perf stat: Fork and launch perf record
> when
> > > perf stat needs to get retire latency value for a metric.
> > >
> > > On Tue, Apr 23, 2024 at 3:16 PM Wang, Weilin <weilin.wang@...el.com>
> > > wrote:
> > > > > > > > -static int __run_perf_record(void)
> > > > > > > > +static int __run_perf_record(const char **record_argv)
> > > > > > > >  {
> > > > > > > > +       int i = 0;
> > > > > > > > +       struct tpebs_event *e;
> > > > > > > > +
> > > > > > > >         pr_debug("Prepare perf record for retire_latency\n");
> > > > > > > > +
> > > > > > > > +       record_argv[i++] = "perf";
> > > > > > > > +       record_argv[i++] = "record";
> > > > > > > > +       record_argv[i++] = "-W";
> > > > > > > > +       record_argv[i++] = "--synth=no";
> > > > > > > > +
> > > > > > > > +       if (stat_config.user_requested_cpu_list) {
> > > > > > > > +               record_argv[i++] = "-C";
> > > > > > > > +               record_argv[i++] = stat_config.user_requested_cpu_list;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       if (stat_config.system_wide)
> > > > > > > > +               record_argv[i++] = "-a";
> > > > > > > > +
> > > > > > > > +       if (!stat_config.system_wide
> > > > > && !stat_config.user_requested_cpu_list)
> > > > > > > {
> > > > > > > > +               pr_err("Require -a or -C option to run sampling.\n");
> > > > > > > > +               return -ECANCELED;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       list_for_each_entry(e, &stat_config.tpebs_events, nd) {
> > > > > > > > +               record_argv[i++] = "-e";
> > > > > > > > +               record_argv[i++] = e->name;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       record_argv[i++] = "-o";
> > > > > > > > +       record_argv[i++] = PERF_DATA;
> > > > > > > > +
> > > > > > > >         return 0;
> > > > > > > >  }
> > > > > > >
> > > > > > > Still I think it's weird it has 'perf record' in perf stat (despite the
> > > > > > > 'perf stat record').  If it's only Intel thing and we don't have a plan
> > > > > > > to do the same on other arches, we can move it to the arch
> > > > > > > directory and keep the perf stat code simple.
> > > > > >
> > > > > > I'm not sure what is the proper way to solve this. And Ian mentioned
> > > > > > that put code in arch directory could potentially cause other bugs.
> > > > > > So I'm wondering if we could keep this code here for now. I could
> work
> > > > > > on it later if we found it's better to be in arch directory.
> > > > >
> > > > > Maybe somewhere in the util/ and keep the main code minimal.
> > > > > IIUC it's only for very recent (or upcoming?) Intel CPUs and we
> > > > > don't have tests (hopefully can run on other arch/CPUs).
> > > > >
> > > > > So I don't think having it here would help fixing potential bugs.
> > > >
> > > > Do you mean by creating a new file in util/ to hold this code?
> > >
> > > Yeah, maybe util/intel-tpebs.c (if it's better than arch/x86/...) ?
> > >
> > > >
> > > > Yes, this feature is for very recent Intel CPUs. It should only be triggered if
> > > > a metric uses event(s) that has the R modifier in the formula.
> > >
> > > Can we have a test with a fake metric so that we can test
> > > the code on non-(or old-)Intel machines?
> >
> > All the existing metrics in non-(or old-)Intel CPUs should work as usual. So I
> think
> > existing metric tests should work for it.
> >
> > If we want to add a fake metric uses the :R modifier for those platforms, the
> metric
> > should either fail (if the fake metric uses an event not exist on the test
> platform) or
> > return all 0 retire latency data.
> >
> > So, I'm not quite sure what we want the fake metric to test for. Maybe we
> could
> > continue using existing metric tests to ensure all the defined metrics work
> correctly
> > in each machine under test?
> 
> I think it's ok to return all 0 retire latency for fake tPEBS metrics.
> It's just to verify the background record + report logic works ok.

Hi Namhyung,

After think more about how TPEBS and metrics work, I feel should discuss more 
about defining a fake TPEBS metric in unsupported platforms. 
If we'd like a add fake metrics, where should we define and store these metrics? 
Should we add this type of metrics for every platform? All the official metrics 
we publish are defined by architect and stored in JSON files under separate 
directories for each platform. I think it is not a good idea to place fake metrics 
together with real metrics. Could you please let me know if there is any other 
method to define fake metrics for testing?

Thanks,
Weilin

> 
> Thanks,
> Namhyung