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

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 10/02, Björn Töpel wrote:
> 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;

[..]

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

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?