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

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Tuesday, October 25, 2022 5:09 PM
> To: Shenwei Wang <shenwei.wang@xxxxxxx>
> Cc: David S. Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet
> <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni
> <pabeni@xxxxxxxxxx>; Alexei Starovoitov <ast@xxxxxxxxxx>; Daniel Borkmann
> <daniel@xxxxxxxxxxxxx>; Jesper Dangaard Brouer <hawk@xxxxxxxxxx>; John
> Fastabend <john.fastabend@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx
> 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