netdev - Re: [PATCH net-next-2.6] net: Introduce u64_stats

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]

Date:	Tue, 15 Jun 2010 21:04:13 +1000
From:	Nick Piggin <npiggin@...e.de>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	bhutchings@...arflare.com
Subject: Re: [PATCH net-next-2.6] net: Introduce u64_stats_sync
 infrastructure

On Tue, Jun 15, 2010 at 12:43:25PM +0200, Eric Dumazet wrote:
> Le mardi 15 juin 2010 à 20:25 +1000, Nick Piggin a écrit :
> > On Tue, Jun 15, 2010 at 12:14:16PM +0200, Eric Dumazet wrote:
> > > Here is the followup patch to abstract things a bit, before upcoming
> > > conversions.
> > > 
> > > Thanks !
> > > 
> > > [PATCH net-next-2.6] net: Introduce u64_stats_sync infrastructure
> > > 
> > > To properly implement 64bits network statistics on 32bit or 64bit hosts,
> > > we provide one new type and four methods, to ease conversions.
> > > 
> > > Stats producer should use following template granted it already got an
> > > exclusive access to counters (a previous lock is taken, or per cpu
> > > data [used in a non preemptable context])
> > > 
> > > Let me repeat : stats producers must be serialized by other means before
> > > using this template. Preemption must be disabled too.
> > > 
> > > u64_stats_update_begin(&stats->syncp);
> > > stats->bytes += len;
> > > stats->packets++;
> > > u64_stats_update_end(&stats->syncp);
> > > 
> > > While a consumer should use following template to get consistent
> > > snapshot :
> > > 
> > > u64 tbytes, tpackets;
> > > unsigned int start;
> > > 
> > > do {
> > > 	start = u64_stats_fetch_begin(&stats->syncp);
> > > 	tbytes = stats->bytes;
> > > 	tpackets = stats->packets;
> > > } while (u64_stats_fetch_retry(&stats->lock, syncp));
> > > 
> > > This patch uses this infrastructure in net loopback driver, instead of
> > > specific one added in commit 6b10de38f0ef (loopback: Implement 64bit
> > > stats on 32bit arches)
> > > 
> > > Suggested by David Miller
> > 
> > Cool, I don't mind this, but perhaps could you add some comments
> > because it _will_ either be misused or copied and misused elsewhere :)
> > 
> > Callers must:
> > - write side must ensure mutual exclusion (even if it was previously OK
> >   to have lost updates on the writer side, the seqlock will explodde if
> >   it is taken concurrently for write)
> > - write side must not sleep
> > - readside and writeside must have local-CPU exclusion from one another;
> >   preempt, irq, bh as appropriate
> > - will only protect 64-bit sizes from tearing -- eg updating 2 different
> >   stats under the same write side will not ensure they are both seen in
> >   the same read side
> 
> Hmm, I am not sure I got this one, could you please give me a buggy
> example ?

So if you have a regular seqlock, the sequence:

write_seqcount_begin
stat1++
stat2--
write_seqcount_end

do read_seqcount_begin
  sum = stat1+stat2;
while (read_seqcount_retry)
BUG_ON(sum != 0);

This code is OK. But if it is using your stat sync, then it is buggy.
This is obvious to you of course, but someone who doesn't consider
the implementation might get caught out.

I guess all my other points are properties of seqcount code itself,
but they are not documented really well with the seqlock API
unfortunately.


> > But I do like the minimal design.
> 
> Thanks !
> 
> I'll submit a v2 patch after my lunch to add all your comments, because
> all clarifications are indeed very very welcomed !

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