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