Message-ID: <bba8b944c61780278a36e76f79b79d19f20ab9cd.camel@nvidia.com>
Date:   Fri, 5 Aug 2022 10:59:53 +0000
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     "paulmck@...nel.org" <paulmck@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        Tariq Toukan <tariqt@...dia.com>,
        Gal Pressman <gal@...dia.com>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "pabeni@...hat.com" <pabeni@...hat.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, 2022-08-04 at 11:40 -0700, Paul E. McKenney wrote:
> 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?

The example with freeing the list (xchg, synchronize_rcu,
rcu_access_pointer) would be a nice illustration to the last paragraph
of rcu_access_pointer's doc comment. Probably not in the comment
itself, but under Documentation/.

Also the example with rcu_access_pointer + queue_work, which takes
rcu_read_lock and calls rcu_dereference could illustrate the usefulness
of rcu_access_pointer outside all locks.

> 
> > 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.

OK, I'll keep it.

> 
> > 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. 

It's the latter - no one else can access ctx after its refcount dropped
to zero.

>  But again, that
> rcu_dereference_protected() has the advantage of protecting against
> copy-pasta bugs.

OK then, I'll use that advantage.

> 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 again for your advice and providing a detailed explanation! I'll
stick to this rule, and maybe it's a good idea to put it to some
prominent place in the documentation to simplify the choice for other
developers.

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

Powered by blists - more mailing lists