Received-SPF: pass (google.com: domain of linux-kernel+bounces-138645-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <014e70fb-e49a-4d5c-9daf-a080e0f12b11@linux.intel.com>
Date: Wed, 10 Apr 2024 09:40:02 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/3] perf report: Add weight[123] output fields
To: Namhyung Kim <namhyung@kernel.org>
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>
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>
 <CAM9d7cg65=Hr2Utuuh=8EC7v80y-n1iZ-dyCfDU=kreuovLjfQ@mail.gmail.com>
Content-Language: en-US
From: "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <CAM9d7cg65=Hr2Utuuh=8EC7v80y-n1iZ-dyCfDU=kreuovLjfQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 2024-04-09 3:27 p.m., Namhyung Kim wrote:
>>>>> 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 enough.
> 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.
>

I did some experiments and found that with the -F weight option, the
hist_entry__cmp() compares the newly added field, weight, as well.
That may not the behavior we want.

I think the expected behavior is that all the samples still be sorted by
symbol, but just add a new field to show the sum of the weight.
So perf probably should not cmp any newly added field.

Another issue is that the current code will only use the weight from the
first sample. If perf can avoid the cmp of the weight, it still needs to
save all the weights and add them up.

I'm not sure how hard the fix is or maybe it's too ugly. Just for your
reference.

> 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.
> 

Yes, if we have to "break" something, it should be -F.
I'm OK with it as long as there are proper warnings that can tell users
that it's broken.

Thanks,
Kan