Date:   Thu, 4 Aug 2022 08:08:37 +0000
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     "paulmck@...nel.org" <paulmck@...nel.org>,
        "kuba@...nel.org" <kuba@...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>,
        "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, 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).

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

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?

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);

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

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?

Thanks,
Max

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

Powered by blists - more mailing lists