Re: [RFC PATCH net-next v17] vmxnet3: Add XDP support.

[William Tu <u9012063@gmail.com>]

On Thu, Feb 23, 2023 at 7:24 AM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: William Tu <u9012063@gmail.com>
> Date: Mon, 20 Feb 2023 20:35:47 -0800
>
> > The patch adds native-mode XDP support: XDP DROP, PASS, TX, and REDIRECT.
>
> [...]
>
> > +static int
> > +vmxnet3_xdp_set(struct net_device *netdev, struct netdev_bpf *bpf,
> > +             struct netlink_ext_ack *extack)
> > +{
> > +     struct vmxnet3_adapter *adapter = netdev_priv(netdev);
> > +     struct bpf_prog *new_bpf_prog = bpf->prog;
> > +     struct bpf_prog *old_bpf_prog;
> > +     bool need_update;
> > +     bool running;
> > +     int err;
> > +
> > +     if (new_bpf_prog && netdev->mtu > VMXNET3_XDP_MAX_MTU) {
> > +             NL_SET_ERR_MSG_MOD(extack, "MTU too large for XDP");
>
> Minor: we now have NL_SET_ERR_MSG_FMT_MOD(), so you could even print to
> user what the maximum MTU you support for XDP is.
good idea, will use it.

>
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     if (adapter->netdev->features & NETIF_F_LRO) {
> > +             NL_SET_ERR_MSG_MOD(extack, "LRO is not supported with XDP");
> > +             adapter->netdev->features &= ~NETIF_F_LRO;
> > +     }
> > +
> > +     old_bpf_prog = rcu_dereference(adapter->xdp_bpf_prog);
> > +     if (!new_bpf_prog && !old_bpf_prog)
> > +             return 0;
> > +
> > +     running = netif_running(netdev);
> > +     need_update = !!old_bpf_prog != !!new_bpf_prog;
> > +
> > +     if (running && need_update)
> > +             vmxnet3_quiesce_dev(adapter);
>
> [...]
>
> > +             bpf_warn_invalid_xdp_action(rq->adapter->netdev, prog, act);
> > +             fallthrough;
> > +     case XDP_ABORTED:
> > +             trace_xdp_exception(rq->adapter->netdev, prog, act);
> > +             rq->stats.xdp_aborted++;
> > +             break;
> > +     case XDP_DROP:
> > +             rq->stats.xdp_drops++;
> > +             break;
> > +     }
> > +
> > +     page_pool_recycle_direct(rq->page_pool, page);
>
> You can speed up stuff a bit here. recycle_direct() takes ->max_len to
> sync DMA for device when recycling. You can use page_pool_put_page() and
> specify the actual length which needs to be synced. This is a bit
> tricky, but some systems have incredibly expensive DMA synchronization
> and every byte counts there.
> "Tricky" because you can't specify the original frame size here and ATST
> can't specify the current xdp.data_end - xdp.data. As xdp.data may grow
> to both left and right, the same with .data_end. So what you basically
> need is the following:
>
> sync_len = max(orig_len,
>                xdp.data_end - xdp.data_hard_start - page_pool->p.offset)
>
> Because we don't care about the data between data_hard_start and
> p.offset -- hardware doesn't touch it. But we care about the whole area
> that might've been touched to the right of it.
>
> Anyway, up to you. On server x86_64 platforms DMA sync is usually a noop.
Thanks a lot!
Actually at hypersor side we noticed that DMA sync is pretty expensive,
that's why we have dataring as optimization (no DMA sync)
So it might worth doing it and I will try and see if there is any
performance difference!
(but not the next version)

>
> > +
> > +     return act;
> > +}
>
> [...]
>
> > +static inline bool vmxnet3_xdp_enabled(struct vmxnet3_adapter *adapter)
> > +{
> > +     return !!rcu_access_pointer(adapter->xdp_bpf_prog);
> > +}
> > +
> > +#endif
>
> I feel good with the rest of the patch, thanks! Glad to see all the
> feedback addressed when applicable.
>
> Olek

Thanks!
William