Received-SPF: pass (google.com: domain of linux-kernel+bounces-137538-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240409000659.1727785-1-namhyung@kernel.org> <20240409000659.1727785-3-namhyung@kernel.org>
 <0ec1328a-0731-42a6-b953-163ac5a56deb@linux.intel.com> <CAM9d7cizZLMNa82VxuuvEWEY3vwdbs_iTG9jsogJQBoWMLP7Fw@mail.gmail.com>
 <16587efd-ab12-463a-bd87-7721adfc731d@linux.intel.com>
In-Reply-To: <16587efd-ab12-463a-bd87-7721adfc731d@linux.intel.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Tue, 9 Apr 2024 12:27:43 -0700
Message-ID: <CAM9d7cg65=Hr2Utuuh=8EC7v80y-n1iZ-dyCfDU=kreuovLjfQ@mail.gmail.com>
Subject: Re: [PATCH 3/3] perf report: Add weight[123] output fields
To: "Liang, Kan" <kan.liang@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Ian Rogers <irogers@google.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Peter Zijlstra <peterz@infradead.org>, 
	Ingo Molnar <mingo@kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
	linux-perf-users@vger.kernel.org, Stephane Eranian <eranian@google.com>, 
	Andi Kleen <ak@linux.intel.com>, Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 9, 2024 at 11:18=E2=80=AFAM Liang, Kan <kan.liang@linux.intel.c=
om> wrote:
>
>
>
> On 2024-04-09 12:53 p.m., Namhyung Kim wrote:
> > Hi Kan,
> >
> > On Tue, Apr 9, 2024 at 9:37=E2=80=AFAM Liang, Kan <kan.liang@linux.inte=
l.com> wrote:
> >>
> >>
> >>
> >> On 2024-04-08 8:06 p.m., Namhyung Kim wrote:
> >>> Add weight1, weight2 and weight3 fields to -F/--fields and their alia=
ses
> >>> like 'ins_lat', 'p_stage_cyc' and 'retire_lat'.  Note that they are i=
n
> >>> the sort keys too but the difference is that output fields will sum u=
p
> >>> the weight values and display the average.
> >>>
> >>> In the sort key, users can see the distribution of weight value and I
> >>> think it's confusing we have local vs. global weight for the same wei=
ght.
> >>>
> >>> For example, I experiment with mem-loads events to get the weights.  =
On
> >>> my laptop, it seems only weight1 field is supported.
> >>>
> >>>   $ perf mem record -- perf test -w noploop
> >>>
> >>> Let's look at the noploop function only.  It has 7 samples.
> >>>
> >>>   $ perf script -F event,ip,sym,weight | grep noploop
> >>>   # event                         weight     ip           sym
> >>>   cpu/mem-loads,ldlat=3D30/P:           43     55b3c122bffc noploop
> >>>   cpu/mem-loads,ldlat=3D30/P:           48     55b3c122bffc noploop
> >>>   cpu/mem-loads,ldlat=3D30/P:           38     55b3c122bffc noploop  =
  <--- same weight
> >>>   cpu/mem-loads,ldlat=3D30/P:           38     55b3c122bffc noploop  =
  <--- same weight
> >>>   cpu/mem-loads,ldlat=3D30/P:           59     55b3c122bffc noploop
> >>>   cpu/mem-loads,ldlat=3D30/P:           33     55b3c122bffc noploop
> >>>   cpu/mem-loads,ldlat=3D30/P:           38     55b3c122bffc noploop  =
  <--- same weight
> >>>
> >>> When you use the 'weight' sort key, it'd show entries with a separate
> >>> weight value separately.  Also note that the first entry has 3 sample=
s
> >>> with weight value 38, so they are displayed together and the weight
> >>> value is the sum of 3 samples (114 =3D 38 * 3).
> >>>
> >>>   $ perf report -n -s +weight | grep -e Weight -e noploop
> >>>   # Overhead  Samples  Command   Shared Object   Symbol         Weigh=
t
> >>>        0.53%        3     perf   perf            [.] noploop    114
> >>>        0.18%        1     perf   perf            [.] noploop    59
> >>>        0.18%        1     perf   perf            [.] noploop    48
> >>>        0.18%        1     perf   perf            [.] noploop    43
> >>>        0.18%        1     perf   perf            [.] noploop    33
> >>>
> >>> If you use 'local_weight' sort key, you can see the actualy weight.
> >>>
> >>>   $ perf report -n -s +local_weight | grep -e Weight -e noploop
> >>>   # Overhead  Samples  Command   Shared Object   Symbol         Local=
 Weight
> >>>        0.53%        3     perf   perf            [.] noploop    38
> >>>        0.18%        1     perf   perf            [.] noploop    59
> >>>        0.18%        1     perf   perf            [.] noploop    48
> >>>        0.18%        1     perf   perf            [.] noploop    43
> >>>        0.18%        1     perf   perf            [.] noploop    33
> >>>
> >>> But when you use the -F/--field option instead, you can see the avera=
ge
> >>> weight for the while noploop funciton (as it won't group samples by
> >>
> >> %s/funciton/function/
> >>
> >>> weight value and use the default 'comm,dso,sym' sort keys).
> >>>
> >>>   $ perf report -n -F +weight | grep -e Weight -e noploop
> >>>   # Overhead  Samples  Weight1  Command  Shared Object  Symbol
> >>>        1.23%        7     42.4  perf     perf           [.] noploop
> >>
> >> I think the current +weight shows the sum of weight1 of all samples,
> >> (global weight). With this patch, it becomes an average (local_weight)=
.
> >> The definition change may break the existing user script.
> >>
> >> Ideally, I think we should keep the meaning of the weight and
> >> local_weight as is.
> >
> > Hmm.. then we may add 'avg_weight' or something.
> >
> > But note that there's a subtle difference in the usage.  If you use
> > 'weight' as a sort key (-s weight) it'd keep the existing behavior
> > that shows the sum (global_weight).  It'd show average only if
> > you use it as an output field (-F weight).
> >
>
> As my understanding, the -F weight is implicitly replaced by the -F
> weight1 with this patch. There is no way to get the sum of weight with
> -F anymore.

Right.

>
> I think that's a user visible behavior change. At least, we have to warn
> the end user with a message, e.g., "weight is not supported with -F
> anymore. Using weight1 to instead". Only updating the doc may not be enou=
gh.

I understand your concern.  I can add the warning.

>
> > The issue of the sort key is that it cannot have the total sum
> > of weights for a function.  It'll have separate entries for each
> > weight for each function like in the above example.
> >
>
> That seems to be a different issue. If the total sum of weights for a
> function is required, we should fix the existing "weight".

Yeah, I guess that's more reasonable behavior.  But I'm not sure
how we can fix it without breaking the existing behavior.

Actually this is my approach to keep the behavior for the "sort" key.
I think users are more familiar with -s (--sort) rather than the -F
(--fields) option.  That's why I'd like to "break" that part only.

Thanks,
Namhyung