From patchwork Fri Dec 26 14:08:55 2008 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Zijlstra X-Patchwork-Id: 15712 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 846DDDDE28 for ; Sat, 27 Dec 2008 01:26:06 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752304AbYLZOZ7 (ORCPT ); Fri, 26 Dec 2008 09:25:59 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752295AbYLZOZ7 (ORCPT ); Fri, 26 Dec 2008 09:25:59 -0500 Received: from viefep25-int.chello.at ([62.179.121.45]:53210 "EHLO viefep25-int.chello.at" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752265AbYLZOZ5 (ORCPT ); Fri, 26 Dec 2008 09:25:57 -0500 X-Greylist: delayed 1025 seconds by postgrey-1.27 at vger.kernel.org; Fri, 26 Dec 2008 09:25:57 EST Received: from edge04.upc.biz ([192.168.13.239]) by viefep13-int.chello.at (InterMail vM.7.09.01.00 201-2219-108-20080618) with ESMTP id <20081226140850.OGII17187.viefep13-int.chello.at@edge04.upc.biz>; Fri, 26 Dec 2008 15:08:50 +0100 Received: from [192.168.0.111] ([213.46.9.244]) by edge04.upc.biz with edge id vq8n1a01g5FupbH04q8ocG; Fri, 26 Dec 2008 15:08:50 +0100 X-SourceIP: 213.46.9.244 Subject: Re: unsafe locks seen with netperf on net-2.6.29 tree From: Peter Zijlstra To: Herbert Xu Cc: Jeff Kirsher , netdev , David Miller , Emil Tantilov , Peter P Waskiewicz Jr , Alexander Duyck , Ingo Molnar , Eric Dumazet In-Reply-To: <20081225112658.GA7260@gondor.apana.org.au> References: <9929d2390812250225w19bcd2f7n11357ff26de78c52@mail.gmail.com> <20081225112658.GA7260@gondor.apana.org.au> Date: Fri, 26 Dec 2008 15:08:55 +0100 Message-Id: <1230300535.9487.292.camel@twins> Mime-Version: 1.0 X-Mailer: Evolution 2.24.2 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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: [] > > __percpu_counter_add+0x4a/0x6d > > [ 1439.759581] > > [ 1439.759582] and this task is already holding: > > [ 1439.759853] (slock-AF_INET){-+..}, at: [] > > 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 --- 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(-) -- 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; }