Received-SPF: pass (google.com: domain of linux-kernel+bounces-157557-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240329191224.1046866-1-weilin.wang@intel.com>
 <20240329191224.1046866-3-weilin.wang@intel.com> <CAM9d7cjBfTM2bjkrvN5XWcXJ=OyBOU06fBN=+eWnDSdP2dkD4A@mail.gmail.com>
 <CO6PR11MB56351C2AB4BE15C5371E29C2EE3F2@CO6PR11MB5635.namprd11.prod.outlook.com>
 <CAM9d7cjORNS9h7v6p2fg8OXsZMpeBODzTSCQNZ5zAea-baFKNQ@mail.gmail.com>
 <CO6PR11MB5635C7D8C91FEDA17EC4BCDEEE112@CO6PR11MB5635.namprd11.prod.outlook.com>
 <CAM9d7chSCQViX=VjgqdVn5un0J5MpGsGDGncUyY-K4=oPvvfQA@mail.gmail.com> <CO6PR11MB56359754A01778EA3EA53981EE102@CO6PR11MB5635.namprd11.prod.outlook.com>
In-Reply-To: <CO6PR11MB56359754A01778EA3EA53981EE102@CO6PR11MB5635.namprd11.prod.outlook.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Wed, 24 Apr 2024 11:49:38 -0700
Message-ID: <CAM9d7cj2Zmd_FMBzf=H6K6jRmYLYuR6mpcyhC0_neTvur0=pyw@mail.gmail.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.
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-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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 24, 2024 at 10:08=E2=80=AFAM Wang, Weilin <weilin.wang@intel.co=
m> 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>; Hun=
ter,
> > 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>; Bigge=
rs,
> > 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=E2=80=AFPM Wang, Weilin <weilin.wang@intel=
com>
> > wrote:
> > > > > > > -static int __run_perf_record(void)
> > > > > > > +static int __run_perf_record(const char **record_argv)
> > > > > > >  {
> > > > > > > +       int i =3D 0;
> > > > > > > +       struct tpebs_event *e;
> > > > > > > +
> > > > > > >         pr_debug("Prepare perf record for retire_latency\n");
> > > > > > > +
> > > > > > > +       record_argv[i++] =3D "perf";
> > > > > > > +       record_argv[i++] =3D "record";
> > > > > > > +       record_argv[i++] =3D "-W";
> > > > > > > +       record_argv[i++] =3D "--synth=3Dno";
> > > > > > > +
> > > > > > > +       if (stat_config.user_requested_cpu_list) {
> > > > > > > +               record_argv[i++] =3D "-C";
> > > > > > > +               record_argv[i++] =3D stat_config.user_request=
ed_cpu_list;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       if (stat_config.system_wide)
> > > > > > > +               record_argv[i++] =3D "-a";
> > > > > > > +
> > > > > > > +       if (!stat_config.system_wide
> > > > && !stat_config.user_requested_cpu_list)
> > > > > > {
> > > > > > > +               pr_err("Require -a or -C option to run sampli=
ng.\n");
> > > > > > > +               return -ECANCELED;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       list_for_each_entry(e, &stat_config.tpebs_events, nd)=
 {
> > > > > > > +               record_argv[i++] =3D "-e";
> > > > > > > +               record_argv[i++] =3D e->name;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       record_argv[i++] =3D "-o";
> > > > > > > +       record_argv[i++] =3D PERF_DATA;
> > > > > > > +
> > > > > > >         return 0;
> > > > > > >  }
> > > > > >
> > > > > > Still I think it's weird it has 'perf record' in perf stat (des=
pite the
> > > > > > 'perf stat record').  If it's only Intel thing and we don't hav=
e 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 mentio=
ned
> > > > > that put code in arch directory could potentially cause other bug=
s.
> > > > > 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 tr=
iggered 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 tes=
t 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 wo=
rk 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.

Thanks,
Namhyung