Re: [bpf-next PATCH v2 2/2] bpf: xdp, remove no longer required rcu_read_{un}lock()

[John Fastabend <john.fastabend@gmail.com>]

Maciej Fijalkowski wrote:
> On Sat, Jan 11, 2020 at 06:37:42PM -0800, John Fastabend wrote:
> > Now that we depend on rcu_call() and synchronize_rcu() to also wait
> > for preempt_disabled region to complete the rcu read critical section
> > in __dev_map_flush() is no longer required. Except in a few special
> > cases in drivers that need it for other reasons.
> > 
> > These originally ensured the map reference was safe while a map was
> > also being free'd. And additionally that bpf program updates via
> > ndo_bpf did not happen while flush updates were in flight. But flush
> > by new rules can only be called from preempt-disabled NAPI context.
> > The synchronize_rcu from the map free path and the rcu_call from the
> > delete path will ensure the reference there is safe. So lets remove
> > the rcu_read_lock and rcu_read_unlock pair to avoid any confusion
> > around how this is being protected.
> > 
> > If the rcu_read_lock was required it would mean errors in the above
> > logic and the original patch would also be wrong.
> > 
> > Now that we have done above we put the rcu_read_lock in the driver
> > code where it is needed in a driver dependent way. I think this
> > helps readability of the code so we know where and why we are
> > taking read locks. Most drivers will not need rcu_read_locks here
> > and further XDP drivers already have rcu_read_locks in their code
> > paths for reading xdp programs on RX side so this makes it symmetric
> > where we don't have half of rcu critical sections define in driver
> > and the other half in devmap.
> > 
> > Fixes: 0536b85239b84 ("xdp: Simplify devmap cleanup")
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---

[...]

> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 4d7d5434..2c11f82 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -498,12 +498,16 @@ static int virtnet_xdp_xmit(struct net_device *dev,
> >  	void *ptr;
> >  	int i;
> >  
> > +	rcu_read_lock();
> > +
> >  	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
> >  	 * indicate XDP resources have been successfully allocated.
> >  	 */
> >  	xdp_prog = rcu_dereference(rq->xdp_prog);
> 
> We could convert that rcu_dereference to rcu_access_pointer so that we
> don't need the rcu critical section here at all. Actually this was
> suggested some time ago by David Ahern during the initial discussion
> around this code. Not sure why we didn't change it.

Makes sense I'll send a v3 with a middle patch to do this and then drop
this segment.

> 
> Veth is also checking the xdp prog presence and it is doing that via
> rcu_access_pointer so such conversion would make it more common, no?

veth derefernces rcv netdevice and this accesses it. The logic to
drop the rcu here is less obvious to me. At least I would have to
study it closely.

> 
> xdp_prog is only check against NULL, so quoting the part of comment from
> rcu_access_pointer:
> "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."

+1 thanks it does make the cleanup nicer.