linux-kernel - Re: [RFC/PATCHSET 0/5] perf tools: Support deferred user callchains (v2)

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: <CAP-5=fUT-60ff-hPS9Anizq9XxiqAOJXYyQqeVoHBYzMvd0L=g@mail.gmail.com>
Date: Wed, 18 Sep 2024 15:39:31 +0200
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>, 
	Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>, 
	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, 
	LKML <linux-kernel@...r.kernel.org>, linux-perf-users@...r.kernel.org, 
	Josh Poimboeuf <jpoimboe@...nel.org>, Steven Rostedt <rostedt@...dmis.org>, 
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Indu Bhagat <indu.bhagat@...cle.com>, 
	linux-toolchains@...r.kernel.org
Subject: Re: [RFC/PATCHSET 0/5] perf tools: Support deferred user callchains (v2)

On Wed, Sep 18, 2024 at 11:38 AM Namhyung Kim <namhyung@...nel.org> wrote:
>
> Hi Ian,
>
> On Wed, Sep 18, 2024 at 08:38:22AM +0200, Ian Rogers wrote:
> > On Wed, Sep 18, 2024 at 12:28 AM Namhyung Kim <namhyung@...nel.org> wrote:
> > >
> > > Hello,
> > >
> > > This is a counterpart for Josh's kernel change v2 [1] to support deferred
> > > user callchains.  The change is transparent and users should not notice
> > > anything with the deferred callchains.
> > >
> > >   $ perf record -g sleep 1
> > >
> > > I added --[no-]merge-callchains option to control output of perf script.
> > > You can verify it has the deferred callchains like this:
> > >
> > >   $ perf script --no-merge-callchains
> > >   perf     801 [000]    18.031793:          1 cycles:P:
> > >           ffffffff91a14c36 __intel_pmu_enable_all.isra.0+0x56 ([kernel.kallsyms])
> > >           ffffffff91d373e9 perf_ctx_enable+0x39 ([kernel.kallsyms])
> > >           ffffffff91d36af7 event_function+0xd7 ([kernel.kallsyms])
> > >           ffffffff91d34222 remote_function+0x42 ([kernel.kallsyms])
> > >           ffffffff91c1ebe1 generic_exec_single+0x61 ([kernel.kallsyms])
> > >           ffffffff91c1edac smp_call_function_single+0xec ([kernel.kallsyms])
> > >           ffffffff91d37a9d event_function_call+0x10d ([kernel.kallsyms])
> > >           ffffffff91d33557 perf_event_for_each_child+0x37 ([kernel.kallsyms])
> > >           ffffffff91d47324 _perf_ioctl+0x204 ([kernel.kallsyms])
> > >           ffffffff91d47c43 perf_ioctl+0x33 ([kernel.kallsyms])
> > >           ffffffff91e2f216 __x64_sys_ioctl+0x96 ([kernel.kallsyms])
> > >           ffffffff9265f1ae do_syscall_64+0x9e ([kernel.kallsyms])
> > >           ffffffff92800130 entry_SYSCALL_64+0xb0 ([kernel.kallsyms])
> > >
> > >   perf     801 [000]    18.031814: DEFERRED CALLCHAIN
> > >                   7fb5fc22034b __GI___ioctl+0x3b (/usr/lib/x86_64-linux-gnu/libc.so.6)
> > >
> > >   ...
> > >
> > > When the callchain is merged (it's the default) it'd look like below:
> > >
> > >   $ perf script
> > >   perf     801 [000]    18.031793:          1 cycles:P:
> > >           ffffffff91a14c36 __intel_pmu_enable_all.isra.0+0x56 ([kernel.kallsyms])
> > >           ffffffff91d373e9 perf_ctx_enable+0x39 ([kernel.kallsyms])
> > >           ffffffff91d36af7 event_function+0xd7 ([kernel.kallsyms])
> > >           ffffffff91d34222 remote_function+0x42 ([kernel.kallsyms])
> > >           ffffffff91c1ebe1 generic_exec_single+0x61 ([kernel.kallsyms])
> > >           ffffffff91c1edac smp_call_function_single+0xec ([kernel.kallsyms])
> > >           ffffffff91d37a9d event_function_call+0x10d ([kernel.kallsyms])
> > >           ffffffff91d33557 perf_event_for_each_child+0x37 ([kernel.kallsyms])
> > >           ffffffff91d47324 _perf_ioctl+0x204 ([kernel.kallsyms])
> > >           ffffffff91d47c43 perf_ioctl+0x33 ([kernel.kallsyms])
> > >           ffffffff91e2f216 __x64_sys_ioctl+0x96 ([kernel.kallsyms])
> > >           ffffffff9265f1ae do_syscall_64+0x9e ([kernel.kallsyms])
> > >           ffffffff92800130 entry_SYSCALL_64+0xb0 ([kernel.kallsyms])
> > >                   7fb5fc22034b __GI___ioctl+0x3b (/usr/lib/x86_64-linux-gnu/libc.so.6)
> > >
> > >   ...
> > >
> > > Notice that the last line and it has the __GI___ioctl in the same
> > > callchain.  It should work with other tools like perf report.
> >
> > Hi Namhyung, I think this is interesting work!
> >
> > The issue feels similar to leader sampling and some of the unpicking
> > of that we've been dealing with. With leader sampling it was added and
> > then the dispatch of events modified so that tools wouldn't see leader
> > samples, instead new events would be synthesized based on the leader
> > sample data. However, the leader sample event wasn't changed and so
> > now we have multiple repeated events and perf inject wouldn't just
> > pass through a perf data file.
> >
> > What I'm expecting based on this description is that a deferred call
> > chain will be merged with a regular one, however, perf inject isn't
> > updated to drop the deferred callchain so now we have the deferred
> > callchain event twice.
> >
> > My feeling is that making the dispatch of events to tools "smart" is a
> > false economy. Tools can add handlers for these events easily enough.
> > What's harder is undoing the smartness when it does things that lead
> > to duplicated events and the like. I'm not a fan of how leader
> > sampling was implemented and I still think it odd that with perf
> > script we see invented events when trying to just dump the contents of
> > a perf.data file.
>
> That's why I added perf_tool.merge_deferred_callchains flag to control
> the behavior.  I haven't implemented it to perf inject because it covers
> a couple of different use cases.  I believe the default behavior is to
> not invoke the callback for deferred callchains during perf inject and
> each sample will get the full callchains.  But you can add a new
> callback and set perf_tool.merge_deferred_callchains to false.

I wonder if there is a different strategy for handling this. Normally
with a visitor pattern you fail when you call an unimplemented
visitor, this is then a signal the (in our case) tool needs to handle
the new case. This avoids naively doing things like making perf inject
duplicate events. The equivalent in the perf code would be to
initialize the callbacks in the tool constructor to be to stubs that
abort, then explicitly initialize and use things like callchain
merging as appropriate. The whole booleans next to the callbacks feels
like a kludge and likely to hide bugs. It is also marginally less
efficient.

Thanks,
Ian