Re: [PATCH net-next v9 5/7] net: bcmgenet: add XDP_REDIRECT and ndo_xdp_xmit support

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: bcmgenet: add XDP_REDIRECT and ndo_xdp_xmit support

This commit adds XDP_REDIRECT handling in the RX path and implements
ndo_xdp_xmit so other devices can redirect frames into bcmgenet via
ring 16. It also advertises NETDEV_XDP_ACT_REDIRECT and
NETDEV_XDP_ACT_NDO_XMIT in xdp_features.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index f1e515526787..4e4fe785f0bf 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -4027,6 +4048,36 @@ static int bcmgenet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
>  	}
>  }
>  
> +static int bcmgenet_xdp_xmit(struct net_device *dev, int num_frames,
> +			     struct xdp_frame **frames, u32 flags)
> +{
> +	struct bcmgenet_priv *priv = netdev_priv(dev);
> +	struct bcmgenet_tx_ring *ring = &priv->xdp_tx_ring;
> +	int sent = 0;
> +	int i;
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> +		return -EINVAL;
> +
> +	if (unlikely(!netif_running(dev)))
> +		return -ENETDOWN;
> +
> +	spin_lock(&ring->lock);
> +
> +	for (i = 0; i < num_frames; i++) {
> +		if (!bcmgenet_xdp_xmit_frame(priv, ring, frames[i], true))
> +			break;
> +		sent++;
> +	}
> +
> +	if (sent)
> +		bcmgenet_xdp_ring_doorbell(priv, ring);
> +
> +	spin_unlock(&ring->lock);
> +
> +	return sent;
> +}

Can this path make forward progress when bcmgenet is used purely as an
XDP_REDIRECT destination (for example, redirecting from a USB NIC or
Wi-Fi adapter into the on-SoC bcmgenet)?

The xdp_tx_ring (ring DESC_INDEX) has no dedicated TX interrupt and no
TX NAPI.  bcmgenet_init_tx_ring() explicitly skips the TX NAPI for this
ring:

    /* Initialize Tx NAPI for priority queues only; ring DESC_INDEX
     * (XDP TX) has its completions handled inline in RX NAPI.
     */
    if (index != DESC_INDEX)
        netif_napi_add_tx(priv->dev, &ring->napi, bcmgenet_tx_poll);

The only place that reclaims completed descriptors on this ring is
bcmgenet_rx_poll():

    static int bcmgenet_rx_poll(struct napi_struct *napi, int budget)
    {
        ...
        /* Reclaim completed XDP TX frames (ring 16 has no interrupt) */
        if (priv->xdp_tx_ring.free_bds < priv->xdp_tx_ring.size)
            bcmgenet_tx_reclaim(priv->dev,
                                &priv->xdp_tx_ring, false);
        ...
    }

Before this patch, the only producer on this ring was XDP_TX from
bcmgenet's own RX path, so reclaim and production were always paired
with RX NAPI.

With ndo_xdp_xmit, a remote device becomes a producer on the same
ring.  If bcmgenet itself has no (or very sparse) RX traffic, its RX
NAPI is never scheduled, bcmgenet_tx_reclaim() on xdp_tx_ring is never
called, free_bds drains to 0, and:

> +	for (i = 0; i < num_frames; i++) {
> +		if (!bcmgenet_xdp_xmit_frame(priv, ring, frames[i], true))
> +			break;
> +		sent++;
> +	}

bcmgenet_xdp_xmit_frame() returns false on the free_bds < 1 check and
bcmgenet_xdp_xmit() returns 0, so bq_xmit_all() in the redirect caller
counts every frame as a drop.

There does not appear to be a TX completion IRQ, a watchdog, or a
napi_schedule() kick from bcmgenet_xdp_xmit() to trigger reclaim
independently of RX traffic.  Is there something else that drains the
ring in this configuration, or does ndo_xdp_xmit need to schedule the
RX NAPI (or an independent reclaim path) when it enqueues frames?

> @@ -4341,7 +4393,8 @@ static int bcmgenet_probe(struct platform_device *pdev)
>  			 NETIF_F_RXCSUM;
>  	dev->hw_features |= dev->features;
>  	dev->vlan_features |= dev->features;
> -	dev->xdp_features = NETDEV_XDP_ACT_BASIC;
> +	dev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
> +			    NETDEV_XDP_ACT_NDO_XMIT;

Relatedly, advertising NETDEV_XDP_ACT_NDO_XMIT tells user space and
redirect maps that this device is a usable xmit target.  Should this
bit only be advertised once ring 16 has an independent completion
source, or is the expectation that users of this feature will always
have concurrent RX traffic on bcmgenet to drive the reclaim?
-- 
pw-bot: cr