On second look it doesn't seem to be a problem of the ieee1394 stack but
rather of underlying skb and net infrastructure.
BTW, the locking in -mm's net/unix/af_unix.c::unix_stream_connect()
differs a bit from stock unix_stream_connect(). I see spin_lock_bh() in
2.6.17-rc5-mm3 where 2.6.17-rc5 has spin_lock().
(added Cc: netdev)
--
Stefan Richter
-=====-=-==- -==- --==-
http://arcgraph.de/sr/
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2006-06-07 07:13:22
On Tue, Jun 06, 2006 at 04:39:21PM +0000, Stefan Richter wrote:
BTW, the locking in -mm's net/unix/af_unix.c::unix_stream_connect()
differs a bit from stock unix_stream_connect(). I see spin_lock_bh() in
2.6.17-rc5-mm3 where 2.6.17-rc5 has spin_lock().
Hi Ingo:
Looks like this change was introduced by the validator patch. Any idea
why this was done? AF_UNIX is a user-space-driven socket so there shouldn't
be any need for BH to be disabled there.
Even if it does this patch should go through the normal channels rather
than the lock validator.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, Jun 06, 2006 at 04:39:21PM +0000, Stefan Richter wrote:
quoted
BTW, the locking in -mm's net/unix/af_unix.c::unix_stream_connect()
differs a bit from stock unix_stream_connect(). I see spin_lock_bh() in
2.6.17-rc5-mm3 where 2.6.17-rc5 has spin_lock().
Hi Ingo:
Looks like this change was introduced by the validator patch. Any
idea why this was done? AF_UNIX is a user-space-driven socket so there
shouldn't be any need for BH to be disabled there.
yeah. I'll investigate - it's quite likely that sk_receive_queue.lock
will have to get per-address family locking rules - right?
Maybe it's enough to introduce a separate key for AF_UNIX alone (and
still having all other protocols share the locking rules for
sk_receive_queue.lock) , by reinitializing its spinlock after
sock_init_data()?
Ingo
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2006-06-12 06:42:23
On Mon, Jun 12, 2006 at 08:38:07AM +0200, Ingo Molnar wrote:
yeah. I'll investigate - it's quite likely that sk_receive_queue.lock
will have to get per-address family locking rules - right?
Yes that's the issue.
Maybe it's enough to introduce a separate key for AF_UNIX alone (and
still having all other protocols share the locking rules for
sk_receive_queue.lock) , by reinitializing its spinlock after
sock_init_data()?
yeah. I'll investigate - it's quite likely that sk_receive_queue.lock
will have to get per-address family locking rules - right?
That's right.
Maybe it's enough to introduce a separate key for AF_UNIX alone (and
still having all other protocols share the locking rules for
sk_receive_queue.lock) , by reinitializing its spinlock after
sock_init_data()?
AF_NETLINK and/or AF_PACKET might be in a similar situation
as AF_UNIX.
Maybe it's enough to introduce a separate key for AF_UNIX alone (and
still having all other protocols share the locking rules for
sk_receive_queue.lock) , by reinitializing its spinlock after
sock_init_data()?
This could work. AF_UNIX is probably the only family that does not
interact with hardware.
ok, great. The patch below does the trick on my box.
regarding your point wrt. path of integration - it is pretty much the
only practical way to do this centrally as part of the lock validator
patches, but to collect ACKs from subsystem maintainers in the process.
So if you like it i'd like to have your ACK but this patch depends on
the other lock validator patches (and only makes sense together with
them), so they should temporarily stay in the lock validator queue.
Hopefully this wont be a state that lasts too long and once the
validator is upstream, all patches of course go via the subsystem
submission rules.
(the #ifdef LOCKDEP should probably be converted to some sort of
lockdep_split_lock_key(&sk->sk_receive_queue.lock) op - i'll do that
later)
Ingo
------
Subject: undo AF_UNIX _bh locking changes and split lock-type instead
From: Ingo Molnar <redacted>
this cleans up lock-validator-special-locking-af_unix.patch: instead
of adding _bh locking to AF_UNIX, this patch splits their
sk_receive_queue.lock type from the other networking skb-queue locks.
Signed-off-by: Ingo Molnar <redacted>
---
net/unix/af_unix.c | 17 +++++++++++++----
net/unix/garbage.c | 8 ++++----
2 files changed, 17 insertions(+), 8 deletions(-)
Index: linux/net/unix/af_unix.c
===================================================================
@@ -1073,12 +1082,12 @@ restart:unix_state_wunlock(sk);/* take ten and and send info to listening sock */-spin_lock_bh(&other->sk_receive_queue.lock);+spin_lock(&other->sk_receive_queue.lock);__skb_queue_tail(&other->sk_receive_queue,skb);/* Undo artificially decreased inflight after embrion*isinstalledtolisteningsocket.*/atomic_inc(&newu->inflight);-spin_unlock_bh(&other->sk_receive_queue.lock);+spin_unlock(&other->sk_receive_queue.lock);unix_state_runlock(other);other->sk_data_ready(other,0);sock_put(other);
@@ -1843,7 +1852,7 @@ static int unix_ioctl(struct socket *socbreak;}-spin_lock_bh(&sk->sk_receive_queue.lock);+spin_lock(&sk->sk_receive_queue.lock);if(sk->sk_type==SOCK_STREAM||sk->sk_type==SOCK_SEQPACKET){skb_queue_walk(&sk->sk_receive_queue,skb)
@@ -1853,7 +1862,7 @@ static int unix_ioctl(struct socket *socif(skb)amount=skb->len;}-spin_unlock_bh(&sk->sk_receive_queue.lock);+spin_unlock(&sk->sk_receive_queue.lock);err=put_user(amount,(int__user*)arg);break;}
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2006-06-12 07:04:32
On Mon, Jun 12, 2006 at 08:57:01AM +0200, Ingo Molnar wrote:
regarding your point wrt. path of integration - it is pretty much the
only practical way to do this centrally as part of the lock validator
patches, but to collect ACKs from subsystem maintainers in the process.
So if you like it i'd like to have your ACK but this patch depends on
the other lock validator patches (and only makes sense together with
them), so they should temporarily stay in the lock validator queue.
Hopefully this wont be a state that lasts too long and once the
validator is upstream, all patches of course go via the subsystem
submission rules.
Obviously as long as Dave is happy with it then it's fine. However,
it's probably a good idea to cc netdev for relevant patches so that
they get a wider review. If you've already sent this one there then
I apologise for missing it :)
(the #ifdef LOCKDEP should probably be converted to some sort of
lockdep_split_lock_key(&sk->sk_receive_queue.lock) op - i'll do that
later)
From: David Miller <davem@davemloft.net> Date: 2006-06-12 07:18:23
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 12 Jun 2006 17:03:56 +1000
On Mon, Jun 12, 2006 at 08:57:01AM +0200, Ingo Molnar wrote:
quoted
regarding your point wrt. path of integration - it is pretty much the
only practical way to do this centrally as part of the lock validator
patches, but to collect ACKs from subsystem maintainers in the process.
So if you like it i'd like to have your ACK but this patch depends on
the other lock validator patches (and only makes sense together with
them), so they should temporarily stay in the lock validator queue.
Hopefully this wont be a state that lasts too long and once the
validator is upstream, all patches of course go via the subsystem
submission rules.
Obviously as long as Dave is happy with it then it's fine. However,
it's probably a good idea to cc netdev for relevant patches so that
they get a wider review. If you've already sent this one there then
I apologise for missing it :)
(the #ifdef LOCKDEP should probably be converted to some sort of
lockdep_split_lock_key(&sk->sk_receive_queue.lock) op - i'll do that
later)
i've added such an op, lockdep_reinit_lock_key() - this makes the patch
cleaner:
----------------------
Subject: undo AF_UNIX _bh locking changes and split lock-type
From: Ingo Molnar <redacted>
this cleans up lock-validator-special-locking-af_unix.patch: instead
of adding _bh locking to AF_UNIX, this patch splits their
sk_receive_queue.lock type from the other networking skb-queue locks.
Signed-off-by: Ingo Molnar <redacted>
---
net/unix/af_unix.c | 18 ++++++++++++++----
net/unix/garbage.c | 8 ++++----
2 files changed, 18 insertions(+), 8 deletions(-)
Index: linux/net/unix/af_unix.c
===================================================================
@@ -1073,12 +1083,12 @@ restart:unix_state_wunlock(sk);/* take ten and and send info to listening sock */-spin_lock_bh(&other->sk_receive_queue.lock);+spin_lock(&other->sk_receive_queue.lock);__skb_queue_tail(&other->sk_receive_queue,skb);/* Undo artificially decreased inflight after embrion*isinstalledtolisteningsocket.*/atomic_inc(&newu->inflight);-spin_unlock_bh(&other->sk_receive_queue.lock);+spin_unlock(&other->sk_receive_queue.lock);unix_state_runlock(other);other->sk_data_ready(other,0);sock_put(other);
@@ -1843,7 +1853,7 @@ static int unix_ioctl(struct socket *socbreak;}-spin_lock_bh(&sk->sk_receive_queue.lock);+spin_lock(&sk->sk_receive_queue.lock);if(sk->sk_type==SOCK_STREAM||sk->sk_type==SOCK_SEQPACKET){skb_queue_walk(&sk->sk_receive_queue,skb)
@@ -1853,7 +1863,7 @@ static int unix_ioctl(struct socket *socif(skb)amount=skb->len;}-spin_unlock_bh(&sk->sk_receive_queue.lock);+spin_unlock(&sk->sk_receive_queue.lock);err=put_user(amount,(int__user*)arg);break;}
Subject: undo AF_UNIX _bh locking changes and split lock-type
From: Ingo Molnar <redacted>
this cleans up lock-validator-special-locking-af_unix.patch: instead
of adding _bh locking to AF_UNIX, this patch splits their
sk_receive_queue.lock type from the other networking skb-queue locks.
Signed-off-by: Ingo Molnar <redacted>
Signed-off-by: David S. Miller <davem@davemloft.net>