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: <1314084577.4791.30.camel@edumazet-laptop>
Date:	Tue, 23 Aug 2011 09:29:37 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	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 à 00:06 -0700, Sathya.Perla@...lex.Com a écrit :
> >-----Original Message-----
> >From: Eric Dumazet [mailto:eric.dumazet@...il.com]
> >Sent: Tuesday, August 23, 2011 12:11 PM
> >
> >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.
> <...>
> >>
> >> +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.
> Yes, the new code still *does not* do a sum of all queues. The code just 
> parses per-queue stats. For each queue, the 16 bit
> HW stats value is taken and stored in a 32-bit *per-queue* variable.
> The comment that "driver accumulates a 32-bit val" may be misleading. The
> code here is not doing a sum of the per-queue stat.

Ah ok, I now see the code intent, and everything seems fine now, thanks
for explaining.


--
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