* Re: possible recursive locking in ATM layer [not found] <200607041759.43064.duncan.sands@math.u-psud.fr> @ 2006-07-04 16:13 ` Arjan van de Ven 2006-07-05 10:23 ` Duncan Sands 2006-07-05 13:33 ` Avi Kivity 0 siblings, 2 replies; 4+ messages in thread From: Arjan van de Ven @ 2006-07-04 16:13 UTC (permalink / raw) To: Duncan Sands; +Cc: netdev, linux-kernel, Ingo Molnar, chas From: Arjan van de Ven <arjan@linux.intel.com> > Linux version 2.6.17-git22 (duncan@baldrick) (gcc version 4.0.3 (Ubuntu 4.0.3-1ubuntu5)) #20 PREEMPT Tue Jul 4 10:35:04 CEST 2006 > > [ 2381.598609] ============================================= > [ 2381.619314] [ INFO: possible recursive locking detected ] > [ 2381.635497] --------------------------------------------- > [ 2381.651706] atmarpd/2696 is trying to acquire lock: > [ 2381.666354] (&skb_queue_lock_key){-+..}, at: [<c028c540>] skb_migrate+0x24/0x6c > [ 2381.688848] ok this is a real potential deadlock in a way, it takes two locks of 2 skbuffs without doing any kind of lock ordering; I think the following patch should fix it. Just sort the lock taking order by address of the skb.. it's not pretty but it's the best this can do in a minimally invasive way. I still agree with the comment that this code shouldn't live in the atm layer... Signed-off-by: Arjan van de Ven <arjan@linux.intel.com> --- net/atm/ipcommon.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) Index: linux-2.6.17-mm6/net/atm/ipcommon.c =================================================================== --- linux-2.6.17-mm6.orig/net/atm/ipcommon.c +++ linux-2.6.17-mm6/net/atm/ipcommon.c @@ -25,8 +25,8 @@ /* * skb_migrate appends the list at "from" to "to", emptying "from" in the * process. skb_migrate is atomic with respect to all other skb operations on - * "from" and "to". Note that it locks both lists at the same time, so beware - * of potential deadlocks. + * "from" and "to". Note that it locks both lists at the same time, so to deal + * with the lock ordering, the locks are taken in address order. * * This function should live in skbuff.c or skbuff.h. */ @@ -39,8 +39,13 @@ void skb_migrate(struct sk_buff_head *fr struct sk_buff *skb_to = (struct sk_buff *) to; struct sk_buff *prev; - spin_lock_irqsave(&from->lock,flags); - spin_lock(&to->lock); + if (from<to) { + spin_lock_irqsave(&from->lock,flags); + spin_lock_nested(&to->lock, SINGLE_DEPTH_NESTING); + } else { + spin_lock_irqsave(&to->lock, flags); + spin_lock_nested(&from->lock, SINGLE_DEPTH_NESTING); + } prev = from->prev; from->next->prev = to->prev; prev->next = skb_to; ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: possible recursive locking in ATM layer 2006-07-04 16:13 ` possible recursive locking in ATM layer Arjan van de Ven @ 2006-07-05 10:23 ` Duncan Sands 2006-07-05 13:33 ` Avi Kivity 1 sibling, 0 replies; 4+ messages in thread From: Duncan Sands @ 2006-07-05 10:23 UTC (permalink / raw) To: Arjan van de Ven; +Cc: netdev, linux-kernel, Ingo Molnar, chas > ok this is a real potential deadlock in a way, it takes two locks of 2 > skbuffs without doing any kind of lock ordering; I think the following > patch should fix it. Just sort the lock taking order by address of the > skb.. it's not pretty but it's the best this can do in a minimally > invasive way. The patch is effective. Thanks for making it! Best wishes, Duncan. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: possible recursive locking in ATM layer 2006-07-04 16:13 ` possible recursive locking in ATM layer Arjan van de Ven 2006-07-05 10:23 ` Duncan Sands @ 2006-07-05 13:33 ` Avi Kivity 2006-07-05 13:42 ` Arjan van de Ven 1 sibling, 1 reply; 4+ messages in thread From: Avi Kivity @ 2006-07-05 13:33 UTC (permalink / raw) To: Arjan van de Ven; +Cc: Duncan Sands, netdev, linux-kernel, Ingo Molnar, chas Arjan van de Ven wrote: > > From: Arjan van de Ven <arjan@linux.intel.com> > > > Linux version 2.6.17-git22 (duncan@baldrick) (gcc version 4.0.3 > (Ubuntu 4.0.3-1ubuntu5)) #20 PREEMPT Tue Jul 4 10:35:04 CEST 2006 > > > > > [ 2381.598609] ============================================= > > [ 2381.619314] [ INFO: possible recursive locking detected ] > > [ 2381.635497] --------------------------------------------- > > [ 2381.651706] atmarpd/2696 is trying to acquire lock: > > [ 2381.666354] (&skb_queue_lock_key){-+..}, at: [<c028c540>] > skb_migrate+0x24/0x6c > > [ 2381.688848] > > > ok this is a real potential deadlock in a way, it takes two locks of 2 > skbuffs without doing any kind of lock ordering; I think the following > patch should fix it. Just sort the lock taking order by address of the > skb.. it's not pretty but it's the best this can do in a minimally > invasive way. > Isn't it a deadlock only if skb_migrate(a, b) and skb_migrate(b, a) can be called concurrently? -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: possible recursive locking in ATM layer 2006-07-05 13:33 ` Avi Kivity @ 2006-07-05 13:42 ` Arjan van de Ven 0 siblings, 0 replies; 4+ messages in thread From: Arjan van de Ven @ 2006-07-05 13:42 UTC (permalink / raw) To: Avi Kivity; +Cc: Duncan Sands, netdev, linux-kernel, Ingo Molnar, chas On Wed, 2006-07-05 at 16:33 +0300, Avi Kivity wrote: > Arjan van de Ven wrote: > > > > From: Arjan van de Ven <arjan@linux.intel.com> > > > > > Linux version 2.6.17-git22 (duncan@baldrick) (gcc version 4.0.3 > > (Ubuntu 4.0.3-1ubuntu5)) #20 PREEMPT Tue Jul 4 10:35:04 CEST 2006 > > > > > > > > [ 2381.598609] ============================================= > > > [ 2381.619314] [ INFO: possible recursive locking detected ] > > > [ 2381.635497] --------------------------------------------- > > > [ 2381.651706] atmarpd/2696 is trying to acquire lock: > > > [ 2381.666354] (&skb_queue_lock_key){-+..}, at: [<c028c540>] > > skb_migrate+0x24/0x6c > > > [ 2381.688848] > > > > > > ok this is a real potential deadlock in a way, it takes two locks of 2 > > skbuffs without doing any kind of lock ordering; I think the following > > patch should fix it. Just sort the lock taking order by address of the > > skb.. it's not pretty but it's the best this can do in a minimally > > invasive way. > > > > Isn't it a deadlock only if skb_migrate(a, b) and skb_migrate(b, a) can > be called concurrently? yes, well, and if there are no other double-takers... > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-07-05 13:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200607041759.43064.duncan.sands@math.u-psud.fr>
2006-07-04 16:13 ` possible recursive locking in ATM layer Arjan van de Ven
2006-07-05 10:23 ` Duncan Sands
2006-07-05 13:33 ` Avi Kivity
2006-07-05 13:42 ` Arjan van de Ven
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).