From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Andrii Nakryiko <andrii@kernel.org>,
linux-trace-kernel@vger.kernel.org, rostedt@goodmis.org,
mhiramat@kernel.org, x86@kernel.org, mingo@redhat.com,
tglx@linutronix.de, bpf@vger.kernel.org, rihams@fb.com,
linux-perf-users@vger.kernel.org, Riham Selim <rihams@meta.com>
Subject: Re: [PATCH 2/4] perf,uprobes: fix user stack traces in the presence of pending uretprobes
Date: Wed, 15 May 2024 08:32:30 -0600 [thread overview]
Message-ID: <CAEf4Bzazi7YMz9n0V46BU7xthQjNdQL_zma5vzgCm_7C-_CvmQ@mail.gmail.com> (raw)
In-Reply-To: <20240515093013.GE40213@noisy.programming.kicks-ass.net>
On Wed, May 15, 2024 at 3:30 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, May 08, 2024 at 02:26:03PM -0700, Andrii Nakryiko wrote:
>
> > +static void fixup_uretprobe_trampoline_entries(struct perf_callchain_entry *entry,
> > + int start_entry_idx)
> > +{
> > +#ifdef CONFIG_UPROBES
> > + struct uprobe_task *utask = current->utask;
> > + struct return_instance *ri;
> > + __u64 *cur_ip, *last_ip, tramp_addr;
> > +
> > + if (likely(!utask || !utask->return_instances))
> > + return;
> > +
> > + cur_ip = &entry->ip[start_entry_idx];
> > + last_ip = &entry->ip[entry->nr - 1];
> > + ri = utask->return_instances;
> > + tramp_addr = uprobe_get_trampoline_vaddr();
> > +
> > + /* If there are pending uretprobes for current thread, they are
>
> Comment style fail. Also 'for *the* current thread'.
>
ack, will fix
> > + * recorded in a list inside utask->return_instances; each such
> > + * pending uretprobe replaces traced user function's return address on
> > + * the stack, so when stack trace is captured, instead of seeing
> > + * actual function's return address, we'll have one or many uretprobe
> > + * trampoline addresses in the stack trace, which are not helpful and
> > + * misleading to users.
>
> I would beg to differ, what if the uprobe is causing the performance
> issue?
If uprobe/uretprobe code itself is causing performance issues, you'll
see that in other stack traces, where this code will be actively
running on CPU. I don't think we make anything worse here.
Here we are talking about the case where the uprobe part is done and
it hijacked the return address on the stack, uretprobe is not yet
running (and so not causing any performance issues). The presence of
this "snooping" (pending) uretprobe is irrelevant to the user that is
capturing stack trace. Right now address in [uprobes] VMA section
installed by uretprobe infra code is directly replacing correct and
actual calling function address.
Worst case, one can argue that both [uprobes] and original caller
address should be in the stack trace, but I think it still will be
confusing to users. And also will make implementation less efficient
because now we'll need to insert entries into the array and shift
everything around.
So as I mentioned above, if the concern is seeing uprobe/uretprobe
code using CPU, that doesn't change, we'll see that in the overall set
of captured stack traces (be it custom uprobe handler code or BPF
program).
>
> While I do think it makes sense to fix the unwind in the sense that we
> should be able to continue the unwind, I don't think it makes sense to
> completely hide the presence of uprobes.
Unwind isn't broken in this sense, we do unwind the entire stack trace
(see examples in the later patch). We just don't capture actual
callers if they have uretprobe pending.
>
> > + * So here we go over the pending list of uretprobes, and each
> > + * encountered trampoline address is replaced with actual return
> > + * address.
> > + */
> > + while (ri && cur_ip <= last_ip) {
> > + if (*cur_ip == tramp_addr) {
> > + *cur_ip = ri->orig_ret_vaddr;
> > + ri = ri->next;
> > + }
> > + cur_ip++;
> > + }
> > +#endif
> > +}
next prev parent reply other threads:[~2024-05-15 14:32 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 21:26 [PATCH 0/4] Fix user stack traces captured from uprobes Andrii Nakryiko
2024-05-08 21:26 ` [PATCH 1/4] uprobes: rename get_trampoline_vaddr() and make it global Andrii Nakryiko
2024-05-08 21:26 ` [PATCH 2/4] perf,uprobes: fix user stack traces in the presence of pending uretprobes Andrii Nakryiko
2024-05-15 9:30 ` Peter Zijlstra
2024-05-15 14:32 ` Andrii Nakryiko [this message]
2024-05-20 15:20 ` Jiri Olsa
2024-05-20 23:56 ` Andrii Nakryiko
2024-05-08 21:26 ` [PATCH 3/4] perf,x86: avoid missing caller address in stack traces captured in uprobe Andrii Nakryiko
2024-05-20 15:20 ` Jiri Olsa
2024-05-08 21:26 ` [PATCH 4/4] selftests/bpf: add test validating uprobe/uretprobe stack traces Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEf4Bzazi7YMz9n0V46BU7xthQjNdQL_zma5vzgCm_7C-_CvmQ@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=rihams@fb.com \
--cc=rihams@meta.com \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).