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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 27 Oct 2022 01:50:54 +0000
From:   Shenwei Wang <shenwei.wang@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "imx@...ts.linux.dev" <imx@...ts.linux.dev>
Subject: RE: [EXT] Re: [PATCH 1/1] net: fec: add initial XDP support



> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Tuesday, October 25, 2022 5:09 PM
> To: Shenwei Wang <shenwei.wang@....com>
> Cc: David S. Miller <davem@...emloft.net>; Eric Dumazet
> <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni
> <pabeni@...hat.com>; Alexei Starovoitov <ast@...nel.org>; Daniel Borkmann
> <daniel@...earbox.net>; Jesper Dangaard Brouer <hawk@...nel.org>; John
> Fastabend <john.fastabend@...il.com>; netdev@...r.kernel.org; linux-
> kernel@...r.kernel.org; imx@...ts.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