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

From: Andrew Hendry
Date: Thu Jan 27 2011 - 05:07:52 EST

Next message: Srikar Dronamraju: "Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes:register/unregister probes."
Previous message: Vincent Guittot: "Re: [PATCH 2/2] tracing, perf : add cpu hotplug trace events"
In reply to: Arnd Bergmann: "[PATCH 12/20] x25: remove the BKL"
Next in thread: Arnd Bergmann: "Re: [PATCH 12/20] x25: remove the BKL"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <arnd@xxxxxxxx> wrote:
>
> 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@xxxxxxxx>
> Cc: Andrew Hendry <andrew.hendry@xxxxxxxxx>
> Cc: linux-x25@xxxxxxxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Srikar Dronamraju: "Re: [RFC] [PATCH 2.6.37-rc5-tip 5/20] 5: Uprobes:register/unregister probes."
Previous message: Vincent Guittot: "Re: [PATCH 2/2] tracing, perf : add cpu hotplug trace events"
In reply to: Arnd Bergmann: "[PATCH 12/20] x25: remove the BKL"
Next in thread: Arnd Bergmann: "Re: [PATCH 12/20] x25: remove the BKL"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]