Thread (17 messages) 17 messages, 7 authors, 2011-01-27

Re: [PATCH 12/20] x25: remove the BKL

From: Andrew Hendry <hidden>
Date: 2011-01-27 10:07:49
Also in: lkml

Left it running and put about 3.0G through x.25, it was running fine
until after about 20 hours.
I was stopping the test programs and hit this.

Jan 27 20:18:34 jaunty kernel: [80403.945790] PGD 1d8b00067 PUD 1ddec3067 PMD 0
Jan 27 20:18:34 jaunty kernel: [80403.945836] CPU 3
Jan 27 20:18:34 jaunty kernel: [80403.945842] Modules linked in: x25
nls_cp437 cifs binfmt_misc kvm_intel kvm snd_hda_codec_via
snd_usb_audio snd_hda_intel snd_hda_codec nouveau snd_pcm_oss
snd_mixer_oss snd_pcm snd_seq_dummy snd_hwdep snd_usbmidi_lib
snd_seq_oss snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq
psmouse snd_timer snd_seq_device serio_raw fbcon ttm tileblit font
bitblit softcursor xhci_hcd drm_kms_helper snd drm asus_atk0110
soundcore snd_page_alloc i2c_algo_bit video usbhid hid usb_storage
r8169 pata_jmicron ahci mii libahci
Jan 27 20:18:34 jaunty kernel: [80403.946026]
Jan 27 20:18:34 jaunty kernel: [80403.946034] Pid: 28187, comm:
x25echotest Not tainted 2.6.38-rc2+ #41 P7P55D-E PRO/System Product
Name
Jan 27 20:18:34 jaunty kernel: [80403.946050] RIP:
0010:[<ffffffffa026f197>]  [<ffffffffa026f197>]
x25_sendmsg+0x1a7/0x530 [x25]
Jan 27 20:18:34 jaunty kernel: [80403.946072] RSP:
0018:ffff880228dbfcb8  EFLAGS: 00010246
Jan 27 20:18:34 jaunty kernel: [80403.946083] RAX: 0000000000000080
RBX: ffff880228dbfd70 RCX: ffff880228dbfce4
Jan 27 20:18:34 jaunty kernel: [80403.946096] RDX: 00000000fffffe00
RSI: 0000000000000000 RDI: ffff8801ba89f050
Jan 27 20:18:34 jaunty kernel: [80403.946109] RBP: ffff880228dbfd18
R08: ffff88022aa91000 R09: 0000000000000000
Jan 27 20:18:34 jaunty kernel: [80403.946482] R10: 0000000000000000
R11: 0000000000000000 R12: ffff8801ba89f000
Jan 27 20:18:34 jaunty kernel: [80403.946495] R13: 0000000000000000
R14: 0000000000000000 R15: 0000000000000000
Jan 27 20:18:34 jaunty kernel: [80403.946509] FS:
00007f09b3013700(0000) GS:ffff8800bf460000(0000)
knlGS:0000000000000000
Jan 27 20:18:34 jaunty kernel: [80403.946523] CS:  0010 DS: 0000 ES:
0000 CR0: 000000008005003b
Jan 27 20:18:34 jaunty kernel: [80403.946534] CR2: 00000000000000b4
CR3: 00000001df992000 CR4: 00000000000006e0
Jan 27 20:18:34 jaunty kernel: [80403.946547] DR0: 0000000000000000
DR1: 0000000000000000 DR2: 0000000000000000
Jan 27 20:18:34 jaunty kernel: [80403.946560] DR3: 0000000000000000
DR6: 00000000ffff0ff0 DR7: 0000000000000400
Jan 27 20:18:34 jaunty kernel: [80403.946574] Process x25echotest
(pid: 28187, threadinfo ffff880228dbe000, task ffff8801d89bc320)
Jan 27 20:18:34 jaunty kernel: [80403.946594]  ffff880200000008
0000000000000016 0000303030390009 0000000000000000
Jan 27 20:18:34 jaunty kernel: [80403.946616]  ffff880228db0000
fffffe00d8832450 0000000000000000 ffff880228dbfd38
Jan 27 20:18:34 jaunty kernel: [80403.946638]  ffff880228dbfec8
ffff880228dbfdf8 ffff8801de73b980 ffff880228dbfd70
Jan 27 20:18:34 jaunty kernel: [80403.946671]  [<ffffffff8140cdd3>]
sock_aio_write+0x183/0x1a0
Jan 27 20:18:34 jaunty kernel: [80403.946686]  [<ffffffff8110304c>] ?
__pte_alloc+0xdc/0x100
Jan 27 20:18:34 jaunty kernel: [80403.946700]  [<ffffffff81138a5a>]
do_sync_write+0xda/0x120
Jan 27 20:18:34 jaunty kernel: [80403.946713]  [<ffffffff8140d026>] ?
move_addr_to_user+0x86/0xa0
Jan 27 20:18:34 jaunty kernel: [80403.946729]  [<ffffffff812431a3>] ?
security_file_permission+0x23/0x90
Jan 27 20:18:34 jaunty kernel: [80403.946743]  [<ffffffff8113903e>]
vfs_write+0x15e/0x180
Jan 27 20:18:34 jaunty kernel: [80403.946757]  [<ffffffff81139151>]
sys_write+0x51/0x90
Jan 27 20:18:34 jaunty kernel: [80403.946771]  [<ffffffff8100bf42>]
system_call_fastpath+0x16/0x1b
Jan 27 20:18:34 jaunty kernel: [80403.946973]  RSP <ffff880228dbfcb8>
Jan 27 20:18:34 jaunty kernel: [80403.950010] ---[ end trace
36cd53b6ce0d6f4b ]---

If i have done it right, x25_sendmsg+0x1a7/0x530 is the skb_reserve
which gets inlined here.
(af_x25.c)
        /* Build a packet */
        SOCK_DEBUG(sk, "x25_sendmsg: sendto: building packet.\n");

        if ((msg->msg_flags & MSG_OOB) && len > 32)
                len = 32;

        size = len + X25_MAX_L2_LEN + X25_EXT_MIN_LEN;

        release_sock(sk);
        skb = sock_alloc_send_skb(sk, size, noblock, &rc);
        lock_sock(sk);

        X25_SKB_CB(skb)->flags = msg->msg_flags;

        skb_reserve(skb, X25_MAX_L2_LEN + X25_EXT_MIN_LEN);

        /*
         *      Put the data on the end
         */
        SOCK_DEBUG(sk, "x25_sendmsg: Copying user data\n");



objdump -dS show it at 2197 here.

static inline void skb_reserve(struct sk_buff *skb, int len)
{
        skb->data += len;
        skb->tail += len;
    2197:       41 83 87 b4 00 00 00    addl   $0x16,0xb4(%r15) <---
    219e:       16
    219f:       41 89 47 28             mov    %eax,0x28(%r15)
    21a3:       49 8b 87 c8 00 00 00    mov    0xc8(%r15),%rax
    21aa:       48 83 c0 16             add    $0x16,%rax
        skb_reserve(skb, X25_MAX_L2_LEN + X25_EXT_MIN_LEN);

But im not sure where to go from there...



On Wed, Jan 26, 2011 at 9:17 AM, Arnd Bergmann [off-list ref] wrote:
quoted hunk ↗ jump to hunk
This replaces all instances of lock_kernel in x25
with lock_sock, taking care to release the socket
lock around sleeping functions (sock_alloc_send_skb
and skb_recv_datagram). It is not clear whether
this is a correct solution, but it seem to be what
other protocols do in the same situation.

Compile-tested only.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Andrew Hendry <redacted>
Cc: linux-x25@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 net/x25/Kconfig   |    1 -
 net/x25/af_x25.c  |   61 ++++++++++++++++------------------------------------
 net/x25/x25_out.c |    7 ++++-
 3 files changed, 24 insertions(+), 45 deletions(-)
diff --git a/net/x25/Kconfig b/net/x25/Kconfig
index 2196e55..e6759c9 100644
--- a/net/x25/Kconfig
+++ b/net/x25/Kconfig
@@ -5,7 +5,6 @@
 config X25
       tristate "CCITT X.25 Packet Layer (EXPERIMENTAL)"
       depends on EXPERIMENTAL
-       depends on BKL # should be fixable
       ---help---
         X.25 is a set of standardized network protocols, similar in scope to
         frame relay; the one physical line from your box to the X.25 network
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index ad96ee9..8f5d1bb 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -40,7 +40,6 @@
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
-#include <linux/smp_lock.h>
 #include <linux/timer.h>
 #include <linux/string.h>
 #include <linux/net.h>
@@ -432,15 +431,6 @@ void x25_destroy_socket_from_timer(struct sock *sk)
       sock_put(sk);
 }

-static void x25_destroy_socket(struct sock *sk)
-{
-       sock_hold(sk);
-       lock_sock(sk);
-       __x25_destroy_socket(sk);
-       release_sock(sk);
-       sock_put(sk);
-}
-
 /*
 *     Handling for system calls applied via the various interfaces to a
 *     X.25 socket object.
@@ -647,18 +637,19 @@ static int x25_release(struct socket *sock)
       struct sock *sk = sock->sk;
       struct x25_sock *x25;

-       lock_kernel();
       if (!sk)
-               goto out;
+               return 0;

       x25 = x25_sk(sk);

+       sock_hold(sk);
+       lock_sock(sk);
       switch (x25->state) {

               case X25_STATE_0:
               case X25_STATE_2:
                       x25_disconnect(sk, 0, 0, 0);
-                       x25_destroy_socket(sk);
+                       __x25_destroy_socket(sk);
                       goto out;

               case X25_STATE_1:
@@ -678,7 +669,8 @@ static int x25_release(struct socket *sock)
       sock_orphan(sk);
 out:
-       unlock_kernel();
+       release_sock(sk);
+       sock_put(sk);
       return 0;
 }
@@ -1085,7 +1077,7 @@ static int x25_sendmsg(struct kiocb *iocb, struct socket *sock,
       size_t size;
       int qbit = 0, rc = -EINVAL;

-       lock_kernel();
+       lock_sock(sk);
       if (msg->msg_flags & ~(MSG_DONTWAIT|MSG_OOB|MSG_EOR|MSG_CMSG_COMPAT))
               goto out;
@@ -1148,9 +1140,10 @@ static int x25_sendmsg(struct kiocb *iocb, struct socket *sock,
       size = len + X25_MAX_L2_LEN + X25_EXT_MIN_LEN;

+       release_sock(sk);
       skb = sock_alloc_send_skb(sk, size, noblock, &rc);
-       if (!skb)
-               goto out;
+       lock_sock(sk);
+
       X25_SKB_CB(skb)->flags = msg->msg_flags;

       skb_reserve(skb, X25_MAX_L2_LEN + X25_EXT_MIN_LEN);
@@ -1231,26 +1224,10 @@ static int x25_sendmsg(struct kiocb *iocb, struct socket *sock,
                       len++;
       }

-       /*
-        * lock_sock() is currently only used to serialize this x25_kick()
-        * against input-driven x25_kick() calls. It currently only blocks
-        * incoming packets for this socket and does not protect against
-        * any other socket state changes and is not called from anywhere
-        * else. As x25_kick() cannot block and as long as all socket
-        * operations are BKL-wrapped, we don't need take to care about
-        * purging the backlog queue in x25_release().
-        *
-        * Using lock_sock() to protect all socket operations entirely
-        * (and making the whole x25 stack SMP aware) unfortunately would
-        * require major changes to {send,recv}msg and skb allocation methods.
-        * -> 2.5 ;)
-        */
-       lock_sock(sk);
       x25_kick(sk);
-       release_sock(sk);
       rc = len;
 out:
-       unlock_kernel();
+       release_sock(sk);
       return rc;
 out_kfree_skb:
       kfree_skb(skb);
@@ -1271,7 +1248,7 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock,
       unsigned char *asmptr;
       int rc = -ENOTCONN;

-       lock_kernel();
+       lock_sock(sk);
       /*
        * This works for seqpacket too. The receiver has ordered the queue for
        * us! We do one quick check first though
@@ -1300,8 +1277,10 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock,
               msg->msg_flags |= MSG_OOB;
       } else {
               /* Now we can treat all alike */
+               release_sock(sk);
               skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
                                       flags & MSG_DONTWAIT, &rc);
+               lock_sock(sk);
               if (!skb)
                       goto out;
@@ -1338,14 +1317,12 @@ static int x25_recvmsg(struct kiocb *iocb, struct socket *sock,
       msg->msg_namelen = sizeof(struct sockaddr_x25);

-       lock_sock(sk);
       x25_check_rbuf(sk);
-       release_sock(sk);
       rc = copied;
 out_free_dgram:
       skb_free_datagram(sk, skb);
 out:
-       unlock_kernel();
+       release_sock(sk);
       return rc;
 }
@@ -1581,18 +1558,18 @@ out_cud_release:
               case SIOCX25CALLACCPTAPPRV: {
                       rc = -EINVAL;
-                       lock_kernel();
+                       lock_sock(sk);
                       if (sk->sk_state != TCP_CLOSE)
                               break;
                       clear_bit(X25_ACCPT_APPRV_FLAG, &x25->flags);
-                       unlock_kernel();
+                       release_sock(sk);
                       rc = 0;
                       break;
               }

               case SIOCX25SENDCALLACCPT:  {
                       rc = -EINVAL;
-                       lock_kernel();
+                       lock_sock(sk);
                       if (sk->sk_state != TCP_ESTABLISHED)
                               break;
                       /* must call accptapprv above */
@@ -1600,7 +1577,7 @@ out_cud_release:
                               break;
                       x25_write_internal(sk, X25_CALL_ACCEPTED);
                       x25->state = X25_STATE_3;
-                       unlock_kernel();
+                       release_sock(sk);
                       rc = 0;
                       break;
               }
diff --git a/net/x25/x25_out.c b/net/x25/x25_out.c
index d00649f..f1a6ff1 100644
--- a/net/x25/x25_out.c
+++ b/net/x25/x25_out.c
@@ -68,8 +68,11 @@ int x25_output(struct sock *sk, struct sk_buff *skb)
               frontlen = skb_headroom(skb);

               while (skb->len > 0) {
-                       if ((skbn = sock_alloc_send_skb(sk, frontlen + max_len,
-                                                       noblock, &err)) == NULL){
+                       release_sock(sk);
+                       skbn = sock_alloc_send_skb(sk, frontlen + max_len,
+                                                  1, &err);
+                       lock_sock(sk);
+                       if (!skbn) {
                               if (err == -EWOULDBLOCK && noblock){
                                       kfree_skb(skb);
                                       return sent;
--
1.7.1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help