Re: [PATCH net-next v7 03/13] net: mana: keep per-queue statistics in the port context

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] mana_get_stats64()
  (drivers/net/ethernet/microsoft/mana/mana_en.c:608) bounds its RX/TX…
- [Low] Commit message / metadata concern for commit "net: mana: keep
  per-queue statistics in the port context".

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but mana_xdp_xmit()
  (drivers/net/ethernet/microsoft/mana/mana_bpf.c:74) updates…

--

net: mana: keep per-queue statistics in the port context

This commit moves the per-queue RX/TX statistics out of the mana_rxq and
mana_txq objects and into port-lifetime arrays sized by max_queues. The
arrays are allocated at probe and freed after the netdev is unregistered.
ndo_get_stats64 and the ethtool stats now read from these arrays and keep
reporting while the port is down.

> Prepare port-lifetime RX/TX statistics before introducing live queue
> replacement. Allocate max_queues slots before netdev registration,

[ ... ]

> Make ndo_get_stats64 and ethtool statistics independent of replaceable
> queue objects. Preserve counters while down, gating only the PHY
> query.

[Severity: Low]
The message presents this as preparation for live queue replacement. Does
it also change behaviour that users can already see today?

Before this patch the RX/TX counters lived in mana_rxq and mana_txq. Those
are freed and reallocated on every mana_detach()/mana_attach(): ifdown/ifup,
MTU change, ethtool -L/-G, XDP install/remove, TX timeout reset and
suspend/resume. So the counters were reset each time. mana_get_stats64()
also returned early when !port_is_up.

After this patch the counters persist across all of those paths, and
mana_get_stats64() sums over max_queues instead of num_queues.

"Preserve counters while down" does hint at this. Could the message state
the change for the existing paths more explicitly?

Documentation/networking/statistics.rst asks for statistics to persist
across down/up. Would it make sense to split that part out as a separate
fix with a Fixes: tag?

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_bpf.c b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> index ec708f3401a05..2008c17416543 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> @@ -68,7 +68,7 @@ int mana_xdp_xmit(struct net_device *ndev, int n, struct xdp_frame **frames,
>  		count++;
>  	}
>  
> -	tx_stats = &apc->tx_qp[q_idx]->txq.stats;
> +	tx_stats = apc->tx_qp[q_idx]->txq.stats;
>  
>  	u64_stats_update_begin(&tx_stats->syncp);
>  	tx_stats->xdp_xmit += count;

[Severity: Low]
This isn't a bug introduced by this patch, but can two CPUs update the
same xdp_xmit counter at the same time here?

mana_xdp_xmit() picks the slot with:

	q_idx = smp_processor_id() % ndev->real_num_tx_queues;

mana_xdp_tx() holds the TX queue lock only around mana_start_xmit():

	__netif_tx_lock(ndevtxq, smp_processor_id());
	rc = mana_start_xmit(skb, ndev);
	__netif_tx_unlock(ndevtxq);

So the xdp_xmit update after the loop runs without that lock.

Devmap bulk queues are per-CPU (bq_enqueue() in kernel/bpf/devmap.c). With
more CPUs than TX queues, two CPUs can flush to the same q_idx and run the
+= on the same counter concurrently. Doesn't that break the u64_stats rule
that writers of a given syncp must be mutually exclusive?

MANA depends on X86_64 || ARM64, so the seqcount compiles away and the only
effect is lost xdp_xmit counts. The baseline code has the same pattern,
and it is still there at the end of the series.

[ ... ]

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 4b2c8039b8fa3..744d5ebe8dbbb 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -597,15 +597,15 @@ static void mana_get_stats64(struct net_device *ndev,
>  			     struct rtnl_link_stats64 *st)
>  {
>  	struct mana_port_context *apc = netdev_priv(ndev);
> -	unsigned int num_queues = apc->num_queues;
>  	struct mana_stats_rx *rx_stats;
>  	struct mana_stats_tx *tx_stats;
> +	unsigned int num_queues;
>  	unsigned int start;
>  	u64 packets, bytes;
>  	int q;
>  
> -	if (!apc->port_is_up)
> -		return;
> +	/* Report even while down; dev_get_stats() zeroes its output. */
> +	num_queues = apc->max_queues;

[Severity: Medium]
Is apc->max_queues stable enough to use as the bound here?

mana_alloc_queue_stats() sizes rxq_stats, rxq_stats_ret and txq_stats from
apc->max_queues at probe time, and that capacity is not stored anywhere
else.

mana_init_port() runs again on every mana_attach(), including the resume
path in mana_probe(), and it can lower max_queues:

	max_queues = min_t(u32, max_txq, max_rxq);
	if (apc->max_queues > max_queues)
		apc->max_queues = max_queues;
	if (apc->max_queues > gc->max_num_queues_vport)
		apc->max_queues = gc->max_num_queues_vport;

On resume, mana_gd_query_max_resources() recomputes the limits from:

	gc->max_num_queues = num_online_cpus();

So fewer online CPUs, or a smaller vPort max_txq/max_rxq, can shrink
max_queues, for example from 16 to 8. Slots 8..15 still hold accumulated
counts, but this loop stops summing them. The rtnl_link_stats64 RX/TX
totals would then go backwards, against the persistence rule in
Documentation/networking/statistics.rst.

Indexing stays in bounds because max_queues only shrinks after probe.

Would it be better to record the allocated slot count separately, or to
fold the excluded slots into a lifetime aggregate?

There is a related effect. mana_get_ethtool_stats(), along with the
sset_count and strings callbacks, still iterates over apc->num_queues.
After ethtool -L reduces the channel count, the ip -s link totals include
slots in [num_queues, max_queues) that ethtool -S never shows. The
per-queue values then no longer add up to the totals. Is that intended?

This code is still the same at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791710735.git.weh%40linux.microsoft.com