Re: [PATCH net-next v8 6/7] net: bcmgenet: add XDP statistics counters

[Jakub Kicinski <kuba@kernel.org>]

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 statistics counters

This commit adds six per-action XDP counters (xdp_pass, xdp_drop, xdp_tx,
xdp_tx_err, xdp_redirect, xdp_redirect_err) to bcmgenet via the existing
soft MIB infrastructure, exposing them through ethtool -S and
incrementing them from bcmgenet_run_xdp().

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 9dd258567824..02ad2f410d6c 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -1169,6 +1169,13 @@ static const struct bcmgenet_stats bcmgenet_gstrings_stats[] = {
>  	STAT_GENET_SOFT_MIB("tx_realloc_tsb", mib.tx_realloc_tsb),
>  	STAT_GENET_SOFT_MIB("tx_realloc_tsb_failed",
>  			    mib.tx_realloc_tsb_failed),
> +	/* XDP counters */
> +	STAT_GENET_SOFT_MIB("xdp_pass", mib.xdp_pass),
> +	STAT_GENET_SOFT_MIB("xdp_drop", mib.xdp_drop),
> +	STAT_GENET_SOFT_MIB("xdp_tx", mib.xdp_tx),
> +	STAT_GENET_SOFT_MIB("xdp_tx_err", mib.xdp_tx_err),
> +	STAT_GENET_SOFT_MIB("xdp_redirect", mib.xdp_redirect),
> +	STAT_GENET_SOFT_MIB("xdp_redirect_err", mib.xdp_redirect_err),

Should these counters be exposed through the standard netdev netlink
qstats interface rather than as driver-private ethtool -S strings?

Per-action XDP statistics are a generic, cross-driver category that
already has a standard uAPI: the netdev netlink family exposes per-device
and per-queue XDP counters (rx-xdp-packets, rx-xdp-bytes, rx-xdp-errors,
tx-xdp-xmit-*, etc.) via the qstats ops, retrievable with ynl.

Documentation/networking/statistics.rst reserves ethtool -S for
driver-private counters, and the networking subsystem guidance is that
new counters should be plumbed through the standard interface rather
than added as private strings, even when the driver does not currently
implement that interface.

Jakub: no, this is wrong, there are no standard counters for XDP today

>  	/* Per TX queues */
>  	STAT_GENET_Q(0),
>  	STAT_GENET_Q(1),
> @@ -2428,6 +2435,7 @@ static unsigned int bcmgenet_run_xdp(struct bcmgenet_rx_ring *ring,
>  
>  	switch (act) {
>  	case XDP_PASS:
> +		priv->mib.xdp_pass++;
>  		return XDP_PASS;
>  	case XDP_TX:
>  		/* Prepend a zeroed TSB (Transmit Status Block).  The GENET
> @@ -2440,6 +2448,7 @@ static unsigned int bcmgenet_run_xdp(struct bcmgenet_rx_ring *ring,
>  		    sizeof(struct status_64) + sizeof(struct xdp_frame)) {
>  			page_pool_put_full_page(ring->page_pool, rx_page,
>  						true);
> +			priv->mib.xdp_tx_err++;
>  			return XDP_DROP;
>  		}
>  		xdp->data -= sizeof(struct status_64);
> @@ -2459,19 +2468,24 @@ static unsigned int bcmgenet_run_xdp(struct bcmgenet_rx_ring *ring,
>  						      xdpf, false))) {
>  			spin_unlock(&tx_ring->lock);
>  			xdp_return_frame_rx_napi(xdpf);
> +			priv->mib.xdp_tx_err++;
>  			return XDP_DROP;
>  		}
>  		bcmgenet_xdp_ring_doorbell(priv, tx_ring);
>  		spin_unlock(&tx_ring->lock);
> +		priv->mib.xdp_tx++;
>  		return XDP_TX;

Is the XDP_TX path missing an increment on one of its failure exits?
The headroom check and the bcmgenet_xdp_xmit_frame() failure both bump
xdp_tx_err, but the xdp_convert_buff_to_frame() failure in between:

	xdpf = xdp_convert_buff_to_frame(xdp);
	if (unlikely(!xdpf)) {
		page_pool_put_full_page(ring->page_pool, rx_page,
					true);
		return XDP_DROP;
	}

returns XDP_DROP without touching any counter.

With this path in place, does the identity

	xdp_pass + xdp_drop + xdp_tx + xdp_tx_err +
	xdp_redirect + xdp_redirect_err == packets_processed

still hold?

>  	case XDP_REDIRECT:
>  		if (unlikely(xdp_do_redirect(priv->dev, xdp, prog))) {
> +			priv->mib.xdp_redirect_err++;
>  			page_pool_put_full_page(ring->page_pool, rx_page,
>  						true);
>  			return XDP_DROP;
>  		}
> +		priv->mib.xdp_redirect++;
>  		return XDP_REDIRECT;
>  	case XDP_DROP:
> +		priv->mib.xdp_drop++;
>  		page_pool_put_full_page(ring->page_pool, rx_page, true);
>  		return XDP_DROP;
>  	default:
> @@ -2479,6 +2493,7 @@ static unsigned int bcmgenet_run_xdp(struct bcmgenet_rx_ring *ring,
>  		fallthrough;
>  	case XDP_ABORTED:
>  		trace_xdp_exception(priv->dev, prog, act);
> +		priv->mib.xdp_drop++;
>  		page_pool_put_full_page(ring->page_pool, rx_page, true);
>  		return XDP_ABORTED;
>  	}

Does folding XDP_ABORTED and the unsupported-action default case into
xdp_drop match the commit message's description of "per-action XDP
counters"?

The function returns XDP_ABORTED (not XDP_DROP) in those cases, and the
existing trace_xdp_exception() call distinguishes aborts and unknown
actions from intentional XDP_DROP.

With the current mapping, operators cannot tell an explicit XDP_DROP
from a program abort or an invalid action return when looking at
xdp_drop.  Would a separate xdp_aborted (or xdp_errors) counter for the
XDP_ABORTED and default cases be more appropriate?