From: Ian Rogers <irogers@google.com>
To: "Wang, Weilin" <weilin.wang@intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>,
"Hunter, Adrian" <adrian.hunter@intel.com>,
Kan Liang <kan.liang@linux.intel.com>,
"linux-perf-users@vger.kernel.org"
<linux-perf-users@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Taylor, Perry" <perry.taylor@intel.com>,
"Alt, Samantha" <samantha.alt@intel.com>,
"Biggers, Caleb" <caleb.biggers@intel.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.
Date: Tue, 14 May 2024 23:06:47 -0700 [thread overview]
Message-ID: <CAP-5=fV_mco8cf4yf0VUTGnpNUmQx-VA0S8TpbekAa83c3u5WQ@mail.gmail.com> (raw)
In-Reply-To: <CO6PR11MB5635CC88D0855FA485C383A9EEEC2@CO6PR11MB5635.namprd11.prod.outlook.com>
On Tue, May 14, 2024 at 10:57 PM Wang, Weilin <weilin.wang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Namhyung Kim <namhyung@kernel.org>
> > Sent: Wednesday, April 24, 2024 11:50 AM
> > To: Wang, Weilin <weilin.wang@intel.com>
> > Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> > <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> > <mingo@redhat.com>; Alexander Shishkin
> > <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> > Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor, Perry
> > <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>; Biggers,
> > Caleb <caleb.biggers@intel.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@intel.com>
> > wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Namhyung Kim <namhyung@kernel.org>
> > > > Sent: Tuesday, April 23, 2024 4:06 PM
> > > > To: Wang, Weilin <weilin.wang@intel.com>
> > > > Cc: Ian Rogers <irogers@google.com>; Arnaldo Carvalho de Melo
> > > > <acme@kernel.org>; Peter Zijlstra <peterz@infradead.org>; Ingo Molnar
> > > > <mingo@redhat.com>; Alexander Shishkin
> > > > <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; Hunter,
> > > > Adrian <adrian.hunter@intel.com>; Kan Liang <kan.liang@linux.intel.com>;
> > > > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org; Taylor,
> > Perry
> > > > <perry.taylor@intel.com>; Alt, Samantha <samantha.alt@intel.com>;
> > Biggers,
> > > > Caleb <caleb.biggers@intel.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@intel.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?
We do put fake events/metrics in the "test" arch:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/arch/test/test_soc/cpu/metrics.json
It is something of a pain bringing things here over to the
NO_JEVENTS=1 (ie no python) empty-pmu-events.c file. I agree on not
wanting to pollute real metrics with test metrics, we currently just
use the test metrics to fake up some expression parsing tests:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/pmu-events.c#n811
Thanks,
Ian
> Thanks,
> Weilin
>
> >
> > Thanks,
> > Namhyung
next prev parent reply other threads:[~2024-05-15 6:06 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-29 19:12 [RFC PATCH v6 0/5] TPEBS counting mode support weilin.wang
2024-03-29 19:12 ` [RFC PATCH v6 1/5] perf stat: Parse and find tpebs events when parsing metrics to prepare for perf record sampling weilin.wang
2024-04-01 20:34 ` Namhyung Kim
2024-04-01 21:55 ` Wang, Weilin
2024-04-01 22:06 ` Wang, Weilin
2024-03-29 19:12 ` [RFC PATCH v6 2/5] perf stat: Fork and launch perf record when perf stat needs to get retire latency value for a metric weilin.wang
2024-04-01 20:58 ` Namhyung Kim
2024-04-01 21:23 ` Wang, Weilin
2024-04-23 20:59 ` Namhyung Kim
2024-04-23 22:16 ` Wang, Weilin
2024-04-23 23:05 ` Namhyung Kim
2024-04-24 17:08 ` Wang, Weilin
2024-04-24 18:49 ` Namhyung Kim
2024-04-24 20:08 ` Wang, Weilin
2024-05-15 5:57 ` Wang, Weilin
2024-05-15 6:06 ` Ian Rogers [this message]
2024-05-15 17:14 ` Wang, Weilin
2024-03-29 19:12 ` [RFC PATCH v6 3/5] perf stat: Add retire latency values into the expr_parse_ctx to prepare for final metric calculation weilin.wang
2024-03-29 19:12 ` [RFC PATCH v6 4/5] perf stat: Add retire latency print functions to print out at the very end of print out weilin.wang
2024-04-01 21:04 ` Namhyung Kim
2024-04-01 21:08 ` Wang, Weilin
2024-04-01 21:15 ` Namhyung Kim
2024-04-01 21:40 ` Wang, Weilin
2024-03-29 19:12 ` [RFC PATCH v6 5/5] perf vendor events intel: Add MTL metric json files weilin.wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAP-5=fV_mco8cf4yf0VUTGnpNUmQx-VA0S8TpbekAa83c3u5WQ@mail.gmail.com' \
--to=irogers@google.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=caleb.biggers@intel.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=perry.taylor@intel.com \
--cc=peterz@infradead.org \
--cc=samantha.alt@intel.com \
--cc=weilin.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).