From: Joel Fernandes <joel@joelfernandes.org>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
Michael Jeanson <mjeanson@efficios.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
rostedt <rostedt@goodmis.org>,
Peter Zijlstra <peterz@infradead.org>,
Alexei Starovoitov <ast@kernel.org>, Yonghong Song <yhs@fb.com>,
paulmck <paulmck@kernel.org>, Ingo Molnar <mingo@redhat.com>,
acme <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
bpf <bpf@vger.kernel.org>
Subject: Re: [RFC PATCH 1/6] tracing: introduce sleepable tracepoints
Date: Mon, 2 Nov 2020 13:51:26 -0500 [thread overview]
Message-ID: <20201102185126.GB595952@google.com> (raw)
In-Reply-To: <1631556114.38532.1603805828748.JavaMail.zimbra@efficios.com>
On Tue, Oct 27, 2020 at 09:37:08AM -0400, Mathieu Desnoyers wrote:
>
> ----- On Oct 26, 2020, at 6:43 PM, Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
>
> > On Fri, Oct 23, 2020 at 03:53:47PM -0400, Michael Jeanson wrote:
> >> -#define __DO_TRACE(tp, proto, args, cond, rcuidle) \
> >> +#define __DO_TRACE(tp, proto, args, cond, rcuidle, tp_flags) \
> >> do { \
> >> struct tracepoint_func *it_func_ptr; \
> >> void *it_func; \
> >> void *__data; \
> >> int __maybe_unused __idx = 0; \
> >> + bool maysleep = (tp_flags) & TRACEPOINT_MAYSLEEP; \
> >> \
> >> if (!(cond)) \
> >> return; \
> >> @@ -170,8 +178,13 @@ static inline struct tracepoint
> >> *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> >> /* srcu can't be used from NMI */ \
> >> WARN_ON_ONCE(rcuidle && in_nmi()); \
> >> \
> >> - /* keep srcu and sched-rcu usage consistent */ \
> >> - preempt_disable_notrace(); \
> >> + if (maysleep) { \
> >> + might_sleep(); \
> >
> > The main purpose of the patch set is to access user memory in tracepoints,
> > right?
>
> Yes, exactly.
>
> > In such case I suggest to use stronger might_fault() here.
> > We used might_sleep() in sleepable bpf and it wasn't enough to catch
> > a combination where sleepable hook was invoked while mm->mmap_lock was
> > taken which may cause a deadlock.
>
> Good point! We will do that for the next round.
>
> By the way, we named this "sleepable" tracepoint (with flag TRACEPOINT_MAYSLEEP),
> but we are open to a better name. Would TRACEPOINT_MAYFAULT be more descriptive ?
> (a "faultable" tracepoint sounds weird though)
What about keeping it might_sleep() here and then adding might_fault() in the
probe handler? Since the probe handler knows that it may cause page fault, it
could itself make sure about it.
One more thought: Should we make _all_ tracepoints sleepable, and then move
the preempt_disable() bit to the probe handler as needed? That could simplify
the tracepoint API as well. Steven said before that whoever registers probes
knows what they are doing so I am ok with that.
No strong feelings one way or the other, for either of these though.
thanks,
- Joel
>
> Thanks,
>
> Mathieu
>
> >
> >> + rcu_read_lock_trace(); \
> >> + } else { \
> >> + /* keep srcu and sched-rcu usage consistent */ \
> >> + preempt_disable_notrace(); \
> > > + } \
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
next prev parent reply other threads:[~2020-11-02 18:51 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-23 19:53 [RFC PATCH 0/6] Sleepable tracepoints Michael Jeanson
2020-10-23 19:53 ` [RFC PATCH 1/6] tracing: introduce sleepable tracepoints Michael Jeanson
2020-10-26 22:43 ` Alexei Starovoitov
2020-10-27 13:37 ` Mathieu Desnoyers
2020-10-28 21:23 ` Alexei Starovoitov
2021-02-11 19:36 ` Mathieu Desnoyers
2020-11-02 18:51 ` Joel Fernandes [this message]
2020-10-23 19:53 ` [RFC PATCH 2/6] tracing: ftrace: add support for " Michael Jeanson
2020-10-23 19:53 ` [RFC PATCH 3/6] tracing: bpf-trace: " Michael Jeanson
2020-10-23 19:53 ` [RFC PATCH 4/6] tracing: perf: " Michael Jeanson
2020-10-23 19:53 ` [RFC PATCH 5/6] tracing: convert sys_enter/exit to " Michael Jeanson
2020-10-23 19:53 ` [RFC PATCH 6/6] tracing: use sched-RCU instead of SRCU for rcuidle tracepoints Michael Jeanson
2020-10-23 21:13 ` Joel Fernandes
2020-10-26 8:20 ` Peter Zijlstra
2020-10-26 14:28 ` Mathieu Desnoyers
2020-10-26 20:44 ` Steven Rostedt
2020-10-27 13:57 ` Mathieu Desnoyers
2020-11-02 18:43 ` Joel Fernandes
2020-10-26 12:05 ` [RFC PATCH 0/6] Sleepable tracepoints peter enderborg
2020-10-26 14:59 ` Mathieu Desnoyers
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=20201102185126.GB595952@google.com \
--to=joel@joelfernandes.org \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=alexei.starovoitov@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mathieu.desnoyers@efficios.com \
--cc=mingo@redhat.com \
--cc=mjeanson@efficios.com \
--cc=namhyung@kernel.org \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=yhs@fb.com \
/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