Re: [PATCH net-next-2.6] net: Introduce u64_stats_sync infrastructure

[Nick Piggin <npiggin@suse.de>]

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!