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

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [Medium] Data race and lost increments on global XDP statistics
- [Medium] Missing statistics increment on frame conversion failure
- [Low] Integer overflow of fast-path software statistics
--

commit 7b7f282ba7f7296a985c1fcb93f530101e6a4b26
Author: Nicolai Buchwitz <nb@tipi-net.de>

net: bcmgenet: add XDP statistics counters

This commit adds XDP statistics counters to the bcmgenet driver. It exposes
xdp_pass, xdp_drop, xdp_tx, xdp_tx_err, xdp_redirect, and xdp_redirect_err
via ethtool statistics.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2428,6 +2435,7 @@ static unsigned int bcmgenet_run_xdp(struct bcmgenet_rx_ring *ring,
>  
>  	switch (act) {
>  	case XDP_PASS:
> +		priv->mib.xdp_pass++;

Is it possible for multiple RX queues to execute this concurrently?

Since priv->mib is global to the device, it seems concurrent NAPI contexts
executing on different CPUs could cause data races and lost increments. Should
these counters be tracked per-queue and aggregated, similar to the existing
tx_rings and rx_rings stats64 counters?

>  		return XDP_PASS;
>  	case XDP_TX:
>  		/* Prepend a zeroed TSB (Transmit Status Block).  The GENET

[ ... ]

>  		xdp->data -= sizeof(struct status_64);
>  		xdp->data_meta -= sizeof(struct status_64);
>  		memset(xdp->data, 0, sizeof(struct status_64));
>  
>  		xdpf = xdp_convert_buff_to_frame(xdp);
>  		if (unlikely(!xdpf)) {
>  			page_pool_put_full_page(ring->page_pool, rx_page,
>  						true);

Does this code miss a stat increment when xdp_convert_buff_to_frame fails?

It returns XDP_DROP here without incrementing xdp_tx_err or xdp_drop, unlike
the other failure paths in this same block.

>  			return XDP_DROP;
>  		}

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h

[ ... ]

> @@ -156,6 +156,12 @@ struct bcmgenet_mib_counters {
>  	u32	tx_dma_failed;
>  	u32	tx_realloc_tsb;
>  	u32	tx_realloc_tsb_failed;
> +	u32	xdp_pass;
> +	u32	xdp_drop;
> +	u32	xdp_tx;
> +	u32	xdp_tx_err;
> +	u32	xdp_redirect;
> +	u32	xdp_redirect_err;
>  };

Since these are fast-path software counters for XDP, can they wrap around
quickly on 32-bit types? Would using 64-bit types like u64_stats_t be safer
to avoid silent wrap-arounds for long-term statistics?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416054743.1289191-1-nb@tipi-net.de?part=6