Message-ID: <20220803163437.GF2125313@paulmck-ThinkPad-P17-Gen-1>
Date:   Wed, 3 Aug 2022 09:34:37 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Maxim Mikityanskiy <maximmi@...dia.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Tariq Toukan <tariqt@...dia.com>,
        Gal Pressman <gal@...dia.com>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Boris Pismenny <borisp@...dia.com>
Subject: Re: [PATCH net-next] net/tls: Use RCU API to access tls_ctx->netdev

On Wed, Aug 03, 2022 at 07:49:57AM -0700, Jakub Kicinski wrote:
> On Wed, 3 Aug 2022 09:33:48 +0000 Maxim Mikityanskiy wrote:
> > > > The documentation of rcu_access_pointer says it shouldn't be used on
> > > > the update side, because we lose lockdep protection:
> > > > 
> > > > --cut--
> > > > 
> > > > Although rcu_access_pointer() may also be used in cases
> > > > where update-side locks prevent the value of the pointer from changing,
> > > > you should instead use rcu_dereference_protected() for this use case.  
> > > 
> > > I think what this is trying to say is to not use the
> > > rcu_access_pointer() as a hack against lockdep:  
> > 
> > Well, maybe we understand it in different ways. This is how I parsed it
> > (the whole comment):
> > 
> > 1. rcu_access_pointer is not for the read side. So, it's either for the
> > write side or for usage outside all locks.

RCU readers really are permitted to use rcu_access_pointer().  As is
pretty much any other code.

See for example Documentation/RCU/rcu_dereference.rst:

	Note that if checks for being within an RCU read-side critical
	section are not required and the pointer is never dereferenced,
	rcu_access_pointer() should be used in place of rcu_dereference().

OK, s/should be/can be/, but I will fix that.

Or, for that matter, the rcu_access_pointer() docbook header comment:

/**
 * rcu_access_pointer() - fetch RCU pointer with no dereferencing
 * @p: The pointer to read
 *
 * Return the value of the specified RCU-protected pointer, but omit the
 * lockdep checks for being in an RCU read-side critical section.  This is
 * useful when the value of this pointer is accessed, but the pointer is
 * not dereferenced, for example, when testing an RCU-protected pointer
 * against NULL.  Although rcu_access_pointer() may also be used in cases
 * where update-side locks prevent the value of the pointer from changing,
 * you should instead use rcu_dereference_protected() for this use case.
 *
 * It is also permissible to use rcu_access_pointer() when read-side
 * access to the pointer was removed at least one grace period ago, as
 * is the case in the context of the RCU callback that is freeing up
 * the data, or after a synchronize_rcu() returns.  This can be useful
 * when tearing down multi-linked structures after a grace period
 * has elapsed.
 */

So the restriction is that the pointer returned from rcu_access_pointer()
cannot be dereferenced or that the structure is beyond being updated.

So this is OK:

	// Not in an RCU reader.  Or even in an RCU updater.
	if (rcu_access_pointer(my_rcu_pointer))
		do_something();
	...

And so is this:

	p = xchg(&my_rcu_pointer, NULL);
	if (p) {
		synchronize_rcu();
		// No one else has access to this list!
		while (p) {
			q = rcu_access_pointer(p->next);
			kfree(p);
			q = p;
			// But why are you hand-crafting list???
			// Any why not use rcu_dereference_protected()?
		}
	}

But this is not:

	p = rcu_access_pointer(my_rcu_pointer);
	do_something_with(p->a); // BUG!!!  Even in an RCU reader.

In this second case, you must instead use rcu_dereference() or
similar.

> > 2. It's not for dereferencing. So, it's for reading the pointer's value
> > on the write side or outside all locks.

True enough, you are not permitted to dereference the value returned
from rcu_access_pointer().  Unless you have the only copy.

But it is just fine to check the value of the pointer, compare it, or
do arithmetic on it.  Just don't dereference it and don't dereference
any value computed from it.

> > 3. Although it can be used on the write side, rcu_dereference_protected
> > should be used. So, it's for reading the pointer's value outside all
> > locks.

Yes, if an RCU updater is going to dereference a pointer, it should
use rcu_dereference_protected() rather than rcu_access_pointer().

So rcu_access_pointer() does what it it says.  It permits the caller
to access the value of the pointer, and only to access that value.
Not dereference that value.

> Using rcu_deref* when we don't dereference the pointer does not compute
> for me, but it's not a big deal. 

It is OK to use rcu_dereference*() to access a pointer without
dereferencing it.

One key difference between rcu_dereference() and rcu_access_pointer()
is that rcu_access_pointer() can be used outside of an RCU reader.
For example:

	// Not in an RCU reader.  Or even in an RCU updater.
	if (rcu_access_pointer(my_rcu_pointer)) {
		rcu_read_lock();
		p = rcu_dereference(my_rcu_pointer);
		if (p)
			do_something_with(p);
		rcu_read_unlock();
	}

This example is silly because the overhead of the extra check might well
exceed that of the rcu_read_lock() and rcu_read_unlock() put together,
especially in CONFIG_PREEMPTION=n kernels.  A less-silly example might
schedule a workqueue or similar to handle the RCU-protected data, and
the overhead of the extra check might be very worthwhile in that case.

> Let me CC Paul for clarification of the docs, as it may also be
> confusing to others and therefore worth rewording. But our case is 
> not that important so unless Paul chimes in clearly indicating one
> interpretation is right - either way is fine by me for v2.

Hope this helps!

And please let me know if it does not.

							Thanx, Paul

Powered by blists - more mailing lists