lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7f1604fd-4bd6-4f16-8aed-2586afac7d15@lunn.ch>
Date: Fri, 10 Nov 2023 16:13:30 +0100
From: Andrew Lunn <andrew@...n.ch>
To: alexey.pakhunov@...cex.com
Cc: mchan@...adcom.com, vincent.wong2@...cex.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, siva.kallam@...adcom.com,
	prashant@...adcom.com
Subject: Re: [PATCH v2 1/2] tg3: Move the [rt]x_dropped counters to tg3_napi

> @@ -11895,6 +11903,9 @@ static void tg3_get_nstats(struct tg3 *tp, struct rtnl_link_stats64 *stats)
>  {
>  	struct rtnl_link_stats64 *old_stats = &tp->net_stats_prev;
>  	struct tg3_hw_stats *hw_stats = tp->hw_stats;
> +	unsigned long rx_dropped;
> +	unsigned long tx_dropped;
> +	int i;
>  
>  	stats->rx_packets = old_stats->rx_packets +
>  		get_stat64(&hw_stats->rx_ucast_packets) +
> @@ -11941,8 +11952,26 @@ static void tg3_get_nstats(struct tg3 *tp, struct rtnl_link_stats64 *stats)
>  	stats->rx_missed_errors = old_stats->rx_missed_errors +
>  		get_stat64(&hw_stats->rx_discards);
>  
> -	stats->rx_dropped = tp->rx_dropped;
> -	stats->tx_dropped = tp->tx_dropped;
> +	/* Aggregate per-queue counters. The per-queue counters are updated
> +	 * by a single writer, race-free. The result computed by this loop
> +	 * might not be 100% accurate (counters can be updated in the middle of
> +	 * the loop) but the next tg3_get_nstats() will recompute the current
> +	 * value so it is acceptable.
> +	 *
> +	 * Note that these counters wrap around at 4G on 32bit machines.
> +	 */
> +	rx_dropped = (unsigned long)(old_stats->rx_dropped);
> +	tx_dropped = (unsigned long)(old_stats->tx_dropped);

Isn't this wrapping artificial? old_stats is of type
rtnl_link_stats64, so the counters are 64 bit.

> +
> +	for (i = 0; i < tp->irq_cnt; i++) {
> +		struct tg3_napi *tnapi = &tp->napi[i];
> +
> +		rx_dropped += tnapi->rx_dropped;
> +		tx_dropped += tnapi->tx_dropped;
> +	}
> +
> +	stats->rx_dropped = rx_dropped;
> +	stats->tx_dropped = tx_dropped;

state is also rtnl_link_stats64 so has 64 bit counters. So this code
is throwing away the upper 32 bits.

Why not use include/linux/u64_stats_sync.h, which should cost you
nothing on 64 bit machines, and do the right thing on 32 bit machines.

	Andrew

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ