netdev - Re: [PATCH net-next 3/5] be2net: fix erx->rx_drops_no

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: <1314081677.4791.28.camel@edumazet-laptop>
Date:	Tue, 23 Aug 2011 08:41:17 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Sathya Perla <sathya.perla@...lex.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH net-next 3/5] be2net: fix erx->rx_drops_no_frags wrap
 around

Le mardi 23 août 2011 à 11:11 +0530, Sathya Perla a écrit :
> The rx_drops_no_frags HW counter for RSS rings is 16bits in HW and can
> wraparound often. Maintain a 32-bit accumulator in the driver to prevent
> frequent wraparound.
> 
> Also, incorporated Eric's feedback to use ACCESS_ONCE() for the accumulator
> write.
> 
> Signed-off-by: Sathya Perla <sathya.perla@...lex.com>
> ---
>  drivers/net/ethernet/emulex/benet/be_main.c |   22 +++++++++++++++++++---
>  1 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
> index 2375c0c..fb2eda0 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> @@ -378,6 +378,18 @@ static void populate_lancer_stats(struct be_adapter *adapter)
>  				pport_stats->rx_drops_too_many_frags_lo;
>  }
>  
> +static void accumulate_16bit_val(u32 *acc, u16 val)
> +{
> +#define lo(x)			(x & 0xFFFF)
> +#define hi(x)			(x & 0xFFFF0000)
> +	bool wrapped = val < lo(*acc);
> +	u32 newacc = hi(*acc) + val;
> +
> +	if (wrapped)
> +		newacc += 65536;
> +	ACCESS_ONCE(*acc) = newacc;
> +}
> +

I still feel something is wrong here :

>  void be_parse_stats(struct be_adapter *adapter)
>  {
>  	struct be_erx_stats_v1 *erx = be_erx_stats_from_cmd(adapter);
> @@ -394,9 +406,13 @@ void be_parse_stats(struct be_adapter *adapter)
>  	}
>  
>  	/* as erx_v1 is longer than v0, ok to use v1 defn for v0 access */
> -	for_all_rx_queues(adapter, rxo, i)
> -		rx_stats(rxo)->rx_drops_no_frags =
> -			erx->rx_drops_no_fragments[rxo->q.id];

previous code was not doing a sum_of_all_queues.

It only gave the final erx->rx_drops_no_fragments[rxo->q.id], not taking
into account previous rx_stats(rxo)->rx_drops_no_frags value.

Your changelog is about wrap around, while the bug might have be
different (No real sum)

Now you say : previous value is meaningfull, and we add to it 16bits
values.

> +	for_all_rx_queues(adapter, rxo, i) {
> +		/* below erx HW counter can actually wrap around after
> +		 * 65535. Driver accumulates a 32-bit value
> +		 */
> +		accumulate_16bit_val(&rx_stats(rxo)->rx_drops_no_frags,
> +				(u16)erx->rx_drops_no_fragments[rxo->q.id]);
> +	}
>  }
>  

Arent multiple calls to be_parse_stats() will have wrong final
rx_drops_no_frags value ?

Or are the rx_drops_no_fragments[rxo->q.id] cleared when read ?

I am afraid that if HW maintains 16bit values, then the only way is to
also have a 16bit accumulator. You cannot detect wraparounds without
also keeping a copy of previous 16bit samples.

u16 accum = 0;
or_all_rx_queues(adapter, rxo, i) {
	accum += erx->rx_drops_no_fragments[rxo->q.id];
}
rx_stats(rxo)->rx_drops_no_frags = accum;


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html