Message-ID: <20220804184022.GR2125313@paulmck-ThinkPad-P17-Gen-1>
Date:   Thu, 4 Aug 2022 11:40:22 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Maxim Mikityanskiy <maximmi@...dia.com>
Cc:     "kuba@...nel.org" <kuba@...nel.org>,
        "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 Thu, Aug 04, 2022 at 08:08:37AM +0000, Maxim Mikityanskiy wrote:
> On Wed, 2022-08-03 at 09:34 -0700, Paul E. McKenney wrote:
> > 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.
> 
> That's essentially one of our cases, we check a pointer and queue a
> work if it's not NULL, and the work dereferences it. But we don't use
> rcu_read_lock there, because it's the teardown flow, and there are no
> concurrent users anymore (refcount is 0).

Very good!

> > > 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!
> 
> Thanks a lot for your detailed explanation! It's truly useful,
> especially the examples are helpful.

Would you like to nominate a simple use case for addition to the
official documentation?

> As far as I understood, rcu_access_pointer can be used in any context,
> including RCU updater, as long as we don't dereference the pointer. At
> the same time, it's OK to use rcu_dereference* without dereferencing.
> So, is there any preference, which helper to use, in the context where
> it can't be changed concurrently, if we don't dereference it, but just
> compare the value?

If you are in an RCU reader, there is little reason to use
rcu_access_pointer(), though it might be a good documentation aid.
It can also be helpful in code that is called both from an RCU reader
and from either and updater or an RCU outsider.

> Specifically, we have two (simplified) cases:
> 
> 1. We set the pointer to NULL under the write-lock, but only if it
> matches another pointer.
> 
>  down_write(&lock);
>  ctx_netdev = rcu_dereference_protected(ctx->netdev,
>                                         lockdep_is_held(&lock));
>  if (ctx_netdev == netdev) {
>          rcu_assign_pointer(ctx->netdev, NULL);
>          synchronize_rcu();
>  }
>  up_write(&lock);

I suggest keeping rcu_dereference_protected() here because it documents
the locking.  This might seem silly in this case because you just
acquired that lock, but code can grown functions and then be subject to
copy-pasta-induced bugs, and that call to rcu_dereference_protected()
can help locate such bugs.  In contrast, rcu_assign_pointer() will
cheerfully aid and abet the creation of this sort of bug.

> 2. ctx->refcount goes down to 0, no one can access ctx->netdev anymore,
> we tear down ctx and need to check whether ctx->netdev is NULL.
> 
>  if (!refcount_dec_and_test(&ctx->refcount))
>          return;
>  netdev = rcu_dereference_protected(ctx->netdev,
>                                     !refcount_read(&ctx->refcount));
>  if (netdev)
>          queue_work(...);
> 
> It's somewhat similar to the "structure is beyond being updated" case,
> but it's ensured by refcount, not by RCU (i.e. you example assigned
> my_rcu_pointer = NULL and called synchronize_rcu() to ensure no one
> touches it, and I ensure that we are the only user of ctx by dropping
> refcount to zero).

If that refcount_dec_and_test() is in an RCU callback or if RCU
readers are otherwise guaranteed to no longer be accessing this
object, then rcu_access_pointer() would work.  But again, that
rcu_dereference_protected() has the advantage of protecting against
copy-pasta bugs.

Otherwise, I would need to better understand the example.

> So, hoping that my understanding of your explanation is correct, both
> cases can use any of rcu_access_pointer or rcu_dereference_protected.
> Is there some rule of thumb which one to pick in such case?

If you have something meaningful to put into the lockdep condition of
rcu_dereference_protected(), you should use rcu_dereference_protected().
If not, and if either:

a.	There are no concurrent updates possible, or

b.	There will be no dereferencing of the returned pointer,

then rcu_access_pointer() can be useful.

Also, rcu_access_pointer() can sometimes simplify common code.

> Thanks,
> Max
> 
> > 
> > And please let me know if it does not.
> > 
> > 							Thanx, Paul
> 

Powered by blists - more mailing lists