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]

Message-ID: <CAM9d7chSCQViX=VjgqdVn5un0J5MpGsGDGncUyY-K4=oPvvfQA@mail.gmail.com>
Date: Tue, 23 Apr 2024 16:05:48 -0700
From: Namhyung Kim <namhyung@...nel.org>
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-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.

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?

Thanks,
Namhyung