Subject: Re: [net-next 03/11] ixgbe: add support for XDP_TX action

From: Jakub Kicinski <[email-masked]>
Date: Sat, 22 Apr 2017 20:52:25 -0700

On Sat, 22 Apr 2017 20:40:22 -0700, John Fastabend wrote:
> >> @@ -9557,7 +9739,21 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
> >>  			return -EINVAL;
> >>  	}
> >>  
> >> +	if (nr_cpu_ids > MAX_XDP_QUEUES)
> >> +		return -ENOMEM;
> >> +
> >>  	old_prog = xchg(&adapter->xdp_prog, prog);
> >> +
> >> +	/* If transitioning XDP modes reconfigure rings */
> >> +	if (!!prog != !!old_prog) {
> >> +		int err = ixgbe_setup_tc(dev, netdev_get_num_tc(dev));
> >> +
> >> +		if (err) {
> >> +			rcu_assign_pointer(adapter->xdp_prog, old_prog);
> >> +			return -EINVAL;
> >> +		}
> >> +	}
> >> +
> >>  	for (i = 0; i < adapter->num_rx_queues; i++)
> >>  		xchg(&adapter->rx_ring[i]->xdp_prog, adapter->xdp_prog);
> >>    
> > 
> > In case of disabling XDP I assume ixgbe_setup_tc() will free the rings
> > before the xdp_prog on the rings is swapped to NULL.  Is there anything
> > preventing TX in that time window?  I think usual ordering would be to
> > install the prog after reconfig but uninstall before.
> >   
> 
> Well in the ixgbe_setup_tc() case we set the rx_ring->xdp_prog in
> ixgbe_setup_rx_resorources(), while the dma engine is disabled, so the for
> loop is just doing another set on the rx_ring assigning it to the program
> already set previously.
> 
> Its not really buggy its just extra useless work so I'll change it to this,
> 
> 	if (!!prog != !!old_prog) {
> 		...
> 	} else {
> 		for ( ... )
> 			swap xdp prog
> 	}
> 
> Nice spot, thanks for reviewing. And I missed a build error so I'll roll these
> fixes in a resend.

Ah, thanks for explaining.  No bugs that I can spot then :)