RE: [EXT] Re: [PATCH 1/1] net: fec: add initial XDP support

Shenwei Wang
2022-10-27T01:50:57Z

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, October 25, 2022 5:09 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann
> <daniel@iogearbox.net>; Jesper Dangaard Brouer <hawk@kernel.org>; John
> Fastabend <john.fastabend@gmail.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: [EXT] Re: [PATCH 1/1] net: fec: add initial XDP support
>
> Caution: EXT Email
>
> > +#define FEC_ENET_XDP_PASS 0
> > +#define FEC_ENET_XDP_CONSUMED BIT(0)
> > +#define FEC_ENET_XDP_TX BIT(1)
> > +#define FEC_ENET_XDP_REDIR BIT(2)
>
> I don't know XDP, so maybe a silly question. Are these action mutually exclusive?
> Are these really bits, or should it be an enum?
> fec_enet_run_xdp() does not combine them as bits.
>
The bit here is to record the states that may required after completing the XDP processing.
As the current implementation for XDP is not full, the other bit like FEC_ENET_XDP_TX is not
used for now. Generally it will require an extra action if a FEC_ENET_XDP_TX is returned.
Because we are processing a batch of packets together, those bits may get combined. It will
then responds to each bit accordingly.

> > +static int fec_enet_bpf(struct net_device *dev, struct netdev_bpf
> > +*bpf) {
> > + struct fec_enet_private *fep = netdev_priv(dev);
> > + bool is_run = netif_running(dev);
>
> You have the space, so maybe call it is_running.
>
> > + struct bpf_prog *old_prog;
> > + unsigned int dsize;
> > + int i;
> > +
> > + switch (bpf->command) {
> > + case XDP_SETUP_PROG:
> > + if (is_run)
> > + fec_enet_close(dev);
>
> fec_net_close() followed by fec_enet_open() is pretty expensive. The PHY is
> stopped and disconnected, and then connected and started. That will probably
> trigger an auto-neg, which takes around 1.5 seconds before the link is up again.
>
> Maybe you should optimise this. I guess the real issue here is you need to resize
> the RX ring. You need to be careful with that anyway. If the machine is under
> memory pressure, you might not be able to allocate the ring, resulting in a
> broken interface. What is recommended for ethtool --set-ring is that you first
> allocate the new ring, and if that is successful, free the old ring. If the allocation
> fails, you still have the old ring, and you can safely return -ENOMEM and still
> have a working interface.
>
> So i think you can split this patch up into a few parts:
>
> XDP using the default ring size. Your benchmarks show it works, its just not
> optimal. But the resulting smaller patch will be easier to review.
>
> Add support for ethtool set-ring, which will allow you to pick apart the bits of
> fec_net_close() and fec_enet_open() which are needed for changing the rings.
> This might actually need a refactoring patch?
>

That sounds good. Let me think about it.

Thanks,
Shenwei

> And then add support for optimal ring size for XDP.
>
> Andrew