ipoib lockdep warning

PATCH In review Zach Brown 8 participants 32 messages Jul 11, 2006 – Jul 17, 2006

Subsystems (18)

Networking VFS SCSI PowerPC +14 more subsystems

lore

Share & tools

lore

Zach Brown

author

2006-07-11 20:10 UTC #0 lore raw

I get an awfully verbose lockdep warning when I bring up ipoib devices and wanted to make sure it didn't go unreported. I've put it up at the following URL:

http://oss.oracle.com/~zab/ipoib-multicast-lockdep-warning.txt

I looked into it a bit and it seems to be on to something. There might be a AB, BC, CA ordering deadlock.

AB:

priv->lock is held while acquiring query_idr.lock

ipoib_mcast_send()
spin_lock(&priv->lock);
ipoib_mcast_sendonly_join()

    ib_sa_mcmember_rec_query()
      send_mad()
        idr_pre_get(&query_idr)
          spin_lock(&idp->lock);

BC:

query_idr.lock is taken with interrupts enabled and so is implicitly ordered before dev->_xmit_lock which is taken in interrupt context.

ipoib_mcast_join_task()
ipoib_mcast_join()

    ib_sa_mcmember_rec_query()
      send_mad()
        idr_pre_get(&query_idr)
          spin_lock(&idp->lock)

CA:

dev->_xmit_lock is held while acquiring priv->lock. This triggers the lockdep warning that adding the dep between dev->_xmit_lock and priv->lock connects a soft-irq-safe lock to a soft-irq-unsafe one.

ipoib_mcast_restart_task()
local_irq_save(flags);
netif_tx_lock(dev)

    spin_lock(&dev->_xmit_lock);

spin_lock(&priv->lock);

I can imagine all sorts of potential fixes (block ints when calling idr?
reorder acquiry in ipoib_mcast_restart_task()?) but I'm operating on a
partial view of the paths here so I wasn't comfortable suggesting a fix.
I wouldn't be surprised to hear that there are circumstances that both
lockdep and I don't know about that stop this from being a problem :).

In any case, it'd be fantastic if someone who knows this code could sit down with lockdep and some ipoib regression suite to shake out lockdep's complaints.

- z

Michael S. Tsirkin

2006-07-11 21:15 UTC #1 lore raw

Quoting r. Zach Brown <[email protected]>:

Quoted 11 lines

BC:
query_idr.lock is taken with interrupts enabled and so is implicitly ordered before dev->_xmit_lock which is taken in interrupt context.
ipoib_mcast_join_task()
ipoib_mcast_join()
    ib_sa_mcmember_rec_query()
      send_mad()
        idr_pre_get(&query_idr)
          spin_lock(&idp->lock)

Got to check, but if that's true we have a simple deadlock here:
ib_sa_mcmember_rec_query might get called from interrupt context as well, deadlocking on idp->lock?

Sean?

Quoted 5 lines

I can imagine all sorts of potential fixes (block ints when calling idr?
reorder acquiry in ipoib_mcast_restart_task()?) but I'm operating on a
partial view of the paths here so I wasn't comfortable suggesting a fix.
I wouldn't be surprised to hear that there are circumstances that both
lockdep and I don't know about that stop this from being a problem :).

Awesome, thanks for the analysis! Your help is very much appreciated.

Signature · 1 line

MST

Sean Hefty

2006-07-11 21:40 UTC #2 lore raw

Quoted 13 lines

Quoting r. Zach Brown <[email protected]>:
Quoted 11 lines
BC:
query_idr.lock is taken with interrupts enabled and so is implicitly ordered before dev->_xmit_lock which is taken in interrupt context.
ipoib_mcast_join_task()
ipoib_mcast_join()
    ib_sa_mcmember_rec_query()
      send_mad()
        idr_pre_get(&query_idr)
          spin_lock(&idp->lock)

Quoted 5 lines

Got to check, but if that's true we have a simple deadlock here:
ib_sa_mcmember_rec_query might get called from interrupt context as well, deadlocking on idp->lock?
Sean?

As a side note, I believe that this is the upstream code and does not include the latest multicast changes.

I'm not sure if anything calls into the sa_query interfaces from interrupt context, but I doubt it. From my brief look at the initially reported issue, I can't determine if there's an actual problem without studying the ipoib code more.

- Sean

Zach Brown

author

2006-07-11 21:50 UTC #3 lore raw

Quoted 3 lines

As a side note, I believe that this is the upstream code and does not include the latest multicast changes.

Indeed, I should have mentioned in my report that I was running 2.6.17-mm6.

- z

Roland Dreier

2006-07-11 22:54 UTC #4 lore raw

In reply to Zach Brown

No time to really look at this in detail, but I think the issue is a slightly bogus conversion to netif_tx_lock(). Can you try this patch and see if things are better?

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +2 -4

index ab40488..ddd1946 100644
@@ -820,9 +820,8 @@ void ipoib_mcast_restart_task(void *dev_
 
 	ipoib_mcast_stop_thread(dev, 0);
 
-	local_irq_save(flags);
 	netif_tx_lock(dev);
-	spin_lock(&priv->lock);
+	spin_lock_irqsave(&priv->lock, flags);
 
 	/*
 	 * Unfortunately, the networking core only gives us a list of all of

@@ -893,9 +892,8 @@ void ipoib_mcast_restart_task(void *dev_
 		}
 	}
 
-	spin_unlock(&priv->lock);
+	spin_unlock_irqrestore(&priv->lock, flags);
 	netif_tx_unlock(dev);
-	local_irq_restore(flags);
 
 	/* We have to cancel outside of the spinlock */
 	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {

Zach Brown

author

2006-07-11 23:27 UTC #5 lore raw

drivers/infiniband/ulp/ipoib/ipoib_multicast.c void ipoib_mcast_restart_task(void *dev_ · line 824

Quoted 8 lines · Roland Dreier

No time to really look at this in detail, but I think the issue is a slightly bogus conversion to netif_tx_lock(). Can you try this patch and see if things are better?
drivers/infiniband/ulp/ipoib/ipoib_multicast.c +1 -2
-	local_irq_save(flags);
 	netif_tx_lock(dev);
-	spin_lock(&priv->lock);
+	spin_lock_irqsave(&priv->lock, flags);

Hmm, won't that hold dev->_xmit_lock with interrupts enabled? That seems like it'd deadlock with dev_watchdog grabbing it from a softirq?

But setting that aside, I don't think this will help as it doesn't change the order that the locks are acquired in this path. This can still be racing with other cpus that are each in the other two paths (queue_idr.lock -> softirq -> dev->_xmit_lock, priv->lock -> queue_idr.lock). The local state of interrupt masking on this cpu doesn't stop the other cpus from each grabbing the first lock in their path and then trying to grab the second. Imagine that they all race to grab the first (A, B, C) and succeed and then all get stuck spinning on their second lock (B, C, A).

Maybe you could get the priv->lock here before dev->_xmit_lock. Then we'd have AB, BC, AC, and that's OK. I'm going to guess that this won't work because other paths have dev->_xmit_lock -> priv->lock ordering.

Another possibility is masking interrupts when getting queue_idr.lock. That drops the implicit dependency between queue_idr and _xmit_lock and gives us AB, B, CA -- which is fine. That means blocking ints while in idr_pre_get() which allocs which leads to GFP_ATOMIC and more likely allocation failure.

That's my reading, anyway.

- z

Roland Dreier

2006-07-11 23:43 UTC #6 lore raw

Hmm, good point.

It sort of seems to me like the idr interfaces are broken by design. Internally, lib/idr.c uses bare spin_lock(&idp->lock) with no interrupt disabling or anything in both the idr_pre_get() and idr_get_new() code paths. But idr_pre_get() is supposed to be called in a context that can sleep, while idr_get_new() is supposed to be called with locks held to serialize things (at least according to
http://lwn.net/Articles/103209/).

So, ugh... maybe the best thing to do is change lib/idr.c to use spin_lock_irqsave() internally?

- R.

Zach Brown

author

2006-07-11 23:53 UTC #7 lore raw

Quoted 6 lines · Roland Dreier

Hmm, good point.
It sort of seems to me like the idr interfaces are broken by design. Internally, lib/idr.c uses bare spin_lock(&idp->lock) with no interrupt disabling or anything in both the idr_pre_get() and idr_get_new() code paths.

I wasn't thrilled to see that either. We seem to have a fair precedent (list.h, rbtree, etc) for leaving serialization to callers.

Quoted 2 lines

So, ugh... maybe the best thing to do is change lib/idr.c to use spin_lock_irqsave() internally?

I dunno, it seems to have had _irq() locking in the past? From the comment at the top:

* Modified by George Anzinger to reuse immediately and to use
* find bit instructions. Also removed _irq on spinlocks.

- z

Roland Dreier

2006-07-12 00:06 UTC #8 lore raw

Quoted 8 lines

Quoted 2 lines
So, ugh... maybe the best thing to do is change lib/idr.c to use spin_lock_irqsave() internally?
I dunno, it seems to have had _irq() locking in the past? From the comment at the top:
* Modified by George Anzinger to reuse immediately and to use
* find bit instructions. Also removed _irq on spinlocks.

Well, _irq would be no good, because we might want to call idr stuff with interrupts disabled. But making idr internally _irqsave seems like the right fix to me.

I think the real issue here is that the sa_query.c stuff wants to use the idr mechanism to assign "query ids", and other modules want to be able to start queries from any context. So if idr uses bare spin_lock internally, then sa_query.c has no choice but to wrap all idr calls inside spin_lock_irqsave and do all allocation with GFP_ATOMIC, which doesn't seem very nice.

- R.

Ingo Molnar

2006-07-12 09:44 UTC #9 lore raw

In reply to Roland Dreier

Quoted 3 lines · Roland Dreier

Hmm, good point.
It sort of seems to me like the idr interfaces are broken by design.

[...]

Quoted 2 lines

So, ugh... maybe the best thing to do is change lib/idr.c to use spin_lock_irqsave() internally?

i agree that the IDR subsystem should be irq-safe if GFP_ATOMIC is passed in. So the _irqsave()/_irqrestore() fix should be done.

But i also think that you should avoid using GFP_ATOMIC for any sort of reliable IO path and push as much work into process context as possible. Is it acceptable for your infiniband IO model to fail with -ENOMEM if GFP_ATOMIC happens to fail, and is the IO retried transparently?

Ingo

Michael S. Tsirkin

2006-07-12 11:09 UTC #10 lore raw

Quoting r. Ingo Molnar <[email protected]>:

Quoted 4 lines

But i also think that you should avoid using GFP_ATOMIC for any sort of reliable IO path and push as much work into process context as possible. Is it acceptable for your infiniband IO model to fail with -ENOMEM if GFP_ATOMIC happens to fail, and is the IO retried transparently?

Yes, this is true for users that pass GFP_ATOMIC to sa_query, at least. But might not be so for other users: send_mad in sa_query actually gets gfp_flags parameter, but for some reason it does not pass it to idr_pre_get, which means even sa query done with GFP_KERNEL flag is likely to fail.

Sean, it seems we need something like the following - what do you think?

Avoid bogus out out memory errors: fix sa_query to actually pass gfp_mask supplied by the user to idr_pre_get.

Signed-off-by Michael S. Tsirkin <[email protected]>

drivers/infiniband/core/sa_query.c +5 -5

index e911c99..aeda484 100644
@@ -488,13 +488,13 @@ static void init_mad(struct ib_sa_mad *m
 	spin_unlock_irqrestore(&tid_lock, flags);
 }
 
-static int send_mad(struct ib_sa_query *query, int timeout_ms)
+static int send_mad(struct ib_sa_query *query, int timeout_ms, gfp_t gfp_mask)
 {
 	unsigned long flags;
 	int ret, id;
 
 retry:
-	if (!idr_pre_get(&query_idr, GFP_ATOMIC))
+	if (!idr_pre_get(&query_idr, gfp_mask))
 		return -ENOMEM;
 	spin_lock_irqsave(&idr_lock, flags);
 	ret = idr_get_new(&query_idr, query, &id);

@@ -630,7 +630,7 @@ int ib_sa_path_rec_get(struct ib_device 
 
 	*sa_query = &query->sa_query;
 
-	ret = send_mad(&query->sa_query, timeout_ms);
+	ret = send_mad(&query->sa_query, timeout_ms, gfp_mask);
 	if (ret < 0)
 		goto err2;

@@ -752,7 +752,7 @@ int ib_sa_service_rec_query(struct ib_de
 
 	*sa_query = &query->sa_query;
 
-	ret = send_mad(&query->sa_query, timeout_ms);
+	ret = send_mad(&query->sa_query, timeout_ms, gfp_mask);
 	if (ret < 0)
 		goto err2;

@@ -844,7 +844,7 @@ int ib_sa_mcmember_rec_query(struct ib_d
 
 	*sa_query = &query->sa_query;
 
-	ret = send_mad(&query->sa_query, timeout_ms);
+	ret = send_mad(&query->sa_query, timeout_ms, gfp_mask);
 	if (ret < 0)
 		goto err2;

Signature · 1 line

MST

Sean Hefty

2006-07-12 16:33 UTC #11 lore raw

Quoted 6 lines · Michael S. Tsirkin

Yes, this is true for users that pass GFP_ATOMIC to sa_query, at least. But might not be so for other users: send_mad in sa_query actually gets gfp_flags parameter, but for some reason it does not pass it to idr_pre_get, which means even sa query done with GFP_KERNEL flag is likely to fail.
Sean, it seems we need something like the following - what do you think?

I noticed this same thing looking at the code yesterday. I can't think of any reason why your patch wouldn't work.

- Sean

Roland Dreier

2006-07-12 18:56 UTC #12 lore raw

In reply to Michael S. Tsirkin

Quoted 2 lines

Avoid bogus out out memory errors: fix sa_query to actually pass gfp_mask supplied by the user to idr_pre_get.

Yes, this looks right to me.

- R.

Michael S. Tsirkin

2006-07-13 15:54 UTC #13 lore raw

PATCH 1/1 IB/core: use correct gfp_mask in sa_query

In reply to Michael S. Tsirkin

Andrew, could you please drop the following in -mm and on to Linus?

Avoid bogus out of memory errors: fix sa_query to actually pass gfp_mask supplied by the user to idr_pre_get.

Signed-off-by Michael S. Tsirkin <[email protected]>

Acked-by "Sean Hefty" <[email protected]>

Acked-by "Roland Dreier" <[email protected]>

drivers/infiniband/core/sa_query.c +5 -5

index e911c99..aeda484 100644
@@ -488,13 +488,13 @@ static void init_mad(struct ib_sa_mad *m
 	spin_unlock_irqrestore(&tid_lock, flags);
 }
 
-static int send_mad(struct ib_sa_query *query, int timeout_ms)
+static int send_mad(struct ib_sa_query *query, int timeout_ms, gfp_t gfp_mask)
 {
 	unsigned long flags;
 	int ret, id;
 
 retry:
-	if (!idr_pre_get(&query_idr, GFP_ATOMIC))
+	if (!idr_pre_get(&query_idr, gfp_mask))
 		return -ENOMEM;
 	spin_lock_irqsave(&idr_lock, flags);
 	ret = idr_get_new(&query_idr, query, &id);

@@ -630,7 +630,7 @@ int ib_sa_path_rec_get(struct ib_device 
 
 	*sa_query = &query->sa_query;
 
-	ret = send_mad(&query->sa_query, timeout_ms);
+	ret = send_mad(&query->sa_query, timeout_ms, gfp_mask);
 	if (ret < 0)
 		goto err2;

@@ -752,7 +752,7 @@ int ib_sa_service_rec_query(struct ib_de
 
 	*sa_query = &query->sa_query;
 
-	ret = send_mad(&query->sa_query, timeout_ms);
+	ret = send_mad(&query->sa_query, timeout_ms, gfp_mask);
 	if (ret < 0)
 		goto err2;

@@ -844,7 +844,7 @@ int ib_sa_mcmember_rec_query(struct ib_d
 
 	*sa_query = &query->sa_query;
 
-	ret = send_mad(&query->sa_query, timeout_ms);
+	ret = send_mad(&query->sa_query, timeout_ms, gfp_mask);
 	if (ret < 0)
 		goto err2;

Signature · 1 line

MST

Roland Dreier

2006-07-12 19:06 UTC #14 lore raw

In reply to Ingo Molnar

Quoted 2 lines

i agree that the IDR subsystem should be irq-safe if GFP_ATOMIC is passed in. So the _irqsave()/_irqrestore() fix should be done.

OK, I will send the idr change to Andrew.

Quoted 4 lines

But i also think that you should avoid using GFP_ATOMIC for any sort of reliable IO path and push as much work into process context as possible. Is it acceptable for your infiniband IO model to fail with -ENOMEM if GFP_ATOMIC happens to fail, and is the IO retried transparently?

Yes, I think it's OK. This idr use is in an inherently unreliable path. With that said, as Michael pointed out, we can change things to use GFP_ATOMIC less.

Thanks,
Roland

Roland Dreier

2006-07-12 20:45 UTC #15 lore raw

PATCH 1/1 Convert idr's internal locking to _irqsave variant

In reply to Ingo Molnar

Currently, the code in lib/idr.c uses a bare spin_lock(&idp->lock) to do internal locking. This is a nasty trap for code that might call idr functions from different contexts; for example, it seems perfectly reasonable to call idr_get_new() from process context and idr_remove() from interrupt context -- but with the current locking this would lead to a potential deadlock.

The simplest fix for this is to just convert the idr locking to use spin_lock_irqsave().

In particular, this fixes a very complicated locking issue detected by lockdep, involving the ib_ipoib driver's priv->lock and dev->_xmit_lock, which get involved with the ib_sa module's query_idr.lock.

Cc Arjan van de Ven <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Zach Brown <[email protected]>,

Signed-off-by Roland Dreier <[email protected]>

---

lib/idr.c +10 -6

index 4d09681..16d2143 100644
@@ -38,14 +38,15 @@ static kmem_cache_t *idr_layer_cache;
 static struct idr_layer *alloc_layer(struct idr *idp)
 {
 	struct idr_layer *p;
+	unsigned long flags;
 
-	spin_lock(&idp->lock);
+	spin_lock_irqsave(&idp->lock, flags);
 	if ((p = idp->id_free)) {
 		idp->id_free = p->ary[0];
 		idp->id_free_cnt--;
 		p->ary[0] = NULL;
 	}
-	spin_unlock(&idp->lock);
+	spin_unlock_irqrestore(&idp->lock, flags);
 	return(p);
 }

@@ -59,12 +60,14 @@ static void __free_layer(struct idr *idp
 
 static void free_layer(struct idr *idp, struct idr_layer *p)
 {
+	unsigned long flags;
+
 	/*
 	 * Depends on the return element being zeroed.
 	 */
-	spin_lock(&idp->lock);
+	spin_lock_irqsave(&idp->lock, flags);
 	__free_layer(idp, p);
-	spin_unlock(&idp->lock);
+	spin_unlock_irqrestore(&idp->lock, flags);
 }
 
 /**

@@ -168,6 +171,7 @@ static int idr_get_new_above_int(struct 
 {
 	struct idr_layer *p, *new;
 	int layers, v, id;
+	unsigned long flags;
 
 	id = starting_id;
 build_up:

@@ -191,14 +195,14 @@ build_up:
 			 * The allocation failed.  If we built part of
 			 * the structure tear it down.
 			 */
-			spin_lock(&idp->lock);
+			spin_lock_irqsave(&idp->lock, flags);
 			for (new = p; p && p != idp->top; new = p) {
 				p = p->ary[0];
 				new->ary[0] = NULL;
 				new->bitmap = new->count = 0;
 				__free_layer(idp, new);
 			}
-			spin_unlock(&idp->lock);
+			spin_unlock_irqrestore(&idp->lock, flags);
 			return -1;
 		}
 		new->ary[0] = p;

Ingo Molnar

Acked-by on v1 1/1

2006-07-12 21:20 UTC #16 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Quoted 10 lines · Roland Dreier

Currently, the code in lib/idr.c uses a bare spin_lock(&idp->lock) to do internal locking. This is a nasty trap for code that might call idr functions from different contexts; for example, it seems perfectly reasonable to call idr_get_new() from process context and idr_remove() from interrupt context -- but with the current locking this would lead to a potential deadlock.
The simplest fix for this is to just convert the idr locking to use spin_lock_irqsave().

Quoted 7 lines

In particular, this fixes a very complicated locking issue detected by lockdep, involving the ib_ipoib driver's priv->lock and dev->_xmit_lock, which get involved with the ib_sa module's query_idr.lock.
Cc Arjan van de Ven <[email protected]>
Cc Ingo Molnar <[email protected]>

Acked-by Ingo Molnar <[email protected]>

Ingo

Andrew Morton

2006-07-13 01:34 UTC #17 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

In reply to Roland Dreier

Quoted 7 lines · On Wed, 12 Jul 2006 13:45:12 -0700 Roland Dreier <[email protected]> wrote:

Currently, the code in lib/idr.c uses a bare spin_lock(&idp->lock) to do internal locking. This is a nasty trap for code that might call idr functions from different contexts; for example, it seems perfectly reasonable to call idr_get_new() from process context and idr_remove() from interrupt context -- but with the current locking this would lead to a potential deadlock.

Quoted 6 lines

The simplest fix for this is to just convert the idr locking to use spin_lock_irqsave().
In particular, this fixes a very complicated locking issue detected by lockdep, involving the ib_ipoib driver's priv->lock and dev->_xmit_lock, which get involved with the ib_sa module's query_idr.lock.

Sigh. It was always a mistake (of the kernel programming 101 type) to put any locking at all in the idr code. At some stage we need to weed it all out and move it to callers.

Your fix is yet more fallout from that mistake.

Roland Dreier

2006-07-13 15:42 UTC #18 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Quoted 5 lines

Sigh. It was always a mistake (of the kernel programming 101 type) to put any locking at all in the idr code. At some stage we need to weed it all out and move it to callers.
Your fix is yet more fallout from that mistake.

Agreed. Consider me on the hook to fix this up in a better way once my life is a little saner. Maybe I'll try to cook something up on the plane ride to Ottawa.

Anyway you can punch me in the stomach if I don't have something in time for 2.6.19.

- R.

Andrew Morton

2006-07-13 20:58 UTC #19 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Quoted 10 lines · On Thu, 13 Jul 2006 08:42:47 -0700 Roland Dreier <[email protected]> wrote:

Quoted 5 lines
Sigh. It was always a mistake (of the kernel programming 101 type) to put any locking at all in the idr code. At some stage we need to weed it all out and move it to callers.
Your fix is yet more fallout from that mistake.
Agreed. Consider me on the hook to fix this up in a better way once my life is a little saner. Maybe I'll try to cook something up on the plane ride to Ottawa.

I suspect it'll get really ugly. It's a container library which needs to allocate memory when items are added, like the radix-tree. Either it needs to assume GFP_ATOMIC, which is bad and can easily fail or it does weird things like radix_tree_preload().

The basic problem is:

	idr_pre_get(GFP_KERNEL);
	spin_lock(my_lock);
	idr_get_new(..);

which is racy, because some other CPU could have got in there and consumed some of the pool which was precharged by idr_pre_get().

It's wildly improbable that it'll actually fail. It requires all of:

a) that the race occur

b) that the racing thread consume an appreciable amount of the pool

c) that this thread also consume an appreciable amount (such that the total of both exceeds the pool size).

d) that a (needs to be added) GFP_ATOMIC attempt to replenish the pool inside idr_get_new() fails.

Roland Dreier

2006-07-13 21:03 UTC #20 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Quoted 4 lines

I suspect it'll get really ugly. It's a container library which needs to allocate memory when items are added, like the radix-tree. Either it needs to assume GFP_ATOMIC, which is bad and can easily fail or it does weird things like radix_tree_preload().

Actually I don't think it has to be too bad. We could tweak the interface a little bit so that consumers do something like:

struct idr_layer *layer = NULL; /* opaque */

retry:

        spin_lock(&my_idr_lock);
	ret = idr_get_new(&my_idr, ptr, &id, layer);
        spin_unlock(&my_idr_lock);

        if (ret == -EAGAIN) {
		layer = idr_alloc_layer(&my_idr, GFP_KERNEL);
		if (!IS_ERR(layer))
			goto retry;
	}

in other words make the consumer responsible for passing in new memory that can be used for a new entry (or freed if other entries have become free in the meantime).

- R.

Arjan van de Ven

2006-07-13 21:05 UTC #21 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Quoted 9 lines · Roland Dreier

Quoted 4 lines
I suspect it'll get really ugly. It's a container library which needs to allocate memory when items are added, like the radix-tree. Either it needs to assume GFP_ATOMIC, which is bad and can easily fail or it does weird things like radix_tree_preload().
Actually I don't think it has to be too bad. We could tweak the interface a little bit so that consumers do something like:

it does get harder if this is needed for your IB device to do more work, so that your swap device on your IB can take more IO's to free up ram..

Roland Dreier

2006-07-14 00:18 UTC #22 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Arjan> it does get harder if this is needed for your IB device to Arjan> do more work, so that your swap device on your IB can take Arjan> more IO's to free up ram..

That's the classic problem, but it's more a matter of the consumer using GFP_NOIO in the right places.

- R.

Arjan van de Ven

2006-07-14 06:20 UTC #23 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Quoted 6 lines · Roland Dreier

Arjan> it does get harder if this is needed for your IB device to Arjan> do more work, so that your swap device on your IB can take Arjan> more IO's to free up ram..
That's the classic problem, but it's more a matter of the consumer using GFP_NOIO in the right places.

GFP_NOIO isn't going to save you in the cases where the memory really is running low and you need the memory to do more IO...

Andrew Morton

2006-07-13 21:47 UTC #24 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

In reply to Roland Dreier

Quoted 21 lines · On Thu, 13 Jul 2006 14:03:21 -0700 Roland Dreier <[email protected]> wrote:

Quoted 4 lines
I suspect it'll get really ugly. It's a container library which needs to allocate memory when items are added, like the radix-tree. Either it needs to assume GFP_ATOMIC, which is bad and can easily fail or it does weird things like radix_tree_preload().
Actually I don't think it has to be too bad. We could tweak the interface a little bit so that consumers do something like:
struct idr_layer *layer = NULL; /* opaque */
retry:
        spin_lock(&my_idr_lock);
	ret = idr_get_new(&my_idr, ptr, &id, layer);
        spin_unlock(&my_idr_lock);

        if (ret == -EAGAIN) {
		layer = idr_alloc_layer(&my_idr, GFP_KERNEL);
		if (!IS_ERR(layer))
			goto retry;
	}

Quoted 4 lines

in other words make the consumer responsible for passing in new memory that can be used for a new entry (or freed if other entries have become free in the meantime).

Good point, a try-again loop would work. Do we really need the caller to maintain a cache? I suspect something like

drat:

	if (idr_pre_get(GFP_KERNEL) == ENOMEM)
		give_up();
	spin_lock();
	ret = idr_get_new();
	spin_unlock();
	if (ret == ENOMEM)
		goto drat;

would do it.

Roland Dreier

2006-07-14 01:11 UTC #25 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Quoted 12 lines

Good point, a try-again loop would work. Do we really need the caller to maintain a cache? I suspect something like
drat:
	if (idr_pre_get(GFP_KERNEL) == ENOMEM)
		give_up();
	spin_lock();
	ret = idr_get_new();
	spin_unlock();
	if (ret == ENOMEM)
		goto drat;

Quoted 1 line

would do it.

The problem (for my tiny brain at least) is that I don't know where idr_pre_get() can put the memory it allocates if there's no lock in the idr structure -- how do you maintain internal consistency if no locks are held when filling the cache?

Having the caller hold a chunk of memory in a stack variable was the trick I came up with to get around that.

- R.

Andrew Morton

2006-07-14 01:22 UTC #26 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Quoted 12 lines · On Thu, 13 Jul 2006 18:08:17 -0700 Roland Dreier <[email protected]> wrote:

Quoted 12 lines
Good point, a try-again loop would work. Do we really need the caller to maintain a cache? I suspect something like
drat:
	if (idr_pre_get(GFP_KERNEL) == ENOMEM)
		give_up();
	spin_lock();
	ret = idr_get_new();
	spin_unlock();
	if (ret == ENOMEM)
		goto drat;

Quoted 6 lines

Quoted 1 line
would do it.
The problem (for my tiny brain at least) is that I don't know where idr_pre_get() can put the memory it allocates if there's no lock in the idr structure -- how do you maintain internal consistency if no locks are held when filling the cache?

argh. Aren't you supposed to be on vacation or something?

Quoted 2 lines

Having the caller hold a chunk of memory in a stack variable was the trick I came up with to get around that.

Yes, that certainly works.

Andrew Morton

2006-07-14 01:34 UTC #27 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Quoted 4 lines · On Thu, 13 Jul 2006 18:18:35 -0700 Andrew Morton <[email protected]> wrote:

Quoted 2 lines
Having the caller hold a chunk of memory in a stack variable was the trick I came up with to get around that.
Yes, that certainly works.

Problem is, I think, you'll need to preallocate IDR_FREE_MAX items. And then free them all again when none of them were consumed (usual).

Yes, storing the preallocated nodes in the idr itself requires locking. But that locking is 100% private to the IDR implementation. It locks only the preload list and not the user's stuff.

radix_tree_preload() effectively does this. Except the preload list is kernel-wide. It's split across CPUs and uses local_irq_disable/preempt_disable locking tricks as a performance optimisation. But conceptually it's the same.

Simply copying that would give something which is known to work... It seems like a large amount of fuss, but when you think about it the problem isn't simple.

Roland Dreier

2006-07-17 15:57 UTC #28 lore raw

Re: [PATCH] Convert idr's internal locking to _irqsave variant

Quoted 2 lines

Problem is, I think, you'll need to preallocate IDR_FREE_MAX items. And then free them all again when none of them were consumed (usual).

Actually I think it's OK if we just pass in no more than one extra layer for each try to add something with idr_get_new(). In the worst case, this leads to a lot of extra calls to idr_get_new(), but in the usual case it's fine.

I'm including a lightly tested big patch with all my idr changes for comments -- I'll split it up into a form more suitable for merging. (Some of the changes are unrelated and obviously good, eg using kmem_cache_zalloc() instead of a slab cache with a constructor that does memset(0)).

I'm not sure I'm thrilled with this approach, but it does seem to be a net win. With an allyesconfig with debugging options turned off (so spinlocks shrink back down to 8 bytes), I get the following:

text data bss dec hex filename
24347759 5971210 2322176 32641145 1f21079 vmlinux.old
24347370 5970474 2320704 32638548 1f20654 vmlinux.new

Most of the savings comes from ocfs2, which has a static array of 255 structures that each contain an idr -- so removing the lock from struct idr saves 255 * 8 = 2040 bytes. However, even without factoring that in, this does seem to be a net win:

add/remove: 2/4 grow/shrink: 23/51 up/down: 719/-3215 (-2496)
function old new delta
idr_get_new_above 38 554 +516

dm_create                                    957    1000     +43
ipath_init_one                              3294    3329     +35
get_layer                                      -      32     +32
idr_alloc_layer                                -      16     +16
sd_probe                                     871     881     +10
rtc_device_register                          493     503     +10
proc_register                                277     286      +9
mmc_add_host_sysfs                           126     135      +9
idr_add_uobj                                  80      85      +5
cma_alloc_port                               224     229      +5
sys_timer_create                             876     880      +4
set_anon_super                               173     177      +4
sctp_process_init                           1312    1316      +4
ib_ucm_ctx_alloc                             197     201      +4
ib_create_cm_id                              287     290      +3
proc_mkdir_mode                               95      97      +2
vfs_kern_mount                               279     280      +1
send_mad                                     325     326      +1
proc_symlink                                 141     142      +1
proc_file_write                               40      41      +1
kill_block_super                              56      57      +1
get_sb_single                                175     176      +1
free_proc_entry                              108     109      +1
deactivate_super                             126     127      +1
proc_readdir                                 353     352      -1
proc_getattr                                  40      39      -1
get_sb_nodev                                 150     149      -1
o2net_send_message_vec                      2032    2030      -2
hwmon_device_register                        198     196      -2
create_proc_entry                            170     168      -2
__put_super_and_need_restart                  54      52      -2
v9fs_read_work                              1424    1421      -3
v9fs_mux_init                               1333    1330      -3
v9fs_mux_flush_cb                            303     300      -3
v9fs_mux_destroy                             369     366      -3
v9fs_mux_cancel                              382     379      -3
o2net_init                                   441     438      -3
inotify_add_watch                            285     280      -5
v9fs_session_init                           1490    1484      -6
unnamed_dev_idr                               32      24      -8
unit_table                                    32      24      -8
tcp_ps                                        32      24      -8
sdp_ps                                        32      24      -8
sd_index_idr                                  32      24      -8
sctp_assocs_id                                32      24      -8
rtc_idr                                       32      24      -8
query_idr                                     32      24      -8
proc_inum_idr                                 32      24      -8
posix_timers_id                               32      24      -8
mmc_host_idr                                  32      24      -8
lpfc_hba_index                                32      24      -8
ib_uverbs_srq_idr                             32      24      -8
ib_uverbs_qp_idr                              32      24      -8
ib_uverbs_pd_idr                              32      24      -8
ib_uverbs_mw_idr                              32      24      -8
ib_uverbs_mr_idr                              32      24      -8
ib_uverbs_cq_idr                              32      24      -8
ib_uverbs_ah_idr                              32      24      -8
i2c_adapter_idr                               32      24      -8
hwmon_idr                                     32      24      -8
ctx_id_table                                  32      24      -8
cm                                           112     104      -8
allocated_ptys                                32      24      -8
_minor_idr                                    32      24      -8
i2c_add_adapter                              494     484     -10
idr_cache_ctor                                12       -     -12
v9fs_get_idpool                              149     134     -15
ib_cm_init                                   237     220     -17
free_layer                                    55      34     -21
idr_init                                      88      62     -26
idr_get_new                                   40      13     -27
idr_remove                                   357     328     -29
infinipath_init                              227     197     -30
lpfc_pci_probe_one                          2752    2710     -42
ptmx_open                                    427     379     -48
idr_pre_get                                   59       -     -59
alloc_layer                                   66       -     -66

idr_get_new_above_int 533 - -533
o2net_nodes 99960 97920 -2040

---

arch/powerpc/mm/mmu_context_64.c +7 -6

index 90a06ac..85d6a03 100644
@@ -26,20 +26,21 @@ static DEFINE_IDR(mmu_context_idr);
 
 int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
 {
+	struct idr_layer *layer = NULL;
 	int index;
 	int err;
 
 again:
-	if (!idr_pre_get(&mmu_context_idr, GFP_KERNEL))
-		return -ENOMEM;
-
 	spin_lock(&mmu_context_lock);
-	err = idr_get_new_above(&mmu_context_idr, NULL, 1, &index);
+	err = idr_get_new_above(&mmu_context_idr, NULL, 1, &index, layer);
 	spin_unlock(&mmu_context_lock);
 
-	if (err == -EAGAIN)
+	if (err == -EAGAIN) {
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer)
+			return -ENOMEM;
 		goto again;
-	else if (err)
+	} else if (err)
 		return err;
 
 	if (index > MAX_CONTEXT) {

drivers/char/tty_io.c +20 ~~-16~~

index bfdb902..0ae099e 100644
@@ -2135,6 +2135,7 @@ #ifdef CONFIG_UNIX98_PTYS
 static int ptmx_open(struct inode * inode, struct file * filp)
 {
 	struct tty_struct *tty;
+	struct idr_layer *layer = NULL;
 	int retval;
 	int index;
 	int idr_ret;

@@ -2143,24 +2144,27 @@ static int ptmx_open(struct inode * inod
 
 	/* find a device that is not in use. */
 	down(&allocated_ptys_lock);
-	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL)) {
-		up(&allocated_ptys_lock);
-		return -ENOMEM;
-	}
-	idr_ret = idr_get_new(&allocated_ptys, NULL, &index);
-	if (idr_ret < 0) {
-		up(&allocated_ptys_lock);
-		if (idr_ret == -EAGAIN)
-			return -ENOMEM;
-		return -EIO;
-	}
-	if (index >= pty_limit) {
-		idr_remove(&allocated_ptys, index);
-		up(&allocated_ptys_lock);
-		return -EIO;
-	}
+	do {
+		idr_ret = idr_get_new(&allocated_ptys, NULL, &index, layer);
+		if (idr_ret == -EAGAIN) {
+			layer = idr_alloc_layer(GFP_KERNEL);
+			if (!layer) {
+				idr_ret = -ENOMEM;
+				break;
+			}
+			continue;
+		}
+
+		if (index >= pty_limit) {
+			idr_remove(&allocated_ptys, index);
+			idr_ret = -EIO;
+		}
+	} while (idr_ret == -EAGAIN);
 	up(&allocated_ptys_lock);
 
+	if (idr_ret)
+		return idr_ret == -EAGAIN ? -ENOMEM : -EIO;
+
 	mutex_lock(&tty_mutex);
 	retval = init_dev(ptm_driver, index, &tty);
 	mutex_unlock(&tty_mutex);

drivers/hwmon/hwmon.c +7 -6

index 106fa01..82d6d04 100644
@@ -38,20 +38,21 @@ static DEFINE_SPINLOCK(idr_lock);
  */
 struct class_device *hwmon_device_register(struct device *dev)
 {
+	struct idr_layer *layer = NULL;
 	struct class_device *cdev;
 	int id, err;
 
 again:
-	if (unlikely(idr_pre_get(&hwmon_idr, GFP_KERNEL) == 0))
-		return ERR_PTR(-ENOMEM);
-
 	spin_lock(&idr_lock);
-	err = idr_get_new(&hwmon_idr, NULL, &id);
+	err = idr_get_new(&hwmon_idr, NULL, &id, layer);
 	spin_unlock(&idr_lock);
 
-	if (unlikely(err == -EAGAIN))
+	if (unlikely(err == -EAGAIN)) {
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer)
+			return ERR_PTR(-ENOMEM);
 		goto again;
-	else if (unlikely(err))
+	} else if (unlikely(err))
 		return ERR_PTR(err);
 
 	id = id & MAX_ID_MASK;

drivers/i2c/i2c-core.c +11 -9

index 9cb277d..f8aa8ea 100644
@@ -151,22 +151,24 @@ static struct device_attribute dev_attr_
 int i2c_add_adapter(struct i2c_adapter *adap)
 {
 	int id, res = 0;
+	struct idr_layer   *layer = NULL;
 	struct list_head   *item;
 	struct i2c_driver  *driver;
 
 	mutex_lock(&core_lists);
 
-	if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0) {
-		res = -ENOMEM;
-		goto out_unlock;
-	}
-
-	res = idr_get_new(&i2c_adapter_idr, adap, &id);
-	if (res < 0) {
-		if (res == -EAGAIN)
+again:
+	res = idr_get_new(&i2c_adapter_idr, adap, &id, layer);
+	if (res == -EAGAIN) {
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer) {
 			res = -ENOMEM;
-		goto out_unlock;
+			goto out_unlock;
+		}
+		goto again;
 	}
+	if (res < 0)
+		goto out_unlock;
 
 	adap->nr =  id & MAX_ID_MASK;
 	mutex_init(&adap->bus_lock);

drivers/infiniband/core/cm.c +9 -3

index f85c97f..cf14d01 100644
@@ -298,6 +298,7 @@ static int cm_init_av_by_path(struct ib_
 
 static int cm_alloc_id(struct cm_id_private *cm_id_priv)
 {
+	struct idr_layer *layer = NULL;
 	unsigned long flags;
 	int ret;
 	static int next_id;

@@ -305,9 +306,15 @@ static int cm_alloc_id(struct cm_id_priv
 	do {
 		spin_lock_irqsave(&cm.lock, flags);
 		ret = idr_get_new_above(&cm.local_id_table, cm_id_priv, next_id++,
-					(__force int *) &cm_id_priv->id.local_id);
+					(__force int *) &cm_id_priv->id.local_id,
+					layer);
 		spin_unlock_irqrestore(&cm.lock, flags);
-	} while( (ret == -EAGAIN) && idr_pre_get(&cm.local_id_table, GFP_KERNEL) );
+		if (ret == -EAGAIN) {
+			layer = idr_alloc_layer(GFP_KERNEL);
+			if (!layer)
+				ret = -ENOMEM;
+		}
+	} while (ret == -EAGAIN);
 	return ret;
 }

@@ -3347,7 +3354,6 @@ static int __init ib_cm_init(void)
 	cm.remote_qp_table = RB_ROOT;
 	cm.remote_sidr_table = RB_ROOT;
 	idr_init(&cm.local_id_table);
-	idr_pre_get(&cm.local_id_table, GFP_KERNEL);
 
 	cm.wq = create_workqueue("ib_cm");
 	if (!cm.wq)

drivers/infiniband/core/cma.c +8 -2

index d6f99d5..314b150 100644
@@ -1433,6 +1433,7 @@ static void cma_bind_port(struct rdma_bi
 static int cma_alloc_port(struct idr *ps, struct rdma_id_private *id_priv,
 			  unsigned short snum)
 {
+	struct idr_layer *layer = NULL;
 	struct rdma_bind_list *bind_list;
 	int port, start, ret;

@@ -1443,8 +1444,13 @@ static int cma_alloc_port(struct idr *ps
 	start = snum ? snum : sysctl_local_port_range[0];
 
 	do {
-		ret = idr_get_new_above(ps, bind_list, start, &port);
-	} while ((ret == -EAGAIN) && idr_pre_get(ps, GFP_KERNEL));
+		ret = idr_get_new_above(ps, bind_list, start, &port, layer);
+		if (ret == -EAGAIN) {
+			layer = idr_alloc_layer(GFP_KERNEL);
+			if (!layer)
+				ret = -ENOMEM;
+		}
+	} while (ret == -EAGAIN);
 
 	if (ret)
 		goto err;

drivers/infiniband/core/sa_query.c +8 -5

index aeda484..824b652 100644
@@ -490,17 +490,20 @@ static void init_mad(struct ib_sa_mad *m
 
 static int send_mad(struct ib_sa_query *query, int timeout_ms, gfp_t gfp_mask)
 {
+	struct idr_layer *layer = NULL;
 	unsigned long flags;
 	int ret, id;
 
 retry:
-	if (!idr_pre_get(&query_idr, gfp_mask))
-		return -ENOMEM;
 	spin_lock_irqsave(&idr_lock, flags);
-	ret = idr_get_new(&query_idr, query, &id);
+	ret = idr_get_new(&query_idr, query, &id, layer);
 	spin_unlock_irqrestore(&idr_lock, flags);
-	if (ret == -EAGAIN)
-		goto retry;
+	if (ret == -EAGAIN) {
+		layer = idr_alloc_layer(gfp_mask);
+		if (layer)
+			goto retry;
+		ret = -ENOMEM;
+	}
 	if (ret)
 		return ret;

drivers/infiniband/core/ucm.c +8 -5

index c1c6fda..22d5e24 100644
@@ -173,6 +173,7 @@ static void ib_ucm_cleanup_events(struct
 
 static struct ib_ucm_context *ib_ucm_ctx_alloc(struct ib_ucm_file *file)
 {
+	struct idr_layer *layer = NULL;
 	struct ib_ucm_context *ctx;
 	int result;

@@ -186,13 +187,15 @@ static struct ib_ucm_context *ib_ucm_ctx
 	INIT_LIST_HEAD(&ctx->events);
 
 	do {
-		result = idr_pre_get(&ctx_id_table, GFP_KERNEL);
-		if (!result)
-			goto error;
-
 		mutex_lock(&ctx_id_mutex);
-		result = idr_get_new(&ctx_id_table, ctx, &ctx->id);
+		result = idr_get_new(&ctx_id_table, ctx, &ctx->id, layer);
 		mutex_unlock(&ctx_id_mutex);
+
+		if (result == -EAGAIN) {
+			layer = idr_alloc_layer(GFP_KERNEL);
+			if (!layer)
+				result = -ENOMEM;
+		}
 	} while (result == -EAGAIN);
 
 	if (result)

drivers/infiniband/core/uverbs_cmd.c +11 -9

index bdf5d50..71dea88 100644
@@ -109,18 +109,20 @@ static void put_uobj_write(struct ib_uob
 
 static int idr_add_uobj(struct idr *idr, struct ib_uobject *uobj)
 {
+	struct idr_layer *layer = NULL;
 	int ret;
 
-retry:
-	if (!idr_pre_get(idr, GFP_KERNEL))
-		return -ENOMEM;
+	do {
+		spin_lock(&ib_uverbs_idr_lock);
+		ret = idr_get_new(idr, uobj, &uobj->id, layer);
+		spin_unlock(&ib_uverbs_idr_lock);
 
-	spin_lock(&ib_uverbs_idr_lock);
-	ret = idr_get_new(idr, uobj, &uobj->id);
-	spin_unlock(&ib_uverbs_idr_lock);
-
-	if (ret == -EAGAIN)
-		goto retry;
+		if (ret == -EAGAIN) {
+			layer = idr_alloc_layer(GFP_KERNEL);
+			if (!layer)
+				ret = -ENOMEM;
+		}
+	} while (ret == -EAGAIN);
 
 	return ret;
 }

drivers/infiniband/hw/ipath/ipath_driver.c +15 ~~-12~~

index 823131d..6f2a711 100644
@@ -168,15 +168,11 @@ static void ipath_free_devdata(struct pc
 
 static struct ipath_devdata *ipath_alloc_devdata(struct pci_dev *pdev)
 {
+	struct idr_layer *layer = NULL;
 	unsigned long flags;
 	struct ipath_devdata *dd;
 	int ret;
 
-	if (!idr_pre_get(&unit_table, GFP_KERNEL)) {
-		dd = ERR_PTR(-ENOMEM);
-		goto bail;
-	}
-
 	dd = vmalloc(sizeof(*dd));
 	if (!dd) {
 		dd = ERR_PTR(-ENOMEM);

@@ -187,7 +183,19 @@ static struct ipath_devdata *ipath_alloc
 
 	spin_lock_irqsave(&ipath_devs_lock, flags);
 
-	ret = idr_get_new(&unit_table, dd, &dd->ipath_unit);
+	do {
+		ret = idr_get_new(&unit_table, dd, &dd->ipath_unit, layer);
+		if (ret == -EAGAIN) {
+			spin_unlock_irqrestore(&ipath_devs_lock, flags);
+			layer = idr_alloc_layer(GFP_KERNEL);
+			if (!layer) {
+				dd = ERR_PTR(-ENOMEM);
+				goto bail;
+			}
+			spin_lock_irqsave(&ipath_devs_lock, flags);
+		}
+	} while (ret == -EAGAIN);
+
 	if (ret < 0) {
 		printk(KERN_ERR IPATH_DRV_NAME
 		       ": Could not allocate unit ID: error %d\n", -ret);

@@ -1754,10 +1762,6 @@ static int __init infinipath_init(void)
 	 * the PCI subsystem.
 	 */
 	idr_init(&unit_table);
-	if (!idr_pre_get(&unit_table, GFP_KERNEL)) {
-		ret = -ENOMEM;
-		goto bail;
-	}
 
 	ret = pci_register_driver(&ipath_driver);
 	if (ret < 0) {

@@ -1780,7 +1784,7 @@ static int __init infinipath_init(void)
 		goto bail_group;
 	}
 
-	goto bail;
+	return ret;
 
 bail_group:
 	ipath_driver_remove_group(&ipath_driver.driver);

@@ -1791,7 +1795,6 @@ bail_pci:
 bail_unit:
 	idr_destroy(&unit_table);
 
-bail:
 	return ret;
 }

drivers/md/dm.c +23 ~~-13~~

index c99bf9f..e2dd20a 100644
@@ -807,23 +807,30 @@ static void free_minor(int minor)
  */
 static int specific_minor(struct mapped_device *md, int minor)
 {
+	struct idr_layer *layer = NULL;
 	int r, m;
 
 	if (minor >= (1 << MINORBITS))
 		return -EINVAL;
 
-	r = idr_pre_get(&_minor_idr, GFP_KERNEL);
-	if (!r)
-		return -ENOMEM;
-
 	spin_lock(&_minor_lock);
 
+again:
 	if (idr_find(&_minor_idr, minor)) {
 		r = -EBUSY;
 		goto out;
 	}
 
-	r = idr_get_new_above(&_minor_idr, MINOR_ALLOCED, minor, &m);
+	r = idr_get_new_above(&_minor_idr, MINOR_ALLOCED, minor, &m, layer);
+	if (r == -EAGAIN) {
+		spin_unlock(&_minor_lock);
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer)
+			return -ENOMEM;
+
+		spin_lock(&_minor_lock);
+		goto again;
+	}
 	if (r)
 		goto out;

@@ -840,18 +847,21 @@ out:
 
 static int next_free_minor(struct mapped_device *md, int *minor)
 {
+	struct idr_layer *layer = NULL;
 	int r, m;
 
-	r = idr_pre_get(&_minor_idr, GFP_KERNEL);
-	if (!r)
-		return -ENOMEM;
-
+again:
 	spin_lock(&_minor_lock);
-
-	r = idr_get_new(&_minor_idr, MINOR_ALLOCED, &m);
-	if (r) {
-		goto out;
+	r = idr_get_new(&_minor_idr, MINOR_ALLOCED, &m, layer);
+	if (r == -EAGAIN) {
+		spin_unlock(&_minor_lock);
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer)
+			return -ENOMEM;
+		goto again;
 	}
+	if (r)
+		goto out;
 
 	if (m >= (1 << MINORBITS)) {
 		idr_remove(&_minor_idr, m);

drivers/mmc/mmc_sysfs.c +9 -4

index a2a35fd..bf61ff4 100644
@@ -280,14 +280,19 @@ struct mmc_host *mmc_alloc_host_sysfs(in
  */
 int mmc_add_host_sysfs(struct mmc_host *host)
 {
+	struct idr_layer *layer = NULL;
 	int err;
 
-	if (!idr_pre_get(&mmc_host_idr, GFP_KERNEL))
-		return -ENOMEM;
-
+again:
 	spin_lock(&mmc_host_lock);
-	err = idr_get_new(&mmc_host_idr, host, &host->index);
+	err = idr_get_new(&mmc_host_idr, host, &host->index, layer);
 	spin_unlock(&mmc_host_lock);
+	if (err == -EAGAIN) {
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer)
+			return -ENOMEM;
+		goto again;
+	}
 	if (err)
 		return err;

drivers/rtc/class.c +9 -7

index 1cb61a7..3388059 100644
@@ -42,19 +42,21 @@ struct rtc_device *rtc_device_register(c
 					struct rtc_class_ops *ops,
 					struct module *owner)
 {
+	struct idr_layer *layer = NULL;
 	struct rtc_device *rtc;
 	int id, err;
 
-	if (idr_pre_get(&rtc_idr, GFP_KERNEL) == 0) {
-		err = -ENOMEM;
-		goto exit;
-	}
-
-
+again:
 	mutex_lock(&idr_lock);
-	err = idr_get_new(&rtc_idr, NULL, &id);
+	err = idr_get_new(&rtc_idr, NULL, &id, layer);
 	mutex_unlock(&idr_lock);
 
+	if (err == -EAGAIN) {
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer)
+			return ERR_PTR(-ENOMEM);
+		goto again;
+	}
 	if (err < 0)
 		goto exit;

drivers/scsi/lpfc/lpfc_init.c +11 -4

index 81755a3..401608b 100644
@@ -1421,6 +1421,7 @@ lpfc_pci_probe_one(struct pci_dev *pdev,
 	struct lpfc_hba  *phba;
 	struct lpfc_sli  *psli;
 	struct lpfc_iocbq *iocbq_entry = NULL, *iocbq_next = NULL;
+	struct idr_layer *layer = NULL;
 	unsigned long bar0map_len, bar2map_len;
 	int error = -ENODEV, retval;
 	int i;

@@ -1443,10 +1444,16 @@ lpfc_pci_probe_one(struct pci_dev *pdev,
 	phba->pcidev = pdev;
 
 	/* Assign an unused board number */
-	if (!idr_pre_get(&lpfc_hba_index, GFP_KERNEL))
-		goto out_put_host;
-
-	error = idr_get_new(&lpfc_hba_index, NULL, &phba->brd_no);
+again:
+	error = idr_get_new(&lpfc_hba_index, NULL, &phba->brd_no, layer);
+	if (error == -EAGAIN) {
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer) {
+			error = -ENOMEM;
+			goto out_put_host;
+		}
+		goto again;
+	}
 	if (error)
 		goto out_put_host;

drivers/scsi/sd.c +11 -4

index 3225d31..fe504be 100644
@@ -1617,6 +1617,7 @@ static int sd_probe(struct device *dev)
 	struct scsi_device *sdp = to_scsi_device(dev);
 	struct scsi_disk *sdkp;
 	struct gendisk *gd;
+	struct idr_layer *layer = NULL;
 	u32 index;
 	int error;

@@ -1636,13 +1637,19 @@ static int sd_probe(struct device *dev)
 	if (!gd)
 		goto out_free;
 
-	if (!idr_pre_get(&sd_index_idr, GFP_KERNEL))
-		goto out_put;
-
+again:
 	spin_lock(&sd_index_lock);
-	error = idr_get_new(&sd_index_idr, NULL, &index);
+	error = idr_get_new(&sd_index_idr, NULL, &index, layer);
 	spin_unlock(&sd_index_lock);
 
+	if (error == -EAGAIN) {
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer) {
+			error = -ENOMEM;
+			goto out_put;
+		}
+		goto again;
+	}
 	if (index >= SD_MAX_DISKS)
 		error = -EBUSY;
 	if (error)

fs/9p/v9fs.c +7 -6

index 22f7ccd..d03c3ba 100644
@@ -198,25 +198,26 @@ struct v9fs_session_info *v9fs_inode2v9s
 
 int v9fs_get_idpool(struct v9fs_idpool *p)
 {
+	struct idr_layer *layer = NULL;
 	int i = 0;
 	int error;
 
 retry:
-	if (idr_pre_get(&p->pool, GFP_KERNEL) == 0)
-		return 0;
-
 	if (down_interruptible(&p->lock) == -EINTR) {
 		eprintk(KERN_WARNING, "Interrupted while locking\n");
 		return -1;
 	}
 
 	/* no need to store exactly p, we just need something non-null */
-	error = idr_get_new(&p->pool, p, &i);
+	error = idr_get_new(&p->pool, p, &i, layer);
 	up(&p->lock);
 
-	if (error == -EAGAIN)
+	if (error == -EAGAIN) {
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer)
+			return -1;
 		goto retry;
-	else if (error)
+	} else if (error)
 		return -1;
 
 	return i;

fs/inotify.c +7 -3

index 723836a..7e12bed 100644
@@ -131,12 +131,16 @@ EXPORT_SYMBOL_GPL(put_inotify_watch);
 static int inotify_handle_get_wd(struct inotify_handle *ih,
 				 struct inotify_watch *watch)
 {
+	struct idr_layer *layer = NULL;
 	int ret;
 
 	do {
-		if (unlikely(!idr_pre_get(&ih->idr, GFP_KERNEL)))
-			return -ENOSPC;
-		ret = idr_get_new_above(&ih->idr, watch, ih->last_wd+1, &watch->wd);
+		ret = idr_get_new_above(&ih->idr, watch, ih->last_wd+1, &watch->wd, layer);
+		if (ret == -EAGAIN) {
+			layer = idr_alloc_layer(GFP_KERNEL);
+			if (!layer)
+				ret = -ENOSPC;
+		}
 	} while (ret == -EAGAIN);
 
 	if (likely(!ret))

fs/ocfs2/cluster/tcp.c +7 -5

index b650efa..f9d1b04 100644
@@ -175,19 +175,21 @@ static u8 o2net_num_from_nn(struct o2net
 
 static int o2net_prep_nsw(struct o2net_node *nn, struct o2net_status_wait *nsw)
 {
+	struct idr_layer *layer = NULL;
 	int ret = 0;
 
 	do {
-		if (!idr_pre_get(&nn->nn_status_idr, GFP_ATOMIC)) {
-			ret = -EAGAIN;
-			break;
-		}
 		spin_lock(&nn->nn_lock);
-		ret = idr_get_new(&nn->nn_status_idr, nsw, &nsw->ns_id);
+		ret = idr_get_new(&nn->nn_status_idr, nsw, &nsw->ns_id, layer);
 		if (ret == 0)
 			list_add_tail(&nsw->ns_node_item,
 				      &nn->nn_status_list);
 		spin_unlock(&nn->nn_lock);
+		if (ret == -EAGAIN) {
+			layer = idr_alloc_layer(GFP_ATOMIC);
+			if (!layer)
+				ret = -ENOMEM;
+		}
 	} while (ret == -EAGAIN);
 
 	if (ret == 0)  {

fs/proc/generic.c +7 -6

index 4ba0300..60bdd42 100644
@@ -318,19 +318,20 @@ #define PROC_DYNAMIC_FIRST 0xF0000000UL
  */
 static unsigned int get_inode_number(void)
 {
+	struct idr_layer *layer = NULL;
 	int i, inum = 0;
 	int error;
 
 retry:
-	if (idr_pre_get(&proc_inum_idr, GFP_KERNEL) == 0)
-		return 0;
-
 	spin_lock(&proc_inum_lock);
-	error = idr_get_new(&proc_inum_idr, NULL, &i);
+	error = idr_get_new(&proc_inum_idr, NULL, &i, layer);
 	spin_unlock(&proc_inum_lock);
-	if (error == -EAGAIN)
+	if (error == -EAGAIN) {
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (!layer)
+			return 0;
 		goto retry;
-	else if (error)
+	} else if (error)
 		return 0;
 
 	inum = (i & MAX_ID_MASK) + PROC_DYNAMIC_FIRST;

fs/super.c +7 -6

index 6d4e817..67455f5 100644
@@ -607,19 +607,20 @@ static DEFINE_SPINLOCK(unnamed_dev_lock)
 
 int set_anon_super(struct super_block *s, void *data)
 {
+	struct idr_layer *layer = NULL;
 	int dev;
 	int error;
 
  retry:
-	if (idr_pre_get(&unnamed_dev_idr, GFP_ATOMIC) == 0)
-		return -ENOMEM;
 	spin_lock(&unnamed_dev_lock);
-	error = idr_get_new(&unnamed_dev_idr, NULL, &dev);
+	error = idr_get_new(&unnamed_dev_idr, NULL, &dev, layer);
 	spin_unlock(&unnamed_dev_lock);
-	if (error == -EAGAIN)
-		/* We raced and lost with another CPU. */
+	if (error == -EAGAIN) {
+		layer = idr_alloc_layer(GFP_ATOMIC);
+		if (!layer)
+			return -ENOMEM;
 		goto retry;
-	else if (error)
+	} else if (error)
 		return -EAGAIN;
 
 	if ((dev & MAX_ID_MASK) == (1 << MINORBITS)) {

include/linux/idr.h +5 -6

index 8268034..de34c4e 100644
@@ -44,7 +44,7 @@ #define MAX_ID_MASK (MAX_ID_BIT - 1)
 #define MAX_LEVEL (MAX_ID_SHIFT + IDR_BITS - 1) / IDR_BITS
 
 /* Number of id_layer structs to leave in free list */
-#define IDR_FREE_MAX MAX_LEVEL + MAX_LEVEL
+#define IDR_FREE_MAX (MAX_LEVEL + MAX_LEVEL)
 
 struct idr_layer {
 	unsigned long		 bitmap; /* A zero bit means "space here" */

@@ -57,7 +57,6 @@ struct idr {
 	struct idr_layer *id_free;
 	int		  layers;
 	int		  id_free_cnt;
-	spinlock_t	  lock;
 };
 
 #define IDR_INIT(name)						\

@@ -66,7 +65,6 @@ #define IDR_INIT(name)						\
 	.id_free	= NULL,					\
 	.layers 	= 0,					\
 	.id_free_cnt	= 0,					\
-	.lock		= __SPIN_LOCK_UNLOCKED(name.lock),	\
 }
 #define DEFINE_IDR(name)	struct idr name = IDR_INIT(name)

@@ -75,9 +73,10 @@ #define DEFINE_IDR(name)	struct idr name
  */
 
 void *idr_find(struct idr *idp, int id);
-int idr_pre_get(struct idr *idp, gfp_t gfp_mask);
-int idr_get_new(struct idr *idp, void *ptr, int *id);
-int idr_get_new_above(struct idr *idp, void *ptr, int starting_id, int *id);
+struct idr_layer *idr_alloc_layer(gfp_t gfp_mask);
+int idr_get_new(struct idr *idp, void *ptr, int *id, struct idr_layer *layer);
+int idr_get_new_above(struct idr *idp, void *ptr, int starting_id, int *id,
+		      struct idr_layer *layer);
 void *idr_replace(struct idr *idp, void *ptr, int id);
 void idr_remove(struct idr *idp, int id);
 void idr_destroy(struct idr *idp);

kernel/posix-timers.c +8 -8

index ac6dc87..18891b2 100644
@@ -434,6 +434,7 @@ sys_timer_create(const clockid_t which_c
 		 struct sigevent __user *timer_event_spec,
 		 timer_t __user * created_timer_id)
 {
+	struct idr_layer *layer = NULL;
 	int error = 0;
 	struct k_itimer *new_timer = NULL;
 	int new_timer_id;

@@ -451,17 +452,16 @@ sys_timer_create(const clockid_t which_c
 
 	spin_lock_init(&new_timer->it_lock);
  retry:
-	if (unlikely(!idr_pre_get(&posix_timers_id, GFP_KERNEL))) {
-		error = -EAGAIN;
-		goto out;
-	}
 	spin_lock_irq(&idr_lock);
 	error = idr_get_new(&posix_timers_id, (void *) new_timer,
-			    &new_timer_id);
+			    &new_timer_id, layer);
 	spin_unlock_irq(&idr_lock);
-	if (error == -EAGAIN)
-		goto retry;
-	else if (error) {
+	if (error == -EAGAIN) {
+		layer = idr_alloc_layer(GFP_KERNEL);
+		if (layer)
+			goto retry;
+	}
+	if (error) {
 		/*
 		 * Wierd looking, but we return EAGAIN if the IDR is
 		 * full (proper POSIX return value for this)

lib/idr.c +34 ~~-83~~

240 lines not on this page · raw message

net/sctp/sm_make_chunk.c +8 -6

index 2a87736..fc712c4 100644
@@ -1928,6 +1928,7 @@ int sctp_process_init(struct sctp_associ
  	 * association.
 	 */
 	if (!asoc->temp) {
+		struct idr_layer *layer = NULL;
 		int assoc_id;
 		int error;

@@ -1937,15 +1938,16 @@ int sctp_process_init(struct sctp_associ
 			goto clean_up;
 
 	retry:
-		if (unlikely(!idr_pre_get(&sctp_assocs_id, gfp)))
-			goto clean_up;
 		spin_lock_bh(&sctp_assocs_id_lock);
 		error = idr_get_new_above(&sctp_assocs_id, (void *)asoc, 1,
-					  &assoc_id);
+					  &assoc_id, layer);
 		spin_unlock_bh(&sctp_assocs_id_lock);
-		if (error == -EAGAIN)
-			goto retry;
-		else if (error)
+		if (error == -EAGAIN) {
+			layer = idr_alloc_layer(gfp);
+			if (layer)
+				goto retry;
+		}
+		if (error)
 			goto clean_up;
 
 		asoc->assoc_id = (sctp_assoc_t) assoc_id;

Roland Dreier

2006-07-12 02:33 UTC #29 lore raw

In reply to Zach Brown

OK, the patch to lib/idr.c below is at least one way to fix this. However, with that applied I get the lockdep warning below, which seems to be a false positive -- I'm not sure what the right fix is, but the blame really seems to fall on udp_ioctl for poking into the sk_buff_head lock itself.

Ingo and/or Arjan, any thoughts on the idr.c change or the sk_buff_head warning?

Thanks,
Roland

Here's the idr.c change:

lib/idr.c +10 -6

index 4d09681..16d2143 100644
@@ -38,14 +38,15 @@ static kmem_cache_t *idr_layer_cache;
 static struct idr_layer *alloc_layer(struct idr *idp)
 {
 	struct idr_layer *p;
+	unsigned long flags;
 
-	spin_lock(&idp->lock);
+	spin_lock_irqsave(&idp->lock, flags);
 	if ((p = idp->id_free)) {
 		idp->id_free = p->ary[0];
 		idp->id_free_cnt--;
 		p->ary[0] = NULL;
 	}
-	spin_unlock(&idp->lock);
+	spin_unlock_irqrestore(&idp->lock, flags);
 	return(p);
 }

@@ -59,12 +60,14 @@ static void __free_layer(struct idr *idp
 
 static void free_layer(struct idr *idp, struct idr_layer *p)
 {
+	unsigned long flags;
+
 	/*
 	 * Depends on the return element being zeroed.
 	 */
-	spin_lock(&idp->lock);
+	spin_lock_irqsave(&idp->lock, flags);
 	__free_layer(idp, p);
-	spin_unlock(&idp->lock);
+	spin_unlock_irqrestore(&idp->lock, flags);
 }
 
 /**

@@ -168,6 +171,7 @@ static int idr_get_new_above_int(struct 
 {
 	struct idr_layer *p, *new;
 	int layers, v, id;
+	unsigned long flags;
 
 	id = starting_id;
 build_up:

@@ -191,14 +195,14 @@ build_up:
 			 * The allocation failed.  If we built part of
 			 * the structure tear it down.
 			 */
-			spin_lock(&idp->lock);
+			spin_lock_irqsave(&idp->lock, flags);
 			for (new = p; p && p != idp->top; new = p) {
 				p = p->ary[0];
 				new->ary[0] = NULL;
 				new->bitmap = new->count = 0;
 				__free_layer(idp, new);
 			}
-			spin_unlock(&idp->lock);
+			spin_unlock_irqrestore(&idp->lock, flags);
 			return -1;
 		}
 		new->ary[0] = p;

And here's the warning I get, which appears to be a false positive:

======================================================
[ INFO: hard-safe -> hard-unsafe lock order detected ]
------------------------------------------------------
swapper/0 [HC0[0]:SC1[2]:HE0:SE0] is trying to acquire:
(&skb_queue_lock_key){-+..}, at: [<ffffffff8038683e>] skb_queue_tail+0x1d/0x47

and this task is already holding:
(&priv->lock){.+..}, at: [<ffffffff881efd5b>] ipoib_mcast_send+0x29/0x413 [ib_ipoib]
which would create a new lock dependency:
(&priv->lock){.+..} -> (&skb_queue_lock_key){-+..}

but this new dependency connects a hard-irq-safe lock:
(&priv->tx_lock){+...}
... which became hard-irq-safe at:

  [<ffffffff8024172a>] lock_acquire+0x4a/0x69
  [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
  [<ffffffff881ee615>] ipoib_ib_completion+0x340/0x3ff [ib_ipoib]
  [<ffffffff881ca8a5>] mthca_cq_completion+0x65/0x6b [ib_mthca]
  [<ffffffff881ca1bc>] mthca_eq_int+0x70/0x3d2 [ib_mthca]
  [<ffffffff881ca644>] mthca_arbel_interrupt+0x3b/0x98 [ib_mthca]
  [<ffffffff802537de>] handle_IRQ_event+0x28/0x64
  [<ffffffff802538c6>] __do_IRQ+0xac/0x117
  [<ffffffff8020becf>] do_IRQ+0xf7/0x108
  [<ffffffff80209bd8>] common_interrupt+0x64/0x65

to a hard-irq-unsafe lock:
(&skb_queue_lock_key){-+..}
... which became hard-irq-unsafe at:
... [<ffffffff8024172a>] lock_acquire+0x4a/0x69

  [<ffffffff803eb349>] _spin_lock_bh+0x26/0x33
  [<ffffffff803c6de8>] udp_ioctl+0x46/0x87
  [<ffffffff803cd1ab>] inet_ioctl+0x8c/0x8f
  [<ffffffff8038203c>] sock_ioctl+0x1c0/0x1ea
  [<ffffffff8028591e>] do_ioctl+0x26/0x74
  [<ffffffff80285bb6>] vfs_ioctl+0x24a/0x264
  [<ffffffff80285c11>] sys_ioctl+0x41/0x68
  [<ffffffff802096a1>] system_call+0x7d/0x83

other info that might help us debug this:

2 locks held by swapper/0:
#0: (&priv->tx_lock){+...}, at: [<ffffffff881ed6ef>] ipoib_start_xmit+0x42/0x66d [ib_ipoib] #1: (&priv->lock){.+..}, at: [<ffffffff881efd5b>] ipoib_mcast_send+0x29/0x413 [ib_ipoib]

the hard-irq-safe lock's dependencies:
-> (&priv->tx_lock){+...} ops: 0 {
initial-use at:

                        [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                        [<ffffffff803eb644>] _spin_lock_irq+0x28/0x35
                        [<ffffffff881ef705>] ipoib_mcast_join_finish+0x340/0x379 [ib_ipoib]
                        [<ffffffff881ef7e0>] ipoib_mcast_join_complete+0xa2/0x294 [ib_ipoib]
                        [<ffffffff881e6389>] ib_sa_mcmember_rec_callback+0x4b/0x57 [ib_sa]
                        [<ffffffff881e648d>] recv_handler+0x3d/0x4a [ib_sa]
                        [<ffffffff881b9c20>] ib_mad_completion_handler+0x3df/0x5e4 [ib_mad]
                        [<ffffffff80238422>] run_workqueue+0xa0/0xf7
                        [<ffffffff8023861d>] worker_thread+0xee/0x122
                        [<ffffffff8023b661>] kthread+0xd0/0xfb
                        [<ffffffff8020a5f1>] child_rip+0x7/0x12
   in-hardirq-W at:
                        [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                        [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                        [<ffffffff881ee615>] ipoib_ib_completion+0x340/0x3ff [ib_ipoib]
                        [<ffffffff881ca8a5>] mthca_cq_completion+0x65/0x6b [ib_mthca]
                        [<ffffffff881ca1bc>] mthca_eq_int+0x70/0x3d2 [ib_mthca]
                        [<ffffffff881ca644>] mthca_arbel_interrupt+0x3b/0x98 [ib_mthca]
                        [<ffffffff802537de>] handle_IRQ_event+0x28/0x64
                        [<ffffffff802538c6>] __do_IRQ+0xac/0x117
                        [<ffffffff8020becf>] do_IRQ+0xf7/0x108
                        [<ffffffff80209bd8>] common_interrupt+0x64/0x65
 }
 ... key      at: [<ffffffff881f9a28>] __key.23652+0x0/0xffffffffffff804f [ib_ipoib]
 -> (&priv->lock){.+..} ops: 0 {
    initial-use  at:
                          [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                          [<ffffffff803eb644>] _spin_lock_irq+0x28/0x35
                          [<ffffffff881ef2c6>] ipoib_mcast_start_thread+0x6e/0x87 [ib_ipoib]
                          [<ffffffff881eee7d>] ipoib_ib_dev_up+0x52/0x58 [ib_ipoib]
                          [<ffffffff881ec23f>] ipoib_open+0x65/0x102 [ib_ipoib]
                          [<ffffffff8038c74c>] dev_open+0x3a/0x80
                          [<ffffffff8038ba90>] dev_change_flags+0x65/0x139
                          [<ffffffff803ccb56>] devinet_ioctl+0x240/0x5e2
                          [<ffffffff803cd18f>] inet_ioctl+0x70/0x8f
                          [<ffffffff8038203c>] sock_ioctl+0x1c0/0x1ea
                          [<ffffffff8028591e>] do_ioctl+0x26/0x74
                          [<ffffffff80285bb6>] vfs_ioctl+0x24a/0x264
                          [<ffffffff80285c11>] sys_ioctl+0x41/0x68
                          [<ffffffff802096a1>] system_call+0x7d/0x83
    in-softirq-W at:
                          [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                          [<ffffffff803eb316>] _spin_lock+0x21/0x2e
                          [<ffffffff881efd5a>] ipoib_mcast_send+0x28/0x413 [ib_ipoib]
                          [<ffffffff881eda0e>] ipoib_start_xmit+0x361/0x66d [ib_ipoib]
                          [<ffffffff8038dfc8>] dev_hard_start_xmit+0x1ab/0x221
                          [<ffffffff8039a024>] __qdisc_run+0xfa/0x1cd
                          [<ffffffff8038e174>] dev_queue_xmit+0x136/0x263
                          [<ffffffff80390209>] neigh_connected_output+0xae/0xc7
                          [<ffffffff8813f4de>] ip6_output2+0x254/0x28c [ipv6]
                          [<ffffffff8813fcf8>] ip6_output+0x7e2/0x7f8 [ipv6]
                          [<ffffffff8814dbf7>] ndisc_send_ns+0x38f/0x4c1 [ipv6]
                          [<ffffffff88145048>] addrconf_dad_timer+0xfb/0x11e [ipv6]
                          [<ffffffff80231a8e>] run_timer_softirq+0x150/0x1dc
                          [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
                          [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                          [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                          [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
                          [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70
  }
  ... key      at: [<ffffffff881f9a30>] __key.23651+0x0/0xffffffffffff8047 [ib_ipoib]
  -> (&idr_lock){....} ops: 0 {
     initial-use  at:
                            [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                            [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                            [<ffffffff881e6161>] send_mad+0x39/0x141 [ib_sa]
                            [<ffffffff881e6a20>] ib_sa_mcmember_rec_query+0x147/0x17c [ib_sa]
                            [<ffffffff881f02a4>] ipoib_mcast_join+0x15f/0x1f2 [ib_ipoib]
                            [<ffffffff881f0582>] ipoib_mcast_join_task+0x24b/0x2e7 [ib_ipoib]
                            [<ffffffff80238422>] run_workqueue+0xa0/0xf7
                            [<ffffffff8023861d>] worker_thread+0xee/0x122
                            [<ffffffff8023b661>] kthread+0xd0/0xfb
                            [<ffffffff8020a5f1>] child_rip+0x7/0x12
   }
   ... key      at: [<ffffffff881ea488>] __key.17990+0x0/0xffffffffffffc8c2 [ib_sa]
   -> (query_idr.lock){....} ops: 0 {
      initial-use  at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                              [<ffffffff802e4581>] free_layer+0x1c/0x40
                              [<ffffffff802e45d6>] idr_pre_get+0x31/0x42
                              [<ffffffff881e614d>] send_mad+0x25/0x141 [ib_sa]
                              [<ffffffff881e6a20>] ib_sa_mcmember_rec_query+0x147/0x17c [ib_sa]
                              [<ffffffff881f02a4>] ipoib_mcast_join+0x15f/0x1f2 [ib_ipoib]
                              [<ffffffff881f0582>] ipoib_mcast_join_task+0x24b/0x2e7 [ib_ipoib]
                              [<ffffffff80238422>] run_workqueue+0xa0/0xf7
                              [<ffffffff8023861d>] worker_thread+0xee/0x122
                              [<ffffffff8023b661>] kthread+0xd0/0xfb
                              [<ffffffff8020a5f1>] child_rip+0x7/0x12
    }
    ... key      at: [<ffffffff881e9f30>] query_idr+0x30/0xffffffffffffce4a [ib_sa]
   ... acquired at:
   [<ffffffff8024172a>] lock_acquire+0x4a/0x69
   [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
   [<ffffffff802e4530>] alloc_layer+0x18/0x4d
   [<ffffffff802e48de>] idr_get_new_above_int+0x37/0x225
   [<ffffffff802e4adb>] idr_get_new+0xf/0x2f
   [<ffffffff881e6177>] send_mad+0x4f/0x141 [ib_sa]
   [<ffffffff881e6a20>] ib_sa_mcmember_rec_query+0x147/0x17c [ib_sa]
   [<ffffffff881f02a4>] ipoib_mcast_join+0x15f/0x1f2 [ib_ipoib]
   [<ffffffff881f0582>] ipoib_mcast_join_task+0x24b/0x2e7 [ib_ipoib]
   [<ffffffff80238422>] run_workqueue+0xa0/0xf7
   [<ffffffff8023861d>] worker_thread+0xee/0x122
   [<ffffffff8023b661>] kthread+0xd0/0xfb
   [<ffffffff8020a5f1>] child_rip+0x7/0x12

... acquired at:

   [<ffffffff8024172a>] lock_acquire+0x4a/0x69
   [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
   [<ffffffff881e6069>] ib_sa_cancel_query+0x1b/0x73 [ib_sa]
   [<ffffffff881eefb5>] wait_for_mcast_join+0x35/0xd2 [ib_ipoib]
   [<ffffffff881ef385>] ipoib_mcast_stop_thread+0xa6/0xe6 [ib_ipoib]
   [<ffffffff881f07da>] ipoib_mcast_restart_task+0x4b/0x3d9 [ib_ipoib]
   [<ffffffff80238422>] run_workqueue+0xa0/0xf7
   [<ffffffff8023861d>] worker_thread+0xee/0x122
   [<ffffffff8023b661>] kthread+0xd0/0xfb
   [<ffffffff8020a5f1>] child_rip+0x7/0x12

  -> (&mad_agent_priv->lock){....} ops: 0 {
     initial-use  at:
                            [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                            [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                            [<ffffffff881b936d>] ib_post_send_mad+0x3e0/0x512 [ib_mad]
                            [<ffffffff881bb251>] agent_send_response+0x11b/0x155 [ib_mad]
                            [<ffffffff881b9b2b>] ib_mad_completion_handler+0x2ea/0x5e4 [ib_mad]
                            [<ffffffff80238422>] run_workqueue+0xa0/0xf7
                            [<ffffffff8023861d>] worker_thread+0xee/0x122
                            [<ffffffff8023b661>] kthread+0xd0/0xfb
                            [<ffffffff8020a5f1>] child_rip+0x7/0x12
   }
   ... key      at: [<ffffffff881c1424>] __key.17415+0x0/0xffffffffffffaf74 [ib_mad]
   -> (base_lock_keys + cpu){++..} ops: 0 {
      initial-use  at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb644>] _spin_lock_irq+0x28/0x35
                              [<ffffffff8023197f>] run_timer_softirq+0x41/0x1dc
                              [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
                              [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                              [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                              [<ffffffff8020bed4>] do_IRQ+0xfc/0x108
                              [<ffffffff80209bd8>] common_interrupt+0x64/0x65
      in-hardirq-W at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb316>] _spin_lock+0x21/0x2e
                              [<ffffffff80231bad>] __mod_timer+0x93/0xc8
                              [<ffffffff80231c0e>] mod_timer+0x2c/0x2f
                              [<ffffffff80377fb2>] i8042_interrupt+0x2a/0x222
                              [<ffffffff802537de>] handle_IRQ_event+0x28/0x64
                              [<ffffffff802538c6>] __do_IRQ+0xac/0x117
                              [<ffffffff8020becf>] do_IRQ+0xf7/0x108
                              [<ffffffff80209bd8>] common_interrupt+0x64/0x65
                              [<7ffffffffffffffd>] 0x7ffffffffffffffd
                              [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                              [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                              [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
                              [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70
      in-softirq-W at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb644>] _spin_lock_irq+0x28/0x35
                              [<ffffffff8023197f>] run_timer_softirq+0x41/0x1dc
                              [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
                              [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                              [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                              [<ffffffff8020bed4>] do_IRQ+0xfc/0x108
                              [<ffffffff80209bd8>] common_interrupt+0x64/0x65
    }
    ... key      at: [<ffffffff805a16e0>] base_lock_keys+0x0/0x20
   ... acquired at:
   [<ffffffff8024172a>] lock_acquire+0x4a/0x69
   [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
   [<ffffffff80231316>] lock_timer_base+0x21/0x47
   [<ffffffff80231c84>] try_to_del_timer_sync+0x14/0x62
   [<ffffffff80231ce3>] del_timer_sync+0x11/0x1a
   [<ffffffff881b8a7b>] ib_mad_complete_send_wr+0x10b/0x1f8 [ib_mad]
   [<ffffffff881b8c8d>] ib_mad_send_done_handler+0x125/0x17a [ib_mad]
   [<ffffffff881b9db2>] ib_mad_completion_handler+0x571/0x5e4 [ib_mad]
   [<ffffffff80238422>] run_workqueue+0xa0/0xf7
   [<ffffffff8023861d>] worker_thread+0xee/0x122
   [<ffffffff8023b661>] kthread+0xd0/0xfb
   [<ffffffff8020a5f1>] child_rip+0x7/0x12

   -> (base_lock_keys + cpu#4){++..} ops: 0 {
      initial-use  at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb644>] _spin_lock_irq+0x28/0x35
                              [<ffffffff8023197f>] run_timer_softirq+0x41/0x1dc
                              [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
                              [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                              [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                              [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
                              [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70
      in-hardirq-W at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                              [<ffffffff80231316>] lock_timer_base+0x21/0x47
                              [<ffffffff80231c2f>] del_timer+0x1e/0x5f
                              [<ffffffff8034f281>] scsi_delete_timer+0x12/0x5d
                              [<ffffffff8034d064>] scsi_done+0xd/0x1e
                              [<ffffffff8035d444>] ata_scsi_qc_complete+0xb1/0xc3
                              [<ffffffff803583e9>] __ata_qc_complete+0x218/0x225
                              [<ffffffff803584c5>] ata_qc_complete+0xcf/0xd5
                              [<ffffffff80359cf0>] ata_hsm_qc_complete+0x1c6/0x1d8
                              [<ffffffff8035a31a>] ata_hsm_move+0x618/0x638
                              [<ffffffff8035a8ba>] ata_interrupt+0x161/0x1ae
                              [<ffffffff802537de>] handle_IRQ_event+0x28/0x64
                              [<ffffffff802538c6>] __do_IRQ+0xac/0x117
                              [<ffffffff8020becf>] do_IRQ+0xf7/0x108
                              [<ffffffff80209bd8>] common_interrupt+0x64/0x65
      in-softirq-W at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb644>] _spin_lock_irq+0x28/0x35
                              [<ffffffff8023197f>] run_timer_softirq+0x41/0x1dc
                              [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
                              [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                              [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                              [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
                              [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70
    }
    ... key      at: [<ffffffff805a16f8>] base_lock_keys+0x18/0x20
   ... acquired at:
   [<ffffffff8024172a>] lock_acquire+0x4a/0x69
   [<ffffffff803eb316>] _spin_lock+0x21/0x2e
   [<ffffffff80231bad>] __mod_timer+0x93/0xc8
   [<ffffffff802386c6>] queue_delayed_work+0x75/0x7d
   [<ffffffff881b776a>] wait_for_response+0xdf/0xe8 [ib_mad]
   [<ffffffff881b8a31>] ib_mad_complete_send_wr+0xc1/0x1f8 [ib_mad]
   [<ffffffff881b8c8d>] ib_mad_send_done_handler+0x125/0x17a [ib_mad]
   [<ffffffff881b9db2>] ib_mad_completion_handler+0x571/0x5e4 [ib_mad]
   [<ffffffff80238422>] run_workqueue+0xa0/0xf7
   [<ffffffff8023861d>] worker_thread+0xee/0x122
   [<ffffffff8023b661>] kthread+0xd0/0xfb
   [<ffffffff8020a5f1>] child_rip+0x7/0x12

   -> (base_lock_keys + cpu#2){++..} ops: 0 {
      initial-use  at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb644>] _spin_lock_irq+0x28/0x35
                              [<ffffffff8023197f>] run_timer_softirq+0x41/0x1dc
                              [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
                              [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                              [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                              [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
                              [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70
      in-hardirq-W at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb316>] _spin_lock+0x21/0x2e
                              [<ffffffff80231bad>] __mod_timer+0x93/0xc8
                              [<ffffffff80231c0e>] mod_timer+0x2c/0x2f
                              [<ffffffff80377fb2>] i8042_interrupt+0x2a/0x222
                              [<ffffffff802537de>] handle_IRQ_event+0x28/0x64
                              [<ffffffff802538c6>] __do_IRQ+0xac/0x117
                              [<ffffffff8020becf>] do_IRQ+0xf7/0x108
                              [<ffffffff80209bd8>] common_interrupt+0x64/0x65
      in-softirq-W at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb644>] _spin_lock_irq+0x28/0x35
                              [<ffffffff8023197f>] run_timer_softirq+0x41/0x1dc
                              [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
                              [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                              [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                              [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
                              [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70
    }
    ... key      at: [<ffffffff805a16e8>] base_lock_keys+0x8/0x20
   ... acquired at:
   [<ffffffff8024172a>] lock_acquire+0x4a/0x69
   [<ffffffff803eb316>] _spin_lock+0x21/0x2e
   [<ffffffff80231bad>] __mod_timer+0x93/0xc8
   [<ffffffff802386c6>] queue_delayed_work+0x75/0x7d
   [<ffffffff881b776a>] wait_for_response+0xdf/0xe8 [ib_mad]
   [<ffffffff881b8a31>] ib_mad_complete_send_wr+0xc1/0x1f8 [ib_mad]
   [<ffffffff881b8c8d>] ib_mad_send_done_handler+0x125/0x17a [ib_mad]
   [<ffffffff881b9db2>] ib_mad_completion_handler+0x571/0x5e4 [ib_mad]
   [<ffffffff80238422>] run_workqueue+0xa0/0xf7
   [<ffffffff8023861d>] worker_thread+0xee/0x122
   [<ffffffff8023b661>] kthread+0xd0/0xfb
   [<ffffffff8020a5f1>] child_rip+0x7/0x12

... acquired at:

   [<ffffffff8024172a>] lock_acquire+0x4a/0x69
   [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
   [<ffffffff881b7a16>] ib_modify_mad+0x27/0x14c [ib_mad]
   [<ffffffff881b7b45>] ib_cancel_mad+0xa/0xd [ib_mad]
   [<ffffffff881e60b8>] ib_sa_cancel_query+0x6a/0x73 [ib_sa]
   [<ffffffff881eefb5>] wait_for_mcast_join+0x35/0xd2 [ib_ipoib]
   [<ffffffff881ef385>] ipoib_mcast_stop_thread+0xa6/0xe6 [ib_ipoib]
   [<ffffffff881f07da>] ipoib_mcast_restart_task+0x4b/0x3d9 [ib_ipoib]
   [<ffffffff80238422>] run_workqueue+0xa0/0xf7
   [<ffffffff8023861d>] worker_thread+0xee/0x122
   [<ffffffff8023b661>] kthread+0xd0/0xfb
   [<ffffffff8020a5f1>] child_rip+0x7/0x12

  -> (modlist_lock){.+..} ops: 0 {
     initial-use  at:
                            [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                            [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                            [<ffffffff80247f0c>] module_text_address+0x15/0x3b
                            [<ffffffff803ee75b>] __register_kprobe+0x2bc/0x2da
                            [<ffffffff803ee8e3>] register_kprobe+0xc/0xf
                            [<ffffffff80806064>] arch_init_kprobes+0xf/0x12
                            [<ffffffff808094be>] init_kprobes+0x3f/0x52
                            [<ffffffff8020718b>] init+0x143/0x308
                            [<ffffffff8020a5f1>] child_rip+0x7/0x12
     in-softirq-W at:
                            [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                            [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                            [<ffffffff80247e1e>] is_module_address+0x14/0x80
                            [<ffffffff8023ede1>] static_obj+0x6c/0x79
                            [<ffffffff8023ee8b>] lockdep_init_map+0x7d/0xc3
                            [<ffffffff802f64d2>] __spin_lock_init+0x2e/0x58
                            [<ffffffff881ef221>] ipoib_mcast_alloc+0x97/0xce [ib_ipoib]
                            [<ffffffff881efe3b>] ipoib_mcast_send+0x109/0x413 [ib_ipoib]
                            [<ffffffff881eda0e>] ipoib_start_xmit+0x361/0x66d [ib_ipoib]
                            [<ffffffff8038dfc8>] dev_hard_start_xmit+0x1ab/0x221
                            [<ffffffff8039a024>] __qdisc_run+0xfa/0x1cd
                            [<ffffffff8038e174>] dev_queue_xmit+0x136/0x263
                            [<ffffffff80390209>] neigh_connected_output+0xae/0xc7
                            [<ffffffff8813f4de>] ip6_output2+0x254/0x28c [ipv6]
                            [<ffffffff8813fcf8>] ip6_output+0x7e2/0x7f8 [ipv6]
                            [<ffffffff8814d736>] ndisc_send_rs+0x33d/0x46f [ipv6]
                            [<ffffffff881427b9>] addrconf_dad_completed+0x90/0xe2 [ipv6]
                            [<ffffffff88144fc1>] addrconf_dad_timer+0x74/0x11e [ipv6]
                            [<ffffffff80231a8e>] run_timer_softirq+0x150/0x1dc
                            [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
                            [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                            [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                            [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
                            [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70
   }
   ... key      at: [<ffffffff804cfd98>] modlist_lock+0x18/0x40
  ... acquired at:
   [<ffffffff8024172a>] lock_acquire+0x4a/0x69
   [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
   [<ffffffff80247e1e>] is_module_address+0x14/0x80
   [<ffffffff8023ede1>] static_obj+0x6c/0x79
   [<ffffffff8023ee8b>] lockdep_init_map+0x7d/0xc3
   [<ffffffff802f64d2>] __spin_lock_init+0x2e/0x58
   [<ffffffff881ef221>] ipoib_mcast_alloc+0x97/0xce [ib_ipoib]
   [<ffffffff881f0903>] ipoib_mcast_restart_task+0x174/0x3d9 [ib_ipoib]
   [<ffffffff80238422>] run_workqueue+0xa0/0xf7
   [<ffffffff8023861d>] worker_thread+0xee/0x122
   [<ffffffff8023b661>] kthread+0xd0/0xfb
   [<ffffffff8020a5f1>] child_rip+0x7/0x12

  -> (&qp->sq.lock){.+..} ops: 0 {
     initial-use  at:
                            [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                            [<ffffffff803eb644>] _spin_lock_irq+0x28/0x35
                            [<ffffffff881cdef3>] mthca_modify_qp+0x67/0xd7b [ib_mthca]
                            [<ffffffff881a68fa>] ib_modify_qp+0xc/0xf [ib_core]
                            [<ffffffff881b8394>] ib_mad_init_device+0x2db/0x55c [ib_mad]
                            [<ffffffff881a83fe>] ib_register_device+0x20d/0x300 [ib_core]
                            [<ffffffff881d2f53>] mthca_register_device+0x3e0/0x430 [ib_mthca]
                            [<ffffffff881c4f26>] mthca_init_one+0xbc2/0xcbc [ib_mthca]
                            [<ffffffff802fcab2>] pci_device_probe+0x4b/0x72
                            [<ffffffff803481b7>] driver_probe_device+0x59/0xaf
                            [<ffffffff803482d3>] __driver_attach+0x58/0x90
                            [<ffffffff80347665>] bus_for_each_dev+0x48/0x7a
                            [<ffffffff80348047>] driver_attach+0x1b/0x1e
                            [<ffffffff803479d2>] bus_add_driver+0x74/0x112
                            [<ffffffff80348705>] driver_register+0x8c/0x91
                            [<ffffffff802fc684>] __pci_register_driver+0x60/0x84
                            [<ffffffff88022016>] 0xffffffff88022016
                            [<ffffffff80247a93>] sys_init_module+0x174c/0x1884
                            [<ffffffff802096a1>] system_call+0x7d/0x83
     in-softirq-W at:
                            [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                            [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                            [<ffffffff881cff9d>] mthca_arbel_post_send+0x2f/0x5be [ib_mthca]
                            [<ffffffff881ee043>] ipoib_send+0x117/0x1d6 [ib_ipoib]
                            [<ffffffff881f012d>] ipoib_mcast_send+0x3fb/0x413 [ib_ipoib]
                            [<ffffffff881eda0e>] ipoib_start_xmit+0x361/0x66d [ib_ipoib]
                            [<ffffffff8038dfc8>] dev_hard_start_xmit+0x1ab/0x221
                            [<ffffffff8039a024>] __qdisc_run+0xfa/0x1cd
                            [<ffffffff8038e174>] dev_queue_xmit+0x136/0x263
                            [<ffffffff80390209>] neigh_connected_output+0xae/0xc7
                            [<ffffffff8813f4de>] ip6_output2+0x254/0x28c [ipv6]
                            [<ffffffff8813fcf8>] ip6_output+0x7e2/0x7f8 [ipv6]
                            [<ffffffff8814dbf7>] ndisc_send_ns+0x38f/0x4c1 [ipv6]
                            [<ffffffff88145048>] addrconf_dad_timer+0xfb/0x11e [ipv6]
                            [<ffffffff80231a8e>] run_timer_softirq+0x150/0x1dc
                            [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
                            [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                            [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                            [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
                            [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70
   }
   ... key      at: [<ffffffff881e4060>] __key.20374+0x0/0xffffffffffff231a [ib_mthca]
   -> (&qp->rq.lock){+...} ops: 0 {
      initial-use  at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb316>] _spin_lock+0x21/0x2e
                              [<ffffffff881cdf02>] mthca_modify_qp+0x76/0xd7b [ib_mthca]
                              [<ffffffff881a68fa>] ib_modify_qp+0xc/0xf [ib_core]
                              [<ffffffff881b8394>] ib_mad_init_device+0x2db/0x55c [ib_mad]
                              [<ffffffff881a83fe>] ib_register_device+0x20d/0x300 [ib_core]
                              [<ffffffff881d2f53>] mthca_register_device+0x3e0/0x430 [ib_mthca]
                              [<ffffffff881c4f26>] mthca_init_one+0xbc2/0xcbc [ib_mthca]
                              [<ffffffff802fcab2>] pci_device_probe+0x4b/0x72
                              [<ffffffff803481b7>] driver_probe_device+0x59/0xaf
                              [<ffffffff803482d3>] __driver_attach+0x58/0x90
                              [<ffffffff80347665>] bus_for_each_dev+0x48/0x7a
                              [<ffffffff80348047>] driver_attach+0x1b/0x1e
                              [<ffffffff803479d2>] bus_add_driver+0x74/0x112
                              [<ffffffff80348705>] driver_register+0x8c/0x91
                              [<ffffffff802fc684>] __pci_register_driver+0x60/0x84
                              [<ffffffff88022016>] 0xffffffff88022016
                              [<ffffffff80247a93>] sys_init_module+0x174c/0x1884
                              [<ffffffff802096a1>] system_call+0x7d/0x83
      in-hardirq-W at:
                              [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                              [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                              [<ffffffff881cd749>] mthca_arbel_post_receive+0x35/0x23a [ib_mthca]
                              [<ffffffff881ee183>] ipoib_ib_post_receive+0x81/0xf9 [ib_ipoib]
                              [<ffffffff881ee55b>] ipoib_ib_completion+0x286/0x3ff [ib_ipoib]
                              [<ffffffff881ca8a5>] mthca_cq_completion+0x65/0x6b [ib_mthca]
                              [<ffffffff881ca1bc>] mthca_eq_int+0x70/0x3d2 [ib_mthca]
                              [<ffffffff881ca644>] mthca_arbel_interrupt+0x3b/0x98 [ib_mthca]
                              [<ffffffff802537de>] handle_IRQ_event+0x28/0x64
                              [<ffffffff802538c6>] __do_IRQ+0xac/0x117
                              [<ffffffff8020becf>] do_IRQ+0xf7/0x108
                              [<ffffffff80209bd8>] common_interrupt+0x64/0x65
    }
    ... key      at: [<ffffffff881e4058>] __key.20375+0x0/0xffffffffffff2322 [ib_mthca]
   ... acquired at:
   [<ffffffff8024172a>] lock_acquire+0x4a/0x69
   [<ffffffff803eb316>] _spin_lock+0x21/0x2e
   [<ffffffff881cdf02>] mthca_modify_qp+0x76/0xd7b [ib_mthca]
   [<ffffffff881a68fa>] ib_modify_qp+0xc/0xf [ib_core]
   [<ffffffff881b8394>] ib_mad_init_device+0x2db/0x55c [ib_mad]
   [<ffffffff881a83fe>] ib_register_device+0x20d/0x300 [ib_core]
   [<ffffffff881d2f53>] mthca_register_device+0x3e0/0x430 [ib_mthca]
   [<ffffffff881c4f26>] mthca_init_one+0xbc2/0xcbc [ib_mthca]
   [<ffffffff802fcab2>] pci_device_probe+0x4b/0x72
   [<ffffffff803481b7>] driver_probe_device+0x59/0xaf
   [<ffffffff803482d3>] __driver_attach+0x58/0x90
   [<ffffffff80347665>] bus_for_each_dev+0x48/0x7a
   [<ffffffff80348047>] driver_attach+0x1b/0x1e
   [<ffffffff803479d2>] bus_add_driver+0x74/0x112
   [<ffffffff80348705>] driver_register+0x8c/0x91
   [<ffffffff802fc684>] __pci_register_driver+0x60/0x84
   [<ffffffff88022016>] 0xffffffff88022016
   [<ffffffff80247a93>] sys_init_module+0x174c/0x1884
   [<ffffffff802096a1>] system_call+0x7d/0x83

... acquired at:

   [<ffffffff8024172a>] lock_acquire+0x4a/0x69
   [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
   [<ffffffff881cff9d>] mthca_arbel_post_send+0x2f/0x5be [ib_mthca]
   [<ffffffff881ee043>] ipoib_send+0x117/0x1d6 [ib_ipoib]
   [<ffffffff881f012d>] ipoib_mcast_send+0x3fb/0x413 [ib_ipoib]
   [<ffffffff881eda0e>] ipoib_start_xmit+0x361/0x66d [ib_ipoib]
   [<ffffffff8038dfc8>] dev_hard_start_xmit+0x1ab/0x221
   [<ffffffff8039a024>] __qdisc_run+0xfa/0x1cd
   [<ffffffff8038e174>] dev_queue_xmit+0x136/0x263
   [<ffffffff80390209>] neigh_connected_output+0xae/0xc7
   [<ffffffff8813f4de>] ip6_output2+0x254/0x28c [ipv6]
   [<ffffffff8813fcf8>] ip6_output+0x7e2/0x7f8 [ipv6]
   [<ffffffff8814dbf7>] ndisc_send_ns+0x38f/0x4c1 [ipv6]
   [<ffffffff88145048>] addrconf_dad_timer+0xfb/0x11e [ipv6]
   [<ffffffff80231a8e>] run_timer_softirq+0x150/0x1dc
   [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
   [<ffffffff8020a94d>] call_softirq+0x1d/0x28
   [<ffffffff8022e0bc>] irq_exit+0x56/0x59
   [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
   [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70

... acquired at:

   [<ffffffff8024172a>] lock_acquire+0x4a/0x69
   [<ffffffff803eb316>] _spin_lock+0x21/0x2e
   [<ffffffff881efd5a>] ipoib_mcast_send+0x28/0x413 [ib_ipoib]
   [<ffffffff881eda0e>] ipoib_start_xmit+0x361/0x66d [ib_ipoib]
   [<ffffffff8038dfc8>] dev_hard_start_xmit+0x1ab/0x221
   [<ffffffff8039a024>] __qdisc_run+0xfa/0x1cd
   [<ffffffff8038e174>] dev_queue_xmit+0x136/0x263
   [<ffffffff80390209>] neigh_connected_output+0xae/0xc7
   [<ffffffff8813f4de>] ip6_output2+0x254/0x28c [ipv6]
   [<ffffffff8813fcf8>] ip6_output+0x7e2/0x7f8 [ipv6]
   [<ffffffff8814dbf7>] ndisc_send_ns+0x38f/0x4c1 [ipv6]
   [<ffffffff88145048>] addrconf_dad_timer+0xfb/0x11e [ipv6]
   [<ffffffff80231a8e>] run_timer_softirq+0x150/0x1dc
   [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
   [<ffffffff8020a94d>] call_softirq+0x1d/0x28
   [<ffffffff8022e0bc>] irq_exit+0x56/0x59
   [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
   [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70

the hard-irq-unsafe lock's dependencies:
-> (&skb_queue_lock_key){-+..} ops: 0 {
initial-use at:

                        [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                        [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                        [<ffffffff8038683d>] skb_queue_tail+0x1c/0x47
                        [<ffffffff8039ff72>] netlink_broadcast+0x212/0x2ea
                        [<ffffffff802e57c4>] kobject_uevent+0x3c9/0x43d
                        [<ffffffff8034897f>] store_uevent+0x16/0x1e
                        [<ffffffff803488ef>] class_device_attr_store+0x1b/0x1e
                        [<ffffffff802b0c33>] sysfs_write_file+0xb7/0xe4
                        [<ffffffff8027448e>] vfs_write+0xad/0x154
                        [<ffffffff80274d8b>] sys_write+0x46/0x6f
                        [<ffffffff802096a1>] system_call+0x7d/0x83
   in-softirq-W at:
                        [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                        [<ffffffff803eb7da>] _spin_lock_irqsave+0x2b/0x3c
                        [<ffffffff80386aca>] skb_dequeue+0x18/0x5c
                        [<ffffffff80387015>] skb_queue_purge+0x18/0x26
                        [<ffffffff80394261>] neigh_timer_handler+0x26f/0x346
                        [<ffffffff80231a8e>] run_timer_softirq+0x150/0x1dc
                        [<ffffffff8022e345>] __do_softirq+0x6b/0xf7
                        [<ffffffff8020a94d>] call_softirq+0x1d/0x28
                        [<ffffffff8022e0bc>] irq_exit+0x56/0x59
                        [<ffffffff802146ee>] smp_apic_timer_interrupt+0x59/0x5f
                        [<ffffffff8020a2c1>] apic_timer_interrupt+0x69/0x70
   hardirq-on-W at:
                        [<ffffffff8024172a>] lock_acquire+0x4a/0x69
                        [<ffffffff803eb349>] _spin_lock_bh+0x26/0x33
                        [<ffffffff803c6de8>] udp_ioctl+0x46/0x87
                        [<ffffffff803cd1ab>] inet_ioctl+0x8c/0x8f
                        [<ffffffff8038203c>] sock_ioctl+0x1c0/0x1ea
                        [<ffffffff8028591e>] do_ioctl+0x26/0x74
                        [<ffffffff80285bb6>] vfs_ioctl+0x24a/0x264
                        [<ffffffff80285c11>] sys_ioctl+0x41/0x68
                        [<ffffffff802096a1>] system_call+0x7d/0x83
 }
 ... key      at: [<ffffffff807ccac8>] skb_queue_lock_key+0x0/0x18

stack backtrace:

Call Trace:
 [<ffffffff8020ac99>] show_trace+0xaa/0x238
 [<ffffffff8020b037>] dump_stack+0x13/0x15
 [<ffffffff8023ff9e>] check_usage+0x282/0x293
 [<ffffffff802412b7>] __lock_acquire+0x85c/0xa29
 [<ffffffff8024172b>] lock_acquire+0x4b/0x69
 [<ffffffff803eb7db>] _spin_lock_irqsave+0x2c/0x3c
 [<ffffffff8038683e>] skb_queue_tail+0x1d/0x47
 [<ffffffff881efecc>] :ib_ipoib:ipoib_mcast_send+0x19a/0x413
 [<ffffffff881eda0f>] :ib_ipoib:ipoib_start_xmit+0x362/0x66d
 [<ffffffff8038dfc9>] dev_hard_start_xmit+0x1ac/0x221
 [<ffffffff8039a025>] __qdisc_run+0xfb/0x1cd
 [<ffffffff8038e175>] dev_queue_xmit+0x137/0x263
 [<ffffffff8039020a>] neigh_connected_output+0xaf/0xc7
 [<ffffffff8813f4df>] :ipv6:ip6_output2+0x255/0x28c
 [<ffffffff8813fcf9>] :ipv6:ip6_output+0x7e3/0x7f8
 [<ffffffff8814d737>] :ipv6:ndisc_send_rs+0x33e/0x46f
 [<ffffffff881427ba>] :ipv6:addrconf_dad_completed+0x91/0xe2
 [<ffffffff88144fc2>] :ipv6:addrconf_dad_timer+0x75/0x11e
 [<ffffffff80231a8f>] run_timer_softirq+0x151/0x1dc
 [<ffffffff8022e346>] __do_softirq+0x6c/0xf7
 [<ffffffff8020a94e>] call_softirq+0x1e/0x28

ib0: no IPv6 routers present

Arjan van de Ven

2006-07-12 06:49 UTC #30 lore raw

Quoted 5 lines · Roland Dreier

OK, the patch to lib/idr.c below is at least one way to fix this. However, with that applied I get the lockdep warning below, which seems to be a false positive -- I'm not sure what the right fix is, but the blame really seems to fall on udp_ioctl for poking into the sk_buff_head lock itself.

this does not have to be a false positive!
It is not legal to take ANY non-hardirq safe lock after having taken a lock that's used in hardirq context.
(having said that the skb_queue_tail lock needs a special treatment for some real false positives; Linus merged that already)

Quoted 8 lines

And here's the warning I get, which appears to be a false positive:
======================================================
[ INFO: hard-safe -> hard-unsafe lock order detected ]
------------------------------------------------------
swapper/0 [HC0[0]:SC1[2]:HE0:SE0] is trying to acquire:
(&skb_queue_lock_key){-+..}, at: [<ffffffff8038683e>] skb_queue_tail+0x1d/0x47

Quoted 5 lines

and this task is already holding:
(&priv->lock){.+..}, at: [<ffffffff881efd5b>] ipoib_mcast_send+0x29/0x413 [ib_ipoib]
which would create a new lock dependency:
(&priv->lock){.+..} -> (&skb_queue_lock_key){-+..}

Lets call priv->lock lock A, and the skb_queue_lock lock B to keep this short

CPU 0 CPU 1
soft irq context user or softirq context

take lock B
(anywhere in net stack)
take lock A
(ipoib_mcast_send)

interrupt happens

take lock A (spins)
in ISR somewhere

take lock B (spins)
(in skb_queue_tail() called from ipoib_mcast_send)

Now this assumes your queue is shared with the networking stack,
so that the first "take lock B" on CPU 0 can happen without disabling interrupts. (as several of the SKB functions do that get used by the networking stack, but not
the simple ones used by drivers for private queues).
If this queue used in ipoib_mcast_send() is a private queue you probably want to apply the patch below (already in/on its way to mainline)

include/linux/skbuff.h +8 -1

Index: linux-2.6.18-rc1/include/linux/skbuff.h
===================================================================
@@ -606,10 +606,17 @@ static inline __u32 skb_queue_len(const 
 
 extern struct lock_class_key skb_queue_lock_key;
 
+/*
+ * This function creates a split out lock class for each invocation;
+ * this is needed for now since a whole lot of users of the skb-queue
+ * infrastructure in drivers have different locking usage (in hardirq)
+ * than the networking core (in softirq only). In the long run either the
+ * network layer or drivers should need annotation to consolidate the
+ * main types of usage into 3 classes.
+ */
 static inline void skb_queue_head_init(struct sk_buff_head *list)
 {
 	spin_lock_init(&list->lock);
-	lockdep_set_class(&list->lock, &skb_queue_lock_key);
 	list->prev = list->next = (struct sk_buff *)list;
 	list->qlen = 0;
 }

Roland Dreier

2006-07-12 19:01 UTC #31 lore raw

Quoted 5 lines

this does not have to be a false positive!
It is not legal to take ANY non-hardirq safe lock after having taken a lock that's used in hardirq context.
(having said that the skb_queue_tail lock needs a special treatment for some real false positives; Linus merged that already)

...

Quoted 1 line

Now this assumes your queue is shared with the networking stack,

Right, understood -- but ipoib has a private skb queue. Which is why I said it was a false positive.

Thanks,
Roland