linux-kernel - Re: [PATCH 15/34] rcu: Add noinstr-fast rcu_read_{,un}lock_tasks

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: <d341688c-fa19-4dab-88cb-3a45838cc2f1@paulmck-laptop>
Date: Wed, 24 Sep 2025 01:44:09 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: rcu@...r.kernel.org, linux-kernel@...r.kernel.org, kernel-team@...a.com,
	rostedt@...dmis.org, Andrii Nakryiko <andrii@...nel.org>,
	Alexei Starovoitov <ast@...nel.org>, bpf@...r.kernel.org
Subject: Re: [PATCH 15/34] rcu: Add noinstr-fast
 rcu_read_{,un}lock_tasks_trace() APIs

On Tue, Sep 23, 2025 at 07:32:16PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 23, 2025 at 07:20:17AM -0700, Paul E. McKenney wrote:
> > When expressing RCU Tasks Trace in terms of SRCU-fast, it was
> > necessary to keep a nesting count and per-CPU srcu_ctr structure
> > pointer in the task_struct structure, which is slow to access.
> > But an alternative is to instead make rcu_read_lock_tasks_trace() and
> > rcu_read_unlock_tasks_trace(), which match the underlying SRCU-fast
> > semantics, avoiding the task_struct accesses.
> > 
> > When all callers have switched to the new API, the previous
> > rcu_read_lock_trace() and rcu_read_unlock_trace() APIs will be removed.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@...nel.org>
> > Cc: Andrii Nakryiko <andrii@...nel.org>
> > Cc: Alexei Starovoitov <ast@...nel.org>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: <bpf@...r.kernel.org>
> > ---
> >  include/linux/rcupdate_trace.h | 37 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/include/linux/rcupdate_trace.h b/include/linux/rcupdate_trace.h
> > index 0bd47f12ecd17b..b87151e6b23881 100644
> > --- a/include/linux/rcupdate_trace.h
> > +++ b/include/linux/rcupdate_trace.h
> > @@ -34,6 +34,43 @@ static inline int rcu_read_lock_trace_held(void)
> >  
> >  #ifdef CONFIG_TASKS_TRACE_RCU
> >  
> > +/**
> > + * rcu_read_lock_tasks_trace - mark beginning of RCU-trace read-side critical section
> > + *
> > + * When synchronize_rcu_tasks_trace() is invoked by one task, then that
> > + * task is guaranteed to block until all other tasks exit their read-side
> > + * critical sections.  Similarly, if call_rcu_trace() is invoked on one
> > + * task while other tasks are within RCU read-side critical sections,
> > + * invocation of the corresponding RCU callback is deferred until after
> > + * the all the other tasks exit their critical sections.
> > + *
> > + * For more details, please see the documentation for srcu_read_lock_fast().
> > + */
> > +static inline struct srcu_ctr __percpu *rcu_read_lock_tasks_trace(void)
> > +{
> > +	struct srcu_ctr __percpu *ret = srcu_read_lock_fast(&rcu_tasks_trace_srcu_struct);
> > +
> > +	if (IS_ENABLED(CONFIG_ARCH_WANTS_NO_INSTR))
> > +		smp_mb();
> 
> I am somewhat confused by the relation between noinstr and smp_mb()
> here. Subject mentions is, but Changelog is awfully silent again.

Thank you for looking this over!

To Alexei's point, this commit should be merged with 18/34.

> Furthermore I note that this is a positive while unlock is a negative
> relation between the two. Which adds even more confusion.

You are right, at most one of these two conditions can be correct.  ;-)

I believe that the one above needs a "!".

The point of this is that architectures that set ARCH_WANTS_NO_INSTR
have promised that any point in the entry/exit code that RCU is not
watching has been marked noinstr.  For those architectures, SRCU-fast
can rely on the fact that the key updates in __srcu_read_lock_fast()
and __srcu_read_unlock_fast() are either interrrupt-disabled regions or
atomic operations, depending on the architecture.  This means that
the synchronize_rcu{,_expedited}() calls in the SRCU-fast grace-period
code will be properly ordered with those accesses.

But for !ARCH_WANTS_NO_INSTR architectures, it is possible to attach
various forms of tracing to entry/exit code that RCU is not watching,
which means that those synchronize_rcu{,_expedited}() calls won't have
the needed ordering properties.  So we use smp_mb() on the read side
to force the needed ordering.

Does that help, or am I missing the point of your question?

							Thanx, Paul

> > +	return ret;
> > +}
> > +
> > +/**
> > + * rcu_read_unlock_tasks_trace - mark end of RCU-trace read-side critical section
> > + * @scp: return value from corresponding rcu_read_lock_tasks_trace().
> > + *
> > + * Pairs with the preceding call to rcu_read_lock_tasks_trace() that
> > + * returned the value passed in via scp.
> > + *
> > + * For more details, please see the documentation for rcu_read_unlock().
> > + */
> > +static inline void rcu_read_unlock_tasks_trace(struct srcu_ctr __percpu *scp)
> > +{
> > +	if (!IS_ENABLED(CONFIG_ARCH_WANTS_NO_INSTR))
> > +		smp_mb();
> > +	srcu_read_unlock_fast(&rcu_tasks_trace_srcu_struct, scp);
> > +}
> > +
> >  /**
> >   * rcu_read_lock_trace - mark beginning of RCU-trace read-side critical section
> >   *
> > -- 
> > 2.40.1
> >