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
