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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aLIixyZOYD3ZE1gh@google.com>
Date: Fri, 29 Aug 2025 14:59:35 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Dmitry Vyukov <dvyukov@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Ian Rogers <irogers@...gle.com>,
	James Clark <james.clark@...aro.org>, Jiri Olsa <jolsa@...nel.org>,
	Kan Liang <kan.liang@...ux.intel.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH 1/1] Revert "perf hist: Fix bogus profiles when filters
 are enabled"

On Fri, Aug 29, 2025 at 07:59:19AM +0200, Dmitry Vyukov wrote:
> On Mon, 25 Aug 2025 at 08:13, Namhyung Kim <namhyung@...nel.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@...nel.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