Re: [PATCH net-next v3 09/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 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 2

- [Medium] mana_get_stats64() keeps the `if (!apc->port_is_up) return;`
  early return above the new port-lifetime accumulation, so the counters…
- [Low] mana_update_rx_dim() now feeds net_dim() from
  rxq->stats->packets/bytes, which after this patch is…
- [Low] The writer invariant documented for the new statistics arrays is
  contradicted by the code added in the same patch.

--

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

This moves the per-queue RX/TX statistics out of struct mana_rxq and
struct mana_txq into arrays owned by the port context, sized to
max_queues and allocated at probe time.  A retiring RX queue counts into
a private drain_stats which is folded into a per-index "retired" array,
and ndo_get_stats64() adds the live and retired slots for every index.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 5c5d1edd20f84..69e34fab8fe8e 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -639,9 +639,9 @@ 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;
> @@ -649,6 +649,12 @@ static void mana_get_stats64(struct net_device *ndev,
>  	if (!apc->port_is_up)
>  		return;
>  
> +	/* Walk every slot, not just the queues currently open: counters
> +	 * accumulated on queues that a later reconfiguration removed must
> +	 * still be reported, or the interface totals would go backwards.
> +	 */
> +	num_queues = apc->max_queues;
> +

[Severity: Medium]
Does the retained port_is_up gate above this still let rx_bytes be
observed going backwards through the very operation the commit message
describes?

mana_publish_qset() clears the flag on entry and only sets it again at the
very end:

	WRITE_ONCE(apc->port_is_up, false);
	...
	netif_tx_disable(ndev);
	mana_qset_set_retiring(out_old, newq, true);
	synchronize_net();
	...
	netif_set_real_num_tx_queues() / netif_set_real_num_rx_queues()
	mana_chn_setxdp()
	mana_config_rss()
	...
	WRITE_ONCE(apc->port_is_up, true);

so the flag is false across the whole ethtool channel/ring/priv-flag, MTU
and XDP-attach window, and stays false indefinitely on the
mana_publish_give_up() dead ends.

Readers are not serialized against it.  netstat_show() (sysfs
statistics/rx_bytes) and dev_seq_show() (/proc/net/dev) reach
dev_get_stats() under rcu_read_lock() only, never RTNL, and dev_get_stats()
zeroes the storage before calling the driver:

net/core/dev.c:dev_get_stats() {
	...
	if (ops->ndo_get_stats64) {
		memset(storage, 0, sizeof(*storage));
		ops->ndo_get_stats64(dev, storage);
	...
}

Returning early therefore publishes zeros rather than the previous values,
so a reader polling during "ethtool -L" would still see
4475831638 -> 0 -> N.

Since the arrays are now port-owned and valid whether or not a queue
exists at that index, could they be reported unconditionally?
mana_get_ethtool_stats() carries the same "if (!apc->port_is_up) return;"
gate, which also makes the newly persistent per-queue counters unreadable
while the port is administratively down.

[ ... ]

> @@ -1092,6 +1112,122 @@ static void mana_cleanup_port_context(struct mana_port_context *apc)
>  	apc->rxqs = NULL;
>  }
>  
> +/* Counters belong to the port, not the queues, so a queue-set replacement
> + * does not reset them. Sized to max_queues, allocated once.
> + *
> + * A swap adds no writer to a TX slot. RX slots do overlap briefly, since a
> + * retiring rxq keeps its NAPI until mana_free_qset() destroys it. MANA is
> + * 64-bit only, so u64_stats_sync has no seqcount and at worst a few
> + * increments are lost; the alternatives are a lock in the receive path or
> + * per-set slots that make ndo_get_stats64() dip during a swap.
> + */
> +static int mana_alloc_queue_stats(struct mana_port_context *apc)

[Severity: Low]
This isn't a bug, but does this comment contradict the one added to struct
mana_port_context in the same patch?

Here the RX live slots are described as having overlapping writers:

	"RX slots do overlap briefly, since a retiring rxq keeps its NAPI
	 until mana_free_qset() destroys it ... at worst a few increments
	 are lost"

while the header says:

	"rxq_stats[] is written by the live RX queue at that index and
	 rxq_stats_ret[] only under RTNL, by mana_destroy_rxq() folding in
	 what a retiring queue counted while it drained. One writer each;
	 readers add the two."

The header is the text a later auditor is likely to consult when deciding
whether these bare u64_stats_sync updates need a lock.

[ ... ]

> @@ -2672,8 +2811,15 @@ static void mana_update_rx_dim(struct mana_cq *cq)
>  	if (!smp_load_acquire(&apc->rx_dim_enabled))
>  		return;
>  
> -	dim_update_sample(READ_ONCE(cq->dim_event_ctr), rxq->stats.packets,
> -			  rxq->stats.bytes, &dim_sample);
> +	/* A retiring queue counts elsewhere and is about to be destroyed, so
> +	 * there is no moderation left to tune and its samples would step off
> +	 * the shared slot onto a counter that restarts at zero.
> +	 */
> +	if (READ_ONCE(rxq->retiring))
> +		return;
> +
> +	dim_update_sample(READ_ONCE(cq->dim_event_ctr), rxq->stats->packets,
> +			  rxq->stats->bytes, &dim_sample);
>  	net_dim(&cq->dim, &dim_sample);
>  }

[Severity: Low]
After this change the DIM input is apc->rxq_stats[rxq_idx], a per-index
counter shared by every queue generation that served that index, while
cq->dim.start_sample belongs to one queue instance.  When the retiring flag
is cleared again on the rollback path, is anything re-basing cq->dim?

mana_publish_qset() rollback:

	mana_qset_set_retiring(newq, out_old, true);
	synchronize_net();
	mana_qset_set_retiring(out_old, NULL, false);

The restored @out_old queues resume sampling with the start_sample they
captured before the swap, at which point the shared slot held X.  The
@newq queues were unmarked and installed earlier and may have advanced the
same slot to Y > X (steering can already point at them when
mana_config_rss() fails part-way), so the first dim_update_sample() after
the rollback attributes another queue's packets and bytes to this queue.

mana_dim_change() memsets cq->dim on enable for what looks like the same
reason:

	if (enable) {
		memset(&cq->dim, 0, sizeof(cq->dim));

Also, the commit message describes the change as moving statistics storage
only, and does not mention that the RX interrupt-moderation input source
and its new skip condition changed.  Should that be called out?

[ ... ]

> @@ -4230,16 +4382,33 @@ static void mana_start_txqs(struct mana_port_context *apc)
>   * A queue both sets own must end up unmarked, so callers mark the leaving set
>   * first and unmark the incoming one second.
>   */
> -static void mana_qset_set_retiring(struct mana_qset *qset, bool retiring)
> +static void mana_qset_set_retiring(struct mana_qset *qset,
> +				   const struct mana_qset *keep, bool retiring)
>  {
>  	unsigned int q;
>  
> -	if (!qset->tx_qp)
> -		return;
> -
>  	for (q = 0; q < qset->num_queues; q++) {
> -		if (qset->tx_qp[q])
> +		if (qset->tx_qp && qset->tx_qp[q])
>  			WRITE_ONCE(qset->tx_qp[q]->txq.retiring, retiring);
> +
> +		if (!qset->rxqs || !qset->rxqs[q])
> +			continue;
> +
> +		/* A queue @keep carries over serves the same index before and
> +		 * after, so it stays the live writer of that index. Marking it
> +		 * would strand the counts it takes during the swap in
> +		 * drain_stats, which only mana_destroy_rxq() drains.
> +		 */

[Severity: Low]
Same comment inconsistency as above: "which only mana_destroy_rxq()
drains" is no longer true once the rollback path below calls
mana_fold_qset_rx_stats().

[ ... ]

> @@ -4384,8 +4553,23 @@ int mana_publish_qset(struct mana_port_context *apc, struct mana_qset *newq,
>  	/* The roles are swapped now: @newq is the set going away and @out_old
>  	 * is live again. Same ordering rule, leaving set first.
>  	 */
> -	mana_qset_set_retiring(newq, true);
> -	mana_qset_set_retiring(out_old, false);
> +	mana_qset_set_retiring(newq, out_old, true);
> +
> +	/* Same grace period as the forward path: a poll that sampled the flag
> +	 * before the line above must finish before @out_old is unmarked, or
> +	 * both sets would briefly count into apc->rxq_stats[].
> +	 */
> +	synchronize_net();
> +
> +	mana_qset_set_retiring(out_old, NULL, false);
> +
> +	/* @out_old counted into drain_stats while it was marked, and it is
> +	 * about to serve again rather than be destroyed, so nothing else
> +	 * would ever publish those packets. Fold them now, once the polls
> +	 * that still saw the flag above have finished writing.
> +	 */
> +	synchronize_net();
> +	mana_fold_qset_rx_stats(apc, out_old);

[Severity: Low]
This is the second writer of apc->rxq_stats_ret[] that the header comment
and mana_qset_set_retiring() both say does not exist.  Both writers do run
under RTNL, so the serialization holds; only the documentation is stale.

[ ... ]

> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index 61d136736cf6c..9a43856760776 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h

[ ... ]

> @@ -415,7 +419,23 @@ struct mana_rxq {
>  
>  	u32 buf_index;
>  
> -	struct mana_stats_rx stats;
> +	/* Points into apc->rxq_stats[], which outlives the queue. Only the
> +	 * live queue at this index writes there; once retiring is set this
> +	 * queue counts into drain_stats instead, so the slot has one writer.
> +	 * Use mana_rxq_stats() rather than either directly.
> +	 */
> +	struct mana_stats_rx *stats;
> +
> +	/* Set under RTNL before a different queue takes over this index. A
> +	 * queue carried across a swap keeps serving its index and is never
> +	 * marked.
> +	 */
> +	bool retiring;
> +
> +	/* What this queue counted after it stopped being the live one.
> +	 * Folded into apc->rxq_stats_ret[] when the queue is destroyed.
> +	 */
> +	struct mana_stats_rx drain_stats;

[Severity: Low]
"Folded into apc->rxq_stats_ret[] when the queue is destroyed" - the
rollback path folds drain_stats for queues that are not destroyed but
resume service.  Could this be reworded to cover both cases?

> @@ -623,6 +643,19 @@ struct mana_port_context {
>  	unsigned int max_queues;
>  	unsigned int num_queues;
>  
> +	/* Per-queue counters, max_queues entries each. Allocated at probe and
> +	 * freed at remove, never on queue teardown, so a reconfiguration does
> +	 * not reset them.
> +	 *
> +	 * rxq_stats[] is written by the live RX queue at that index and
> +	 * rxq_stats_ret[] only under RTNL, by mana_destroy_rxq() folding in
> +	 * what a retiring queue counted while it drained. One writer each;
> +	 * readers add the two.
> +	 */

[Severity: Low]
"only under RTNL, by mana_destroy_rxq()" and "One writer each" are the two
claims that the mana_alloc_queue_stats() comment and the
mana_fold_qset_rx_stats() call in mana_publish_qset()'s rollback path
contradict.  Should this describe both rxq_stats_ret[] writers, and drop
the "One writer each" statement for the live slots?

> +	struct mana_stats_rx *rxq_stats;
> +	struct mana_stats_rx *rxq_stats_ret;
> +	struct mana_stats_tx *txq_stats;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901014442.2945689-1-longli%40microsoft.com