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

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

[Arnaldo Carvalho de Melo]

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.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 64ff427040c34112..f3f64aff9b882303 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1391,16 +1391,7 @@ hist_entry__cmp_impl(struct perf_hpp_list *hpp_list, struct hist_entry *left,
 	struct hists *hists = left->hists;
 	struct perf_hpp_fmt *fmt;
 	perf_hpp_fmt_cmp_t *fn;
-	int64_t cmp;
-
-	/*
-	 * Never collapse filtered and non-filtered entries.
-	 * Note this is not the same as having an extra (invisible) fmt
-	 * that corresponds to the filtered status.
-	 */
-	cmp = (int64_t)!!left->filtered - (int64_t)!!right->filtered;
-	if (cmp)
-		return cmp;
+	int64_t cmp = 0;
 
 	perf_hpp_list__for_each_sort_list(hpp_list, fmt) {
 		if (ignore_dynamic && perf_hpp__is_dynamic_entry(fmt) &&
-- 
2.50.1

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

[Dmitry Vyukov]

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?

> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/hist.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 64ff427040c34112..f3f64aff9b882303 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -1391,16 +1391,7 @@ hist_entry__cmp_impl(struct perf_hpp_list *hpp_list, struct hist_entry *left,
>         struct hists *hists = left->hists;
>         struct perf_hpp_fmt *fmt;
>         perf_hpp_fmt_cmp_t *fn;
> -       int64_t cmp;
> -
> -       /*
> -        * Never collapse filtered and non-filtered entries.
> -        * Note this is not the same as having an extra (invisible) fmt
> -        * that corresponds to the filtered status.
> -        */
> -       cmp = (int64_t)!!left->filtered - (int64_t)!!right->filtered;
> -       if (cmp)
> -               return cmp;
> +       int64_t cmp = 0;
>
>         perf_hpp_list__for_each_sort_list(hpp_list, fmt) {
>                 if (ignore_dynamic && perf_hpp__is_dynamic_entry(fmt) &&
> --
> 2.50.1
>

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

[Namhyung Kim]

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.

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.

Filtering by unused sort keys would be undefined.  We probably want to
warn users instead.

Thanks,
Namhyung

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

[Arnaldo Carvalho de Melo]

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?

Unsure, I have not investigated it further, just noticed the strange
output with multiple entries for the same symbol, bisected and doing the
revert found what looks like a familiar output.

It is possible that the familiar output is problematic, but I haven't
dig deeper so far.

Since it is a change of behaviour, can we apply the revert and continue
the discussion from that point?

'perf top' decays histogram entries, which is in addition to the
"static" processing done in 'perf report', maybe this is a problem that
is there since forever but statistically, by means of decaying, goes
away and only now we're noticing? Merits investigation.

- Arnaldo

> 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?
> 
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  tools/perf/util/hist.c | 11 +----------
> >  1 file changed, 1 insertion(+), 10 deletions(-)
> >
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index 64ff427040c34112..f3f64aff9b882303 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -1391,16 +1391,7 @@ hist_entry__cmp_impl(struct perf_hpp_list *hpp_list, struct hist_entry *left,
> >         struct hists *hists = left->hists;
> >         struct perf_hpp_fmt *fmt;
> >         perf_hpp_fmt_cmp_t *fn;
> > -       int64_t cmp;
> > -
> > -       /*
> > -        * Never collapse filtered and non-filtered entries.
> > -        * Note this is not the same as having an extra (invisible) fmt
> > -        * that corresponds to the filtered status.
> > -        */
> > -       cmp = (int64_t)!!left->filtered - (int64_t)!!right->filtered;
> > -       if (cmp)
> > -               return cmp;
> > +       int64_t cmp = 0;
> >
> >         perf_hpp_list__for_each_sort_list(hpp_list, fmt) {
> >                 if (ignore_dynamic && perf_hpp__is_dynamic_entry(fmt) &&
> > --
> > 2.50.1
> >

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

[Dmitry Vyukov]

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?

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

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

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

[Namhyung Kim]

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

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

[Dmitry Vyukov]

On Fri, 29 Aug 2025 at 23:59, Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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.

Does it mean that the sorting and merging predicates somehow end up
being different, while the assumption is that they should be the same?

Will it help if filtered field is checked as the last condition,
rather than the first? Then I assume unrelated entries should still be
compared as they were before.

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

Is it the root cause of the reported duplicate entries, or is it a
separate issue?
Arnaldo said to use just 'perf top' w/o any flags, so I am not sure if
this is related or not.