Re: [PATCH net-next v01 2/4] Fixes: Avoid concurrent updates on 32-bit systems.

[netdev-bot+sashiko@kernel.org]

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

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

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] The subject and body of commit "Fixes: Avoid concurrent
  updates on 32-bit systems." do not match the change.
- [Medium] hinic3_get_stats64()
  (drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c) quietly…
- [Low] The patch fixes a double-counting bug that cannot happen at this
  tree state, or at the end of the series.
- [Low] hinic3: dead counter initializations in hinic3_get_stats64()

--

Fixes: Avoid concurrent updates on 32-bit systems.

This commit changes the u64_stats reader retry loops in
hinic3_get_stats64(). The per-queue values are now assigned inside the
loop and added into the rtnl_link_stats64 fields after it. It also drops
the nic_dev->txqs and nic_dev->rxqs NULL checks and stops adding
csum_errors into rx_errors.

> Fixes: Avoid concurrent updates on 32-bit systems.

[Severity: Medium]
Could the subject be reworked?

It has no subsystem prefix. Something like "net: hinic3:" would be
expected.

It also starts with "Fixes:", which is a trailer tag. That text isn't
followed by a commit reference, and the tags have no real
Fixes: <sha> ("...") trailer. Tools that grep for '^Fixes:' may also
match this subject.

The subject says the patch avoids concurrent updates, but none of the
writers are changed. The only change is on the reader side of
hinic3_get_stats64(), where '+=' becomes '=' inside the retry loop.

> As noted in the subsequent review.

The body starts with this line, then repeats a reviewer's comment about
the old code. It never says what this patch does. Could the changelog
describe the change itself?

The other patches in the series use the same layout. In "Fixes: Solve an
LRO stall issue" the body even keeps the reviewer's question "Was it
intended? ... could it go in a separate patch".

Were these review fixups meant to be folded into the patches they
address, rather than sent as standalone commits?

> If the loop retries due to concurrent updates on 32-bit systems,
> the counters are added multiple times. This was previously benign
> since the fields weren't populated by the driver, but these new
> updates will cause incorrect statistics on 32-bit platforms.

[Severity: Low]
Which new updates does this refer to?

In drivers/net/ethernet/huawei/hinic3/, the only writers of txq_stats
and rxq_stats are hinic3_txq_clean_stats() and hinic3_rxq_clean_stats().
Both only zero the counters:

hinic3_rx.c:hinic3_rxq_clean_stats() {
	u64_stats_update_begin(&rxq_stats->syncp);
	rxq_stats->bytes = 0;
	rxq_stats->packets = 0;
	...
}

The rx checksum error path in hinic3_rx_csum() doesn't count anything
either:

	if (unlikely(csum_err)) {
		/* pkt type is recognized by HW, and csum is wrong */
		skb->ip_summed = CHECKSUM_NONE;
		return;
	}

So a retry of the old '+=' loop would only add zeros. No per-packet
updates are added by this patch, by the baseline, or by any later patch
in the series.

Should this be squashed into the patch that starts populating the
counters, instead of being a standalone fix?

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> index da73811641a9d..474ea81f17508 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> @@ -795,45 +795,38 @@ static void hinic3_get_stats64(struct net_device *netdev,

[ ... ]

>  	for (i = 0; i < nic_dev->max_qps; i++) {
> -		if (!nic_dev->rxqs)
> -			break;
> -
>  		rxq = &nic_dev->rxqs[i];
>  		rxq_stats = &rxq->rxq_stats;
>  		do {
>  			start = u64_stats_fetch_begin(&rxq_stats->syncp);
> -			bytes += rxq_stats->bytes;
> -			packets += rxq_stats->packets;
> -			errors += rxq_stats->csum_errors +
> -				rxq_stats->other_errors;
> -			dropped += rxq_stats->dropped;
> +			bytes = rxq_stats->bytes;
> +			packets = rxq_stats->packets;
> +			errors = rxq_stats->other_errors;
> +			dropped = rxq_stats->dropped;

[Severity: Medium]
Is dropping csum_errors from rx_errors intended here? The old code did:

	errors += rxq_stats->csum_errors + rxq_stats->other_errors;

The new code reads only other_errors, so rtnl_link_stats64.rx_errors no
longer includes checksum errors. The retry fix didn't need this, and the
commit message doesn't mention it.

struct hinic3_rxq_stats still declares csum_errors, and
hinic3_rxq_clean_stats() still resets it under the syncp. After this
patch, nothing reads it.

The sibling hinic driver still reports errors = csum_errors +
other_errors in hinic_rxq_get_stats(), which is what hinic3 did before
this patch.

Once the rx path starts populating csum_errors, would checksum failures
drop out of rx_errors in 'ip -s link', with no other counter reporting
them? hinic3_get_stats64() is unchanged at the end of the series, so no
later patch restores it.

The same patch also removes the nic_dev->txqs and nic_dev->rxqs NULL
checks. That looks safe: hinic3_sw_init() allocates txqs and rxqs before
register_netdev(). In hinic3_nic_remove(), hinic3_sw_uninit() frees them
after unregister_netdev().

It's still unrelated to the stated fix, though. Could these two changes
be explained in the changelog or moved into separate patches?

[ ... ]

> @@ -795,45 +795,38 @@ static void hinic3_get_stats64(struct net_device *netdev,
>  	packets = 0;
>  	dropped = 0;
>  	for (i = 0; i < nic_dev->max_qps; i++) {
> -		if (!nic_dev->txqs)
> -			break;
> -
>  		txq = &nic_dev->txqs[i];
>  		txq_stats = &txq->txq_stats;
>  		do {
>  			start = u64_stats_fetch_begin(&txq_stats->syncp);
> -			bytes += txq_stats->bytes;
> -			packets += txq_stats->packets;
> -			dropped += txq_stats->dropped;
> +			bytes = txq_stats->bytes;
> +			packets = txq_stats->packets;
> +			dropped = txq_stats->dropped;
>  		} while (u64_stats_fetch_retry(&txq_stats->syncp, start));
> +		stats->tx_packets += packets;
> +		stats->tx_bytes   += bytes;
> +		stats->tx_dropped += dropped;
>  	}
> -	stats->tx_packets = packets;
> -	stats->tx_bytes   = bytes;
> -	stats->tx_dropped = dropped;
>  
>  	bytes = 0;
>  	packets = 0;
>  	errors = 0;
>  	dropped = 0;
>  	for (i = 0; i < nic_dev->max_qps; i++) {
> -		if (!nic_dev->rxqs)
> -			break;
> -
>  		rxq = &nic_dev->rxqs[i];
>  		rxq_stats = &rxq->rxq_stats;
>  		do {
>  			start = u64_stats_fetch_begin(&rxq_stats->syncp);
> -			bytes += rxq_stats->bytes;
> -			packets += rxq_stats->packets;
> -			errors += rxq_stats->csum_errors +
> -				rxq_stats->other_errors;
> -			dropped += rxq_stats->dropped;
> +			bytes = rxq_stats->bytes;
> +			packets = rxq_stats->packets;
> +			errors = rxq_stats->other_errors;
> +			dropped = rxq_stats->dropped;
>  		} while (u64_stats_fetch_retry(&rxq_stats->syncp, start));
> +		stats->rx_packets += packets;
> +		stats->rx_bytes   += bytes;
> +		stats->rx_errors  += errors;
> +		stats->rx_dropped += dropped;
>  	}
> -	stats->rx_packets = packets;
> -	stats->rx_bytes   = bytes;
> -	stats->rx_errors  = errors;
> -	stats->rx_dropped = dropped;
>  }
>  
>  static void hinic3_nic_set_rx_mode(struct net_device *netdev)

[Severity: Low]
Now that the reads inside both retry loops use '=' instead of '+=',
the resets before the loops are dead stores. That is bytes, packets and
dropped before the tx loop, and bytes, packets, errors and dropped
before the rx loop.

Every iteration of the for loop overwrites those locals in the do/while
before anything reads them. The per-queue values are then summed into
the rtnl_link_stats64 fields, which dev_get_stats() has already cleared
before it calls ndo_get_stats64. Nothing reads the zero values.

This causes no functional problem. The leftover resets still make the
locals look like accumulators, which is the pattern this patch is
removing.

Could the resets be dropped? Alternatively, the locals could be declared
inside the loop bodies, so it's clear they only hold one queue's
snapshot.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793946318.git.gongfan1%40huawei.com