From: Andrew Hendry <andrew.hendry@gmail.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: linux-kernel@vger.kernel.org, linux-x25@vger.kernel.org,
netdev@vger.kernel.org
Subject: Re: [PATCH 12/20] x25: remove the BKL
Date: Thu, 27 Jan 2011 21:07:44 +1100 [thread overview]
Message-ID: <AANLkTim=d4Tok-ri1BozZUfBFS=rofaodN8ZBA5AjKKd@mail.gmail.com> (raw)
In-Reply-To: <1295993854-4971-13-git-send-email-arnd@arndb.de>
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@arndb.de> 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@arndb.de>
> Cc: Andrew Hendry <andrew.hendry@gmail.com>
> 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
>
next prev parent reply other threads:[~2011-01-27 10:07 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-01-25 22:17 [RFC 00/20] Proposal for remaining BKL users Arnd Bergmann
2011-01-25 22:17 ` Arnd Bergmann
2011-01-25 22:17 ` [PATCH 01/20] drm/i810: remove the BKL Arnd Bergmann
2011-01-25 22:17 ` [PATCH 02/20] drm: remove i830 driver Arnd Bergmann
2011-01-25 22:17 ` [PATCH 03/20] staging/usbip: convert to kthread Arnd Bergmann
2011-01-28 17:53 ` Max Vozeler
2011-01-28 18:48 ` Arnd Bergmann
2011-03-01 22:15 ` Arnd Bergmann
2011-01-25 22:17 ` [PATCH 04/20] staging/cx25721: serialize access to devlist Arnd Bergmann
2011-01-26 16:23 ` Palash Bandyopadhyay
2011-01-31 21:37 ` Greg KH
2011-01-25 22:17 ` [PATCH 05/20] staging/go7007: remove the BKL Arnd Bergmann
2011-01-25 22:17 ` [PATCH 06/20] staging: Remove autofs3 Arnd Bergmann
2011-01-26 7:41 ` H. Peter Anvin
2011-01-25 22:17 ` [PATCH 07/20] staging: remove smbfs Arnd Bergmann
2011-01-25 22:17 ` [PATCH 08/20] adfs: remove the big kernel lock Arnd Bergmann
2011-01-25 22:20 ` Russell King
2011-01-25 22:17 ` [PATCH 09/20] hpfs: rename big kernel lock to hpfs_lock Arnd Bergmann
2011-01-25 22:17 ` [PATCH 10/20] hpfs: replace BKL with a global mutex Arnd Bergmann
2011-01-26 0:15 ` Andi Kleen
2011-01-26 0:19 ` Andi Kleen
2011-01-26 12:48 ` [PATCH v2] hpfs: remove the BKL Arnd Bergmann
2011-01-26 12:50 ` [PATCH 10/20] hpfs: replace BKL with a global mutex Arnd Bergmann
2011-01-26 16:52 ` Andi Kleen
2011-01-27 5:01 ` Nick Piggin
2011-01-27 10:57 ` Miklos Szeredi
2011-01-25 22:17 ` [PATCH 11/20] hpfs: move to drivers/staging Arnd Bergmann
2011-02-07 16:17 ` Mikulas Patocka
2011-02-07 19:31 ` Arnd Bergmann
2011-01-25 22:17 ` [PATCH 12/20] x25: remove the BKL Arnd Bergmann
2011-01-27 10:07 ` Andrew Hendry [this message]
2011-01-27 12:17 ` Arnd Bergmann
2011-01-27 12:38 ` [PATCH v2] " Arnd Bergmann
2011-01-27 13:20 ` Eric Dumazet
2011-01-27 13:43 ` Arnd Bergmann
2011-01-25 22:17 ` [PATCH 13/20] appletalk: move to staging Arnd Bergmann
2011-01-25 22:17 ` [PATCH 14/20] staging/appletalk: remove the BKL Arnd Bergmann
2011-01-25 22:29 ` David Miller
2011-01-26 12:57 ` Arnd Bergmann
2011-01-25 22:17 ` [PATCH 15/20] ufs: " Arnd Bergmann
2011-01-26 2:30 ` Nick Bowler
2011-01-26 12:53 ` Arnd Bergmann
2011-01-27 5:47 ` Nick Piggin
2011-01-27 13:13 ` Arnd Bergmann
2011-01-25 22:17 ` [PATCH 16/20] ipx: " Arnd Bergmann
2011-01-25 22:17 ` [PATCH 17/20] tracing: don't trace " Arnd Bergmann
2011-01-25 22:28 ` Frederic Weisbecker
2011-01-25 22:17 ` [PATCH 18/20] rtmutex-tester: remove BKL tests Arnd Bergmann
2011-01-26 15:00 ` [tip:core/locking] rtmutex-tester: Remove " tip-bot for Arnd Bergmann
2011-02-22 20:57 ` [tip:irq/core] rtmutex: tester: " tip-bot for Arnd Bergmann
2011-01-25 22:17 ` [PATCH 19/20] drivers: remove extraneous includes of smp_lock.h Arnd Bergmann
2011-01-25 22:17 ` [PATCH 20/20] BKL: That's all, folks Arnd Bergmann
2011-01-26 6:19 ` Ingo Molnar
2011-01-26 8:47 ` Alan Cox
2011-01-26 11:01 ` Ingo Molnar
2011-01-26 11:22 ` Thomas Gleixner
2011-01-26 2:22 ` [RFC 00/20] Proposal for remaining BKL users Greg KH
2011-01-26 2:22 ` Greg KH
2011-01-26 11:31 ` Arnd Bergmann
2011-01-26 11:31 ` Arnd Bergmann
2011-01-26 11:58 ` Mauro Carvalho Chehab
2011-01-26 13:45 ` Arnd Bergmann
2011-01-26 13:45 ` Arnd Bergmann
2011-01-26 13:45 ` Arnd Bergmann
2011-01-26 16:24 ` Palash Bandyopadhyay
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='AANLkTim=d4Tok-ri1BozZUfBFS=rofaodN8ZBA5AjKKd@mail.gmail.com' \
--to=andrew.hendry@gmail.com \
--cc=arnd@arndb.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-x25@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.