RE: [EXTERNAL] Re: [PATCH net-next v2 08/13] net: mana: keep per-queue statistics in the port context

> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [Medium] Concurrent writers to the same `u64_stats_sync` structure during
> RX queue swap.
> --
> 
> --- Patch [8]: [PATCH net-next v2 08/13] net: mana: keep per-queue statistics in
> the port context --- commit 48e144b542daf3b7f2ff07b74db60098e784159d
> Author: Long Li <[email protected]>
> 
> net: mana: keep per-queue statistics in the port context
> 
> This commit moves per-queue statistics out of the queue structures and into
> a port-context array to prevent statistics from resetting when the queue is
> reconfigured. The stats are indexed by the queue index, and the arrays
> outlive the queues so totals stay monotonic.
> 
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> [ ... ]
> > @@ -1059,6 +1065,48 @@ 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.
> > + */
> 
> [Severity: Medium]
> Is it safe to allow concurrent writers to the same u64_stats_sync structure?
> 
> The u64_stats_sync API requires that writers must be mutually exclusive for a
> given syncp. While MANA is 64-bit only and lacks a seqcount in the syncp,
> allowing both the retiring RX queue's NAPI and the new RX queue's NAPI to
> concurrently update the same apc->rxq_stats[rxq_idx] introduces a data race.
> 
> For example, both could be running mana_rx_skb() at the same time:
> 
> 	u64_stats_update_begin(&rx_stats->syncp);
> 	rx_stats->packets++;
> 	rx_stats->bytes += pkt_len;
> 
> Could this lead to undefined behavior or KCSAN warnings when the queues
> overlap during a swap?

Wil fix this.

Long

> 
> > +static int mana_alloc_queue_stats(struct mana_port_context *apc) {
> > +	unsigned int i;
> 
> --
> Sashiko AI
> review * https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%
> 2Fsashiko.dev%2F%23%2Fpatchset%2F20260813050418.2906468-1-
> longli%40microsoft.com%3Fpart%3D8&data=05%7C02%7Clongli%40microsoft.
> com%7C310903ff6b08415ea5b508def9c1a301%7C72f988bf86f141af91ab2d7c
> d011db47%7C1%7C0%7C639222807222824332%7CUnknown%7CTWFpbGZsb
> 3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkF
> OIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=%2BNqZwTPmXgY
> tTuwJUM%2Fj%2FSq%2F8ZbA9FR0XOQrz81lpDQ%3D&reserved=0