Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 26 Oct 2022 00:08:30 +0200
From:   Andrew Lunn <andrew@lunn.ch>
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: Re: [PATCH 1/1] net: fec: add initial XDP support
Message-ID: <Y1heXnxgA6e5T8sr@lunn.ch>
References: <20221025201156.776576-1-shenwei.wang@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221025201156.776576-1-shenwei.wang@nxp.com>
Precedence: bulk

> +#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.

> +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?

And then add support for optimal ring size for XDP.

    Andrew