Re: [PATCH net-next v02 2/5] hinic3: Avoid concurrent updates on 32-bit systems.

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch is titled "Avoid concurrent updates on 32-bit systems",
  says the `+=` inside the u64_stats retry loop of hinic3_get_stats64()…
- [Low] After this patch, struct hinic3_rxq_stats::csum_errors
  (hinic3_rx.h:36) has no consumer anywhere in the tree.

--

hinic3: Avoid concurrent updates on 32-bit systems.

This commit changes hinic3_get_stats64() to read the per-queue counters
by assignment inside the u64_stats retry loop and add them into
rtnl_link_stats64 after the loop. It also removes csum_errors from
rx_errors, removes the NULL checks on nic_dev->txqs and nic_dev->rxqs,
and removes the dead stores before the loops.

> hinic3: Avoid concurrent updates on 32-bit systems.
>
> Within the while loop of hinic3_get_stats64(), the counters between
> u64_stats_fetch_retry and u64_stats_fetch_begin are incremented by +=.
> This operation will cause repeated counting of the counters on a 32-bit
> system. Therefore, change += to + and the counters are incremented
> out side the wile loop.

[ ... ]

> Fixes: f47872bed40f ("hinic3: Add .ndo_tx_timeout and .ndo_get_stats64")

[Severity: Low]
Can the double counting described here happen in this driver?

The hinic3 Kconfig entry only allows 64-bit builds, and COMPILE_TEST
does not remove that limit:

drivers/net/ethernet/huawei/hinic3/Kconfig:
	depends on X86 || ARM64 || COMPILE_TEST
	depends on PCI_MSI && 64BIT

When BITS_PER_LONG == 64, the retry helper always returns false:

include/linux/u64_stats_sync.h:
static inline bool __u64_stats_fetch_retry(const struct u64_stats_sync *syncp,
					   unsigned int start)
{
	return false;
}

So each do/while body in hinic3_get_stats64() runs exactly once, and the
old += loop already gave correct totals.

Even on a hypothetical 32-bit build, nothing would force a retry. The
only u64_stats_update_begin()/u64_stats_update_end() sections in hinic3
seem to be the init-time zeroing helpers hinic3_txq_clean_stats() and
hinic3_rxq_clean_stats().

The new reader pattern is fine as a cleanup. Would it be better to
describe the patch as a cleanup, drop the Fixes: tag and target net-next?

With the tag, a change that has no runtime effect goes to stable. It
would also bring along the NULL check removal, the dead store removal
and the change to what rx_errors reports.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> index da73811641a9d..d1fd91d97ccfc 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> @@ -791,49 +791,35 @@ 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: Low]
The commit message says:

    csum_errors and other_errors are not the same things. csum_errors will
    go to csum_bad in netdev netlink qstats.

Does hinic3 have a qstats path for this? I couldn't find netdev_stat_ops,
get_queue_stats_rx or csum_bad anywhere under
drivers/net/ethernet/huawei/hinic3/.

That means this line removes the last reader of csum_errors in struct
hinic3_rxq_stats.

The field has no writer either. hinic3_rx_csum() detects the checksum
error but returns without counting it:

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

The only other reference is the zeroing in hinic3_rxq_clean_stats().

Removing checksum errors from rx_errors does match the rx-csum-bad
definition. Could the commit message be corrected, or the qstats
reporting be added along with an increment in hinic3_rx_csum()?

As things stand, csum_errors is declared and zeroed but never counted
or reported. If a writer is added later, checksum errors still won't
show up in any user-visible interface until qstats support exists.

> +		} 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;

[ ... ]

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