Received-SPF: pass (google.com: domain of linux-kernel+bounces-197524-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240529064327.4080674-1-weilin.wang@intel.com>
 <20240529064327.4080674-4-weilin.wang@intel.com> <CAM9d7cgSwZ3PU3H9VNvxQakocEZ_CQEqB9M1o3xTMdJ7F1sufQ@mail.gmail.com>
 <CO6PR11MB56357F58D99B17CD08671A26EEFC2@CO6PR11MB5635.namprd11.prod.outlook.com>
In-Reply-To: <CO6PR11MB56357F58D99B17CD08671A26EEFC2@CO6PR11MB5635.namprd11.prod.outlook.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Fri, 31 May 2024 14:39:50 -0700
Message-ID: <CAM9d7cjbHzgfW2fKQkZ2=gtEMD9N5wG4xaWgPU+N-YvmfhXviQ@mail.gmail.com>
Subject: Re: [RFC PATCH v10 3/8] 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 Thu, May 30, 2024 at 11:46=E2=80=AFPM Wang, Weilin <weilin.wang@intel.co=
m> wrote:
>
>
>
> > -----Original Message-----
> > From: Namhyung Kim <namhyung@kernel.org>
> > Sent: Thursday, May 30, 2024 11:41 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 v10 3/8] perf stat: Fork and launch perf record=
 when
> > perf stat needs to get retire latency value for a metric.
> >
> > On Tue, May 28, 2024 at 11:43=E2=80=AFPM <weilin.wang@intel.com> wrote:
> > >
> > > From: Weilin Wang <weilin.wang@intel.com>
> > >
> > > When retire_latency value is used in a metric formula, evsel would fo=
rk a perf
> > > record process with "-e" and "-W" options. Perf record will collect r=
equired
> > > retire_latency values in parallel while perf stat is collecting count=
ing values.
> > >
> > > At the point of time that perf stat stops counting, evsel would stop =
perf
> > record
> > > by sending sigterm signal to perf record process. Sampled data will b=
e
> > process
> > > to get retire latency value.
> > >
> > > Another thread is required to synchronize between perf stat and perf =
record
> > > when we pass data through pipe.
> > >
> > > Signed-off-by: Weilin Wang <weilin.wang@intel.com>
> > > ---
> > [SNIP]
> > > +int tpebs_set_evsel(struct evsel *evsel, int cpu_map_idx, int thread=
)
> > > +{
> > > +       struct perf_counts_values *count;
> > > +       struct tpebs_retire_lat *t;
> > > +       bool found =3D false;
> > > +       __u64 val;
> > > +       int ret;
> > > +
> > > +       /* Non reitre_latency evsel should never enter this function.=
 */
> > > +       if (!evsel__is_retire_lat(evsel))
> > > +               return -1;
> > > +
> > > +       ret =3D tpebs_stop();
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       count =3D perf_counts(evsel->counts, cpu_map_idx, thread);
> > > +
> > > +       list_for_each_entry(t, &tpebs_results, nd) {
> > > +               if (!strcmp(t->tpebs_name, evsel->name) || !strcmp(t-
> > >tpebs_name, evsel->metric_id)) {
> > > +                       found =3D true;
> > > +                       break;
> > > +               }
> > > +       }
> > > +
> > > +       /* Set ena and run to non-zero */
> > > +       count->ena =3D count->run =3D 1;
> > > +       count->lost =3D 0;
> > > +
> > > +       if (!found) {
> > > +               /*
> > > +                * Set default value or 0 when retire_latency for thi=
s event is
> > > +                * not found from sampling data (enable_tpebs_recordi=
ng not set
> > > +                * or 0 sample recorded).
> > > +                */
> > > +               val =3D 0;
> > > +               return 0;
> > > +       }
> > > +
> > > +       /*
> > > +        * Only set retire_latency value to the first CPU and thread.
> > > +        */
> > > +       if (cpu_map_idx =3D=3D 0 && thread =3D=3D 0) {
> > > +       /* Lost precision when casting from double to __u64. Any
> > improvement? */
> >
> > As I said before I think you can set t->val * 1000 and then
> > set the evsel->scale to 1e3 or 1e-3.
>
> Hi Namhyung,
>
> Sorry if this is a repeated message. I thought I replied to your suggesti=
on
> on this last time. I'm thinking we should keep it like this for now and m=
ake
> this change unless we find the precision loss is critical. Because I thou=
ght
> we don't want to add special code to handle the calculation and final pri=
nt
> to keep code simple.
>
> I kept this comment here so that we don't forget about it. Please let me
> know if you'd like me to remove it.

Please see print_counter_aggrdata().  It's the generic code to print
the event value and it'll display the value multiplied by the scale
(default to 1.0).  So you can keep precision as long as you set the
scale properly (1e-3).

Thanks,
Namhyung