Received-SPF: pass (google.com: domain of linux-kernel+bounces-201501-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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>
 <CAM9d7cjbHzgfW2fKQkZ2=gtEMD9N5wG4xaWgPU+N-YvmfhXviQ@mail.gmail.com>
 <CO6PR11MB563511F1A6165F0F4B30C4DFEEF82@CO6PR11MB5635.namprd11.prod.outlook.com>
 <Zl-WCEhhxBtiGGhn@google.com>
In-Reply-To: <Zl-WCEhhxBtiGGhn@google.com>
From: Ian Rogers <irogers@google.com>
Date: Tue, 4 Jun 2024 15:41:50 -0700
Message-ID: <CAP-5=fW41UxUDO-FSymMX_R5nEJDnV7FOjfhW5=XGCVj=TSYKQ@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: Namhyung Kim <namhyung@kernel.org>
Cc: "Wang, Weilin" <weilin.wang@intel.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 Tue, Jun 4, 2024 at 3:32=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> w=
rote:
>
> On Tue, Jun 04, 2024 at 08:00:26PM +0000, Wang, Weilin wrote:
>
> > > On Thu, May 30, 2024 at 11:46=E2=80=AFPM Wang, Weilin <weilin.wang@in=
tel.com>
> > > > > > +       /*
> > > > > > +        * Only set retire_latency value to the first CPU and t=
hread.
> > > > > > +        */
> > > > > > +       if (cpu_map_idx =3D=3D 0 && thread =3D=3D 0) {
> > > > > > +       /* Lost precision when casting from double to __u64. An=
y
> > > > > 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 su=
ggestion
> > > > on this last time. I'm thinking we should keep it like this for now=
 and make
> > > > this change unless we find the precision loss is critical. Because =
I thought
> > > > we don't want to add special code to handle the calculation and fin=
al print
> > > > to keep code simple.
> > > >
> > > > I kept this comment here so that we don't forget about it. Please l=
et 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).
> >
> > I could see the retire_latency is printed correctly after set the evsel=
->scale to 1e-3
> > and multiply the t->val * 1000. However, this scale is not used in metr=
ic calculations.
> > We need to add code in metric calculation or display part to scale it a=
s well. Is that
> > acceptable or do you have other suggestions?
>
> Hmm.. I don't know if other metric already dealt with the scale like with
> RAPL events.. If not, I think it's reasonable to add that to the metric
> calculation.
>
> Ian, what do you think?

Tbh, I don't understand the conversation and it looks like we're in
the weeds. In metrics the scale/unit from the event aren't used - that
is all events in a metric are the unscaled quantities unless something
is broken.

Thanks,
Ian

> Thanks,
> Namhyung
>