Re: [PATCH] net/mlx4_en: protect ring->xdp_prog with rcu_read_lock

[Brenden Blanco <bblanco@plumgrid.com>]

On Fri, Sep 02, 2016 at 01:59:40AM +0300, Saeed Mahameed wrote:
> On Wed, Aug 31, 2016 at 4:50 AM, Brenden Blanco <bblanco@plumgrid.com> wrote:
> > On Tue, Aug 30, 2016 at 12:35:58PM +0300, Saeed Mahameed wrote:
[...]
> >> Sorry folks I am with Tariq on this, you can't just add a single
> >> instruction which is only valid/needed for 1% of the use cases
> >> to the driver's general data path, even if it was as cheap as one cpu cycle!
> > How about 0?
> >
> > $ diff mlx4_en.ko.norcu.s mlx4_en.ko.rcu.s | wc -l
> > 0
> >
> 
> Well, If you put it this way, it seems OK then.
> 
> Anyway I would add a friendly comment beside the rcu_read_lock that
> "this is needed to protect
> access to ring->xdp_prog".

Thanks, I will go ahead with this then.

> 
> >>
> >> Let me try to suggest something:
> >> instead of taking the rcu_read_lock for the whole
> >> mlx4_en_process_rx_cq, we can minimize to XDP code path only
> >> by double checking xdp_prog (non-protected check followed by a
> >> protected check inside mlx4 XDP critical path).
> >>
> >> i.e instead of:
> >>
> >> rcu_read_lock();
> >>
> >> xdp_prog = ring->xdp_prog;
> >>
> >> //__Do lots of non-XDP related stuff__
> >>
> >> if (xdp_prog) {
> >>     //Do XDP magic ..
> >> }
> >> //__Do more of non-XDP related stuff__
> >>
> >> rcu_read_unlock();
> >>
> >>
> >> We can minimize it to XDP critical path only:
> >>
> >> //Non protected xdp_prog dereference.
> >> if (xdp_prog) {
> >>      rcu_read_lock();
> >>      //Protected dereference to ring->xdp_prog
> >>      xdp_prog = ring->xdp_prog;
> >>      if(unlikely(!xdp_prg)) goto unlock;
> >
> > The addition of this branch and extra deref is now slowing down the xdp
> > path compared to the current proposal.
> >
> 
> Yep, but this is an unlikely condition and the critical code here is
> much smaller and it is more clear that the rcu_read_lock here meant to
> protect the ring->xdp_prog under this small xdp critical section in
> comparison to your patch where it is held across the whole RX
> function.

It's really an improper use of RCU though.

RCU is meant to provide correctness without sacrificing any performance
in the fastpath. It is designed to avoid having to double-dereference
and other such tricks, so shouldn't we use it how it was designed?
Having a larger scoped rcu_read_lock doesn't hurt anybody here, but the
extra memory reads certainly _does_ impact the XDP path, which folks are
going to start relying on to be performant. Let's not start chipping
away at that.