Re: [RFC net-next 13/15] eth: fbnic: Support AF_XDP zero-copy receive

Stanislav Fomichev [off-list ref] writes:

On 10/02, Björn Töpel wrote:

quoted

fbnic gets RX buffers from page_pool. Use the XSK buffer pool as the
RX queue's page-pool memory provider, so allocation, DMA sync,
recycling and refill stay in page_pool. Only aligned 4 KiB UMEM
chunks are supported.

Each UMEM frame on the header queue must hold one packet. Raise the
header-split threshold to at least 3072 bytes, more than half of a
4 KiB page, so the device finishes the frame after one header. Cap
it to the space left in the frame after the headroom. An MTU above
the threshold needs a socket with XDP_USE_SG and an XDP program with
frags support. With XDP_USE_SG, the payload queue also uses UMEM
frames, and the device uses each frame for one descriptor only.
Without it, the payload queue uses a normal page pool. If the device
does not finish a provider frame after one descriptor, drop the
packet.

The BDQ refill loop now alternates header and payload buffers on all
queues, so two rings that share one finite provider stay balanced.
It keeps NAPI scheduled while the provider still has buffers.

Provider frames are never split, so they get no fragment bias. A
redirected frame can be recycled before its old BDQ slot is cleaned,
and a stale bias would corrupt the new allocation.

The headroom comes from the queue configuration. It must be at least
XDP_PACKET_HEADROOM, a multiple of 128 bytes, and fit the hardware
field; otherwise queue validation fails. MTU, ring parameter and XDP
program changes that do not fit an active provider are rejected.

fbnic did not support XDP_REDIRECT. Add it, with xdp_do_flush(), and
advertise it. This also enables copy-mode AF_XDP and the other
redirect targets on fbnic. Do not advertise AF_XDP zero copy until
TX support is added.

Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |   5 +
 .../net/ethernet/meta/fbnic/fbnic_netdev.c    | 140 ++++++-
 .../net/ethernet/meta/fbnic/fbnic_netdev.h    |   4 +
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c  | 350 +++++++++++++-----
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.h  |   6 +
 5 files changed, 402 insertions(+), 103 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index e774740fcb5d..188abe0ead14 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c

@@ -385,6 +385,11 @@ fbnic_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
 		return -EINVAL;
 	}
 
+	err = fbnic_xsk_validate(fbn, fbn->xdp_prog, netdev->mtu,
+				 kernel_ring->hds_thresh, extack);
+	if (err)
+		return err;
+
 	if (!netif_running(netdev)) {
 		fbnic_set_rings(fbn, ring, kernel_ring);
 		return 0;

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
index 8a6703afca38..038e91ef14e4 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c

@@ -5,6 +5,8 @@
 #include <linux/ipv6.h>
 #include <linux/types.h>
 #include <net/netdev_queues.h>
+#include <net/netdev_rx_queue.h>
+#include <net/xdp_sock_drv.h>
 
 #include "fbnic.h"
 #include "fbnic_netdev.h"

@@ -276,6 +278,8 @@ static int fbnic_set_mac(struct net_device *netdev, void *p)
 static int fbnic_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct fbnic_net *fbn = netdev_priv(dev);
+	struct netlink_ext_ack extack = {};
+	int err;
 
 	if (fbnic_check_split_frames(fbn->xdp_prog, new_mtu, fbn->hds_thresh)) {
 		dev_err(&dev->dev,

@@ -285,6 +289,15 @@ static int fbnic_change_mtu(struct net_device *dev, int new_mtu)
 		return -EINVAL;
 	}
 
+	err = fbnic_xsk_validate(fbn, fbn->xdp_prog, new_mtu,
+				 fbn->hds_thresh, &extack);
+	if (err) {
+		if (extack._msg)
+			netdev_err(dev, "%s\n", extack._msg);
+
+		return err;
+	}
+
 	WRITE_ONCE(dev->mtu, new_mtu);
 
 	return 0;

@@ -534,26 +547,123 @@ bool fbnic_check_split_frames(struct bpf_prog *prog, unsigned int mtu,
 	return mtu + ETH_HLEN > hds_thresh;
 }
 
+u32 fbnic_xsk_hds_thresh(u32 headroom, u32 hds_thresh)
+{
+	u32 max_hds;
+
+	/* Reserve more than half a device page for each header so the device
+	 * retires the provider frame after one descriptor.
+	 */
+	static_assert(2 * FBNIC_XSK_HDS_THRESH > FBNIC_BD_PAGE_SIZE);
+
+	max_hds = FBNIC_BD_PAGE_SIZE - headroom - FBNIC_RX_TROOM -
+		  FBNIC_RX_PAD;
+
+	return min(max_t(u32, hds_thresh, FBNIC_XSK_HDS_THRESH), max_hds);
+}
+
+static int fbnic_xsk_validate_frames(struct xsk_buff_pool *pool,
+				     struct bpf_prog *prog,
+				     unsigned int mtu, u32 hds_thresh,
+				     struct netlink_ext_ack *extack)
+{
+	u32 xsk_hds_thresh;
+
+	xsk_hds_thresh = fbnic_xsk_hds_thresh(xsk_pool_get_headroom(pool),
+					      hds_thresh);
+
+	if (mtu + ETH_HLEN <= xsk_hds_thresh)
+		return 0;
+
+	if (!xsk_pool_uses_sg(pool)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "AF_XDP zero-copy socket requires XDP_USE_SG");
+		return -EOPNOTSUPP;
+	}
+
+	if (fbnic_check_split_frames(prog, mtu, xsk_hds_thresh)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "AF_XDP zero-copy socket requires XDP frags");
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+int fbnic_xsk_validate(struct fbnic_net *fbn, struct bpf_prog *prog,
+		       unsigned int mtu, u32 hds_thresh,
+		       struct netlink_ext_ack *extack)
+{
+	struct net_device *netdev = fbn->netdev;
+	struct xsk_buff_pool *pool;
+	unsigned int qid;
+	int err;
+
+	for (qid = 0; qid < fbn->num_rx_queues; qid++) {
+		pool = xsk_get_pool_from_rxq(netdev, qid);
+		if (!pool)
+			continue;
+
+		err = fbnic_xsk_validate_frames(pool, prog, mtu, hds_thresh,
+						extack);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int fbnic_xsk_setup_pool(struct net_device *netdev,
+				struct xsk_buff_pool *pool, u16 queue_id,
+				struct netlink_ext_ack *extack)
+{
+	struct fbnic_net *fbn = netdev_priv(netdev);
+	int err;
+
+	if (queue_id >= fbn->num_rx_queues)
+		return -EINVAL;
+
+	if (!pool)
+		return xsk_pool_setup_page_pool(netdev, NULL, queue_id, extack);
+
+	err = fbnic_xsk_validate_frames(pool, fbn->xdp_prog, netdev->mtu,
+					fbn->hds_thresh, extack);
+	if (err)
+		return err;
+
+	return xsk_pool_setup_page_pool(netdev, pool, queue_id, extack);
+}
+
 static int fbnic_bpf(struct net_device *netdev, struct netdev_bpf *bpf)
 {
 	struct bpf_prog *prog = bpf->prog, *prev_prog;
 	struct fbnic_net *fbn = netdev_priv(netdev);
+	int err;
 
-	if (bpf->command != XDP_SETUP_PROG)
+	switch (bpf->command) {
+	case XDP_SETUP_PROG:
+		if (fbnic_check_split_frames(prog, netdev->mtu,
+					     fbn->hds_thresh)) {
+			NL_SET_ERR_MSG_MOD(bpf->extack,
+					   "MTU too high, or HDS threshold is too low for single buffer XDP");
+			return -EOPNOTSUPP;
+		}
+
+		err = fbnic_xsk_validate(fbn, prog, netdev->mtu,
+					 fbn->hds_thresh, bpf->extack);
+		if (err)
+			return err;
+
+		prev_prog = xchg(&fbn->xdp_prog, prog);
+		if (prev_prog)
+			bpf_prog_put(prev_prog);
+		return 0;

[..]

quoted

+	case XDP_SETUP_XSK_POOL:
+		return fbnic_xsk_setup_pool(netdev, bpf->xsk.pool,
+					    bpf->xsk.queue_id, bpf->extack);

Is there a good reason to ask drivers to call us back with
xsk_pool_setup_page_pool? For the PP mode, maybe we should add new
XSK_VALIDATE_POOL or something? So most of that fbnic_xsk_setup_pool stays
in core?

Makes sense. I'll try core-owned setup with driver opt-in and
validation. Existing direct drivers can keep their setup.

I guess I'm mentally trying to have a clear separation between
pp vs non-pp modes. Even that whole xsk_buff_pool might be legacy non-pp
artifact? New drivers shouldn't bother with it? Or is it too much?

Hmm. New RX code shouldn't need allocator internals, but rings, DMA and
TX state are still shared. I need to noodle a bit on it...