Message ID | 1230300535.9487.292.camel@twins |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
Peter Zijlstra wrote: > On Thu, 2008-12-25 at 22:26 +1100, Herbert Xu wrote: >> On Thu, Dec 25, 2008 at 10:25:44AM +0000, Jeff Kirsher wrote: >>> >>> [ 1439.758437] >>> ====================================================== [ >>> 1439.758724] [ INFO: soft-safe -> soft-unsafe lock order detected ] >>> [ 1439.758868] 2.6.28-rc8-net-next-igb #13 [ 1439.759007] >>> ------------------------------------------------------ [ >>> 1439.759150] netperf/22302 [HC0[0]:SC0[1]:HE1:SE0] is trying to >>> acquire: [ 1439.759293] (&fbc->lock){--..}, at: >>> [<ffffffff803691a6>] __percpu_counter_add+0x4a/0x6d [ 1439.759581] >>> [ 1439.759582] and this task is already holding: >>> [ 1439.759853] (slock-AF_INET){-+..}, at: [<ffffffff804fdca6>] >>> tcp_close+0x16c/0x2da [ 1439.760137] which would create a new lock >>> dependency: [ 1439.762122] (slock-AF_INET){-+..} -> >>> (&fbc->lock){--..} >> >> This is a false positive. The lock slock is not a normal lock. >> It's an ancient creature that's a spinlock in interrupt context >> and a semaphore in process context. >> >> In particular, holding slock in process context does not disable >> softirqs and you're still allowed to take the spinlock portion of >> slock on the same CPU through an interrupt. What happens is that >> the softirq will notice that the slock is already taken by process >> context, and defer the work for later. > > Which doesn't seem to be relevant to the report in question. > > What happens is that two different percpu counters with different irq > semantics get put in the same class (I suspect Eric never tested this > stuff with lockdep enabled). > > Does the below -- which isn't even compile tested solve the issue? > > Jeff, would you, in future, take care not to word wrap splats like > that, > it takes way too much effort to untangle that mess. > > --- > Subject: lockdep: annotate percpu_counter > > Classify percpu_counter instances similar to regular lock objects -- > that is, per instantiation site. > > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> > --- > include/linux/percpu_counter.h | 14 ++++++++++---- > lib/percpu_counter.c | 18 ++++-------------- > lib/proportions.c | 6 +++--- > mm/backing-dev.c | 2 +- > 4 files changed, 18 insertions(+), 22 deletions(-) > > diff --git a/include/linux/percpu_counter.h > b/include/linux/percpu_counter.h > index 9007ccd..a074d77 100644 > --- a/include/linux/percpu_counter.h > +++ b/include/linux/percpu_counter.h > @@ -30,8 +30,16 @@ struct percpu_counter { > #define FBC_BATCH (NR_CPUS*4) > #endif > > -int percpu_counter_init(struct percpu_counter *fbc, s64 amount); > -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount); > +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, > + struct lock_class_key *key); > + > +#define percpu_counter_init(fbc, value) \ > + do { \ > + static struct lock_class_key __key; \ > + \ > + __percpu_counter_init(fbc, value, &__key); \ > + } while (0) > + > void percpu_counter_destroy(struct percpu_counter *fbc); > void percpu_counter_set(struct percpu_counter *fbc, s64 amount); > void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, > s32 batch); @@ -85,8 +93,6 @@ static inline int > percpu_counter_init(struct percpu_counter *fbc, s64 amount) return > 0; } > > -#define percpu_counter_init_irq percpu_counter_init > - > static inline void percpu_counter_destroy(struct percpu_counter *fbc) > { > } > diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c > index b255b93..4bb0ed3 100644 > --- a/lib/percpu_counter.c > +++ b/lib/percpu_counter.c > @@ -68,11 +68,11 @@ s64 __percpu_counter_sum(struct percpu_counter > *fbc) } > EXPORT_SYMBOL(__percpu_counter_sum); > > -static struct lock_class_key percpu_counter_irqsafe; > - > -int percpu_counter_init(struct percpu_counter *fbc, s64 amount) > +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, > + struct lock_class_key *key) > { > spin_lock_init(&fbc->lock); > + lockdep_set_class(&fbc->lock, key); > fbc->count = amount; > fbc->counters = alloc_percpu(s32); > if (!fbc->counters) > @@ -84,17 +84,7 @@ int percpu_counter_init(struct percpu_counter > *fbc, s64 amount) #endif > return 0; > } > -EXPORT_SYMBOL(percpu_counter_init); > - > -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount) > -{ > - int err; > - > - err = percpu_counter_init(fbc, amount); > - if (!err) > - lockdep_set_class(&fbc->lock, &percpu_counter_irqsafe); > - return err; > -} > +EXPORT_SYMBOL(__percpu_counter_init); > > void percpu_counter_destroy(struct percpu_counter *fbc) > { > diff --git a/lib/proportions.c b/lib/proportions.c > index 4f387a6..7367f2b 100644 > --- a/lib/proportions.c > +++ b/lib/proportions.c > @@ -83,11 +83,11 @@ int prop_descriptor_init(struct prop_descriptor > *pd, int shift) pd->index = 0; > pd->pg[0].shift = shift; > mutex_init(&pd->mutex); > - err = percpu_counter_init_irq(&pd->pg[0].events, 0); > + err = percpu_counter_init(&pd->pg[0].events, 0); > if (err) > goto out; > > - err = percpu_counter_init_irq(&pd->pg[1].events, 0); > + err = percpu_counter_init(&pd->pg[1].events, 0); > if (err) > percpu_counter_destroy(&pd->pg[0].events); > > @@ -191,7 +191,7 @@ int prop_local_init_percpu(struct > prop_local_percpu *pl) spin_lock_init(&pl->lock); > pl->shift = 0; > pl->period = 0; > - return percpu_counter_init_irq(&pl->events, 0); > + return percpu_counter_init(&pl->events, 0); > } > > void prop_local_destroy_percpu(struct prop_local_percpu *pl) > diff --git a/mm/backing-dev.c b/mm/backing-dev.c > index 801c08b..a7c6c56 100644 > --- a/mm/backing-dev.c > +++ b/mm/backing-dev.c > @@ -223,7 +223,7 @@ int bdi_init(struct backing_dev_info *bdi) > bdi->max_prop_frac = PROP_FRAC_BASE; > > for (i = 0; i < NR_BDI_STAT_ITEMS; i++) { > - err = percpu_counter_init_irq(&bdi->bdi_stat[i], 0); > + err = percpu_counter_init(&bdi->bdi_stat[i], 0); > if (err) > goto err; > } This patch fails to compile: mm/backing-dev.c: In function 'bdi_init': mm/backing-dev.c:226: error: expected expression bedore 'do' Thakns, Emil -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2008-12-27 at 12:38 -0700, Tantilov, Emil S wrote: > Peter Zijlstra wrote: > > index 9007ccd..a074d77 100644 > > --- a/include/linux/percpu_counter.h > > +++ b/include/linux/percpu_counter.h > > @@ -30,8 +30,16 @@ struct percpu_counter { > > #define FBC_BATCH (NR_CPUS*4) > > #endif > > > > -int percpu_counter_init(struct percpu_counter *fbc, s64 amount); > > -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount); > > +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, > > + struct lock_class_key *key); > > + > > +#define percpu_counter_init(fbc, value) \ > > + do { \ > > + static struct lock_class_key __key; \ > > + \ > > + __percpu_counter_init(fbc, value, &__key); \ > > + } while (0) > > + > > void percpu_counter_destroy(struct percpu_counter *fbc); > > void percpu_counter_set(struct percpu_counter *fbc, s64 amount); > > void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, > This patch fails to compile: > > mm/backing-dev.c: In function 'bdi_init': > mm/backing-dev.c:226: error: expected expression bedore 'do' Ah indeed, stupid me... Please try something like this instead of the above hunk: @@ -30,8 +30,16 @@ struct percpu_counter { #define FBC_BATCH (NR_CPUS*4) #endif -int percpu_counter_init(struct percpu_counter *fbc, s64 amount); -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount); +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, + struct lock_class_key *key); + +#define percpu_counter_init(fbc, value) \ + ({ \ + static struct lock_class_key __key; \ + \ + __percpu_counter_init(fbc, value, &__key); \ + }) + void percpu_counter_destroy(struct percpu_counter *fbc); void percpu_counter_set(struct percpu_counter *fbc, s64 amount); void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Peter Zijlstra wrote: > On Sat, 2008-12-27 at 12:38 -0700, Tantilov, Emil S wrote: >> Peter Zijlstra wrote: > >>> index 9007ccd..a074d77 100644 >>> --- a/include/linux/percpu_counter.h >>> +++ b/include/linux/percpu_counter.h >>> @@ -30,8 +30,16 @@ struct percpu_counter { >>> #define FBC_BATCH (NR_CPUS*4) >>> #endif >>> >>> -int percpu_counter_init(struct percpu_counter *fbc, s64 amount); >>> -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 >>> amount); +int __percpu_counter_init(struct percpu_counter *fbc, s64 >>> amount, + struct lock_class_key *key); + >>> +#define percpu_counter_init(fbc, value) >>> \ + do { >>> \ + static struct lock_class_key __key; >>> \ + >>> \ + __percpu_counter_init(fbc, value, &__key); >>> \ + } while (0) + >>> void percpu_counter_destroy(struct percpu_counter *fbc); >>> void percpu_counter_set(struct percpu_counter *fbc, s64 amount); >>> void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, > >> This patch fails to compile: >> >> mm/backing-dev.c: In function 'bdi_init': >> mm/backing-dev.c:226: error: expected expression bedore 'do' > > Ah indeed, stupid me... > > Please try something like this instead of the above hunk: > > @@ -30,8 +30,16 @@ struct percpu_counter { > #define FBC_BATCH (NR_CPUS*4) > #endif > > -int percpu_counter_init(struct percpu_counter *fbc, s64 amount); > -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount); > +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, > + struct lock_class_key *key); > + > +#define percpu_counter_init(fbc, value) > \ + ({ > \ + static struct lock_class_key __key; > \ + > \ + __percpu_counter_init(fbc, value, &__key); > \ + }) > + > void percpu_counter_destroy(struct percpu_counter *fbc); > void percpu_counter_set(struct percpu_counter *fbc, s64 amount); > void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, > s32 batch) With this compiled, but I still get the following: [ 435.632627] ================================= [ 435.633030] [ INFO: inconsistent lock state ] [ 435.633037] 2.6.28-rc8-net-next-igbL #14 [ 435.633040] --------------------------------- [ 435.633044] inconsistent {in-softirq-W} -> {softirq-on-W} usage. [ 435.633049] netperf/12669 [HC0[0]:SC0[0]:HE1:SE1] takes: [ 435.633053] (key#8){-+..}, at: [<ffffffff803691ac>] __percpu_counter_add+0x4a/0x6d [ 435.633068] {in-softirq-W} state was registered at: [ 435.633070] [<ffffffffffffffff>] 0xffffffffffffffff [ 435.633078] irq event stamp: 988533 [ 435.633080] hardirqs last enabled at (988533): [<ffffffff80243712>] _local_bh_enable_ip+0xc8/0xcd [ 435.633088] hardirqs last disabled at (988531): [<ffffffff8024369e>] _local_bh_enable_ip+0x54/0xcd [ 435.633093] softirqs last enabled at (988532): [<ffffffff804fc814>] sock_orphan+0x3f/0x44 [ 435.633100] softirqs last disabled at (988530): [<ffffffff8056454d>] _write_lock_bh+0x11/0x3d [ 435.633107] [ 435.633108] other info that might help us debug this: [ 435.633110] 1 lock held by netperf/12669: [ 435.633112] #0: (sk_lock-AF_INET6){--..}, at: [<ffffffff804fc544>] lock_sock+0xb/0xd [ 435.633119] [ 435.633120] stack backtrace: [ 435.633124] Pid: 12669, comm: netperf Not tainted 2.6.28-rc8-net-next-igbL #14 [ 435.633127] Call Trace: [ 435.633134] [<ffffffff8025ffb8>] print_usage_bug+0x159/0x16a [ 435.633139] [<ffffffff8026000e>] valid_state+0x45/0x52 [ 435.633143] [<ffffffff802601cf>] mark_lock_irq+0x1b4/0x27b [ 435.633148] [<ffffffff80260339>] mark_lock+0xa3/0x110 [ 435.633152] [<ffffffff80260480>] mark_irqflags+0xda/0xf2 [ 435.633157] [<ffffffff8026122e>] __lock_acquire+0x1c3/0x2ee [ 435.633161] [<ffffffff80261d93>] lock_acquire+0x55/0x71 [ 435.633166] [<ffffffff803691ac>] ? __percpu_counter_add+0x4a/0x6d [ 435.633170] [<ffffffff80564434>] _spin_lock+0x2c/0x38 [ 435.633175] [<ffffffff803691ac>] ? __percpu_counter_add+0x4a/0x6d [ 435.633179] [<ffffffff803691ac>] __percpu_counter_add+0x4a/0x6d [ 435.633184] [<ffffffff804fc827>] percpu_counter_add+0xe/0x10 [ 435.633188] [<ffffffff804fc837>] percpu_counter_inc+0xe/0x10 [ 435.633193] [<ffffffff804fdc91>] tcp_close+0x157/0x2da [ 435.633197] [<ffffffff8051907e>] inet_release+0x58/0x5f [ 435.633204] [<ffffffff80527c48>] inet6_release+0x30/0x35 [ 435.633213] [<ffffffff804c9354>] sock_release+0x1a/0x76 [ 435.633221] [<ffffffff804c9804>] sock_close+0x22/0x26 [ 435.633229] [<ffffffff802a345a>] __fput+0x82/0x110 [ 435.633234] [<ffffffff802a381a>] fput+0x15/0x17 [ 435.633239] [<ffffffff802a09c5>] filp_close+0x67/0x72 [ 435.633246] [<ffffffff80240ae3>] close_files+0x66/0x8d [ 435.633251] [<ffffffff80240b39>] put_files_struct+0x19/0x42 [ 435.633256] [<ffffffff80240b98>] exit_files+0x36/0x3b [ 435.633260] [<ffffffff80241eec>] do_exit+0x1b7/0x2b1 [ 435.633265] [<ffffffff80242087>] sys_exit_group+0x0/0x14 [ 435.633269] [<ffffffff80242099>] sys_exit_group+0x12/0x14 [ 435.633275] [<ffffffff8020b9cb>] system_call_fastpath+0x16/0x1b Thanks, Emil -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h index 9007ccd..a074d77 100644 --- a/include/linux/percpu_counter.h +++ b/include/linux/percpu_counter.h @@ -30,8 +30,16 @@ struct percpu_counter { #define FBC_BATCH (NR_CPUS*4) #endif -int percpu_counter_init(struct percpu_counter *fbc, s64 amount); -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount); +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, + struct lock_class_key *key); + +#define percpu_counter_init(fbc, value) \ + do { \ + static struct lock_class_key __key; \ + \ + __percpu_counter_init(fbc, value, &__key); \ + } while (0) + void percpu_counter_destroy(struct percpu_counter *fbc); void percpu_counter_set(struct percpu_counter *fbc, s64 amount); void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch); @@ -85,8 +93,6 @@ static inline int percpu_counter_init(struct percpu_counter *fbc, s64 amount) return 0; } -#define percpu_counter_init_irq percpu_counter_init - static inline void percpu_counter_destroy(struct percpu_counter *fbc) { } diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c index b255b93..4bb0ed3 100644 --- a/lib/percpu_counter.c +++ b/lib/percpu_counter.c @@ -68,11 +68,11 @@ s64 __percpu_counter_sum(struct percpu_counter *fbc) } EXPORT_SYMBOL(__percpu_counter_sum); -static struct lock_class_key percpu_counter_irqsafe; - -int percpu_counter_init(struct percpu_counter *fbc, s64 amount) +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, + struct lock_class_key *key) { spin_lock_init(&fbc->lock); + lockdep_set_class(&fbc->lock, key); fbc->count = amount; fbc->counters = alloc_percpu(s32); if (!fbc->counters) @@ -84,17 +84,7 @@ int percpu_counter_init(struct percpu_counter *fbc, s64 amount) #endif return 0; } -EXPORT_SYMBOL(percpu_counter_init); - -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount) -{ - int err; - - err = percpu_counter_init(fbc, amount); - if (!err) - lockdep_set_class(&fbc->lock, &percpu_counter_irqsafe); - return err; -} +EXPORT_SYMBOL(__percpu_counter_init); void percpu_counter_destroy(struct percpu_counter *fbc) { diff --git a/lib/proportions.c b/lib/proportions.c index 4f387a6..7367f2b 100644 --- a/lib/proportions.c +++ b/lib/proportions.c @@ -83,11 +83,11 @@ int prop_descriptor_init(struct prop_descriptor *pd, int shift) pd->index = 0; pd->pg[0].shift = shift; mutex_init(&pd->mutex); - err = percpu_counter_init_irq(&pd->pg[0].events, 0); + err = percpu_counter_init(&pd->pg[0].events, 0); if (err) goto out; - err = percpu_counter_init_irq(&pd->pg[1].events, 0); + err = percpu_counter_init(&pd->pg[1].events, 0); if (err) percpu_counter_destroy(&pd->pg[0].events); @@ -191,7 +191,7 @@ int prop_local_init_percpu(struct prop_local_percpu *pl) spin_lock_init(&pl->lock); pl->shift = 0; pl->period = 0; - return percpu_counter_init_irq(&pl->events, 0); + return percpu_counter_init(&pl->events, 0); } void prop_local_destroy_percpu(struct prop_local_percpu *pl) diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 801c08b..a7c6c56 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -223,7 +223,7 @@ int bdi_init(struct backing_dev_info *bdi) bdi->max_prop_frac = PROP_FRAC_BASE; for (i = 0; i < NR_BDI_STAT_ITEMS; i++) { - err = percpu_counter_init_irq(&bdi->bdi_stat[i], 0); + err = percpu_counter_init(&bdi->bdi_stat[i], 0); if (err) goto err; }