Re: [PATCH 1/1] Revert "perf hist: Fix bogus profiles when filters are enabled"

[Namhyung Kim <namhyung@kernel.org>]

On Fri, Aug 29, 2025 at 07:59:19AM +0200, Dmitry Vyukov wrote:
> On Mon, 25 Aug 2025 at 08:13, Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hello,
> >
> > On Wed, Aug 20, 2025 at 06:14:08PM -0700, Dmitry Vyukov wrote:
> > > On Wed, 20 Aug 2025 at 10:23, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > >
> > > > This reverts commit 8b4799e4f0f40a4ec737bf870aa38d06288bf0fb.
> > > >
> > > > Not combining entries in 'perf top', so we're getting multiple lines for
> > > > the same symbol, with the same address.
> > > >
> > > > To test it, simply run 'perf top', then do /acpi to see just symbols
> > > > starting with acpi_ and notice that there are various lines with the
> > > > same symbol, press V to see the address and its the same.
> > >
> > > With this revert, does it show 1 entry but with a wrong percent?
> > > I am not sure why there are 2 entries for the same symbol, but if we
> > > merge them, we can sum of percents. Is it the right thing to do?
> >
> > I don't think it'd have a wrong percent.  The hists maintain stats for
> > filtered entries separately.
> 
> I still don't fully follow.
> 
> If we merge a filtered entry into non-filtered entry (with the
> revert), now we attribute what was filtered out to the non-filtered
> entry, and the non-filtered entry has wrong overhead, no?

Oh, my bad.  I thought we track both periods in the hist_entry, but it
seems it's only in the hists for total.

> 
> If we merge the other way around: non-filtered entry into filtered
> entry, then we won't show it at all.
> 
> > Based on the position of filtered entries in the RB tree, I think it
> > might not merge correct samples together and create multiple entries
> > with the same info.
> 
> The second thing I don't understand: without this revert we don't
> merge filtered and non-filtered entries, top shows duplicate entries,
> does it mean it shows filtered out entries? This also looks wrong...

I checked those duplicated entries all non-filtered (filtered = 0).
I suspect an entry can miss existing one (to be merged) when it sees
unrelated filtered entries in the middle of RB tree traversal.

> 
> > Filtering by unused sort keys would be undefined.  We probably want to
> > warn users instead.
> 
> Do you mean that the filtered=1 is set incorrectly in this case?
> Do you mean that with this revert 2 bugs just compensate each other by
> luck: we wrongly set filtered=1, and then wrongly merge them together,
> so it all works out in the end?

I mean you should not allow users to use filters not listed in the sort
keys.  For example, `perf report -s comm --tid=123` would return error.
They can use `perf report -s tid` or `perf report -s comm,tid -H`.

Thanks,
Namhyung