* [RFC 00/20] Proposal for remaining BKL users
@ 2011-01-25 22:17 Arnd Bergmann
2011-01-25 22:17 ` [PATCH 12/20] x25: remove the BKL Arnd Bergmann
2011-01-26 2:22 ` [RFC 00/20] Proposal for remaining BKL users Greg KH
0 siblings, 2 replies; 12+ messages in thread
From: Arnd Bergmann @ 2011-01-25 22:17 UTC (permalink / raw)
To: linux-kernel
Cc: Mauro Carvalho Chehab, Frederic Weisbecker, dri-devel,
Mikulas Patocka, H. Peter Anvin, Ian Kent, linux-cifs,
Nick Bowler, Jeff Layton, Takahiro Hirofuchi, Ross Cohen,
Arnaldo Carvalho de Melo, Evgeniy Dushistov, Arnd Bergmann,
Stuart Swales, Thomas Gleixner, Arjan van de Ven, autofs,
linux-x25, netdev, Greg Kroah-Hartman, Palash Bandyopadhyay,
linux-fsdevel, A
I've gone through all the code in the kernel that
uses the big kernel lock and come up with a solution
that seems at least half-reasonable for each of them.
The decisions are somewhat arbitrary, but here is
what I'd suggest we do:
* Remove in 2.6.39:
i830, autofs3, smbfs
* Move to staging now, kill in 2.6.41 (or later):
appletalk, hpfs
* Work around in an ugly way, but keep alive:
* ufs, ipx, i810, cx25721
* Fix properly:
* usbip, go7007, adfs, x25
Some of the patches are rather tricky and I haven't
really done much proper testing, so I'd much prefer
the maintainers to pick up the patches and do the
necessary testing where possible, or even come up
with a better solution.
Arnd Bergmann (20):
drm/i810: remove the BKL
drm: remove i830 driver
staging/usbip: convert to kthread
staging/cx25721: serialize access to devlist
staging/go7007: remove the BKL
staging: Remove autofs3
staging: remove smbfs
adfs: remove the big kernel lock
hpfs: rename big kernel lock to hpfs_lock
hpfs: replace BKL with a global mutex
hpfs: move to drivers/staging
x25: remove the BKL
appletalk: move to staging
staging/appletalk: remove the BKL
ufs: remove the BKL
ipx: remove the BKL
tracing: don't trace the BKL
rtmutex-tester: remove BKL tests
drivers: remove extraneous includes of smp_lock.h
BKL: That's all, folks
MAINTAINERS | 11 +-
drivers/gpu/drm/Kconfig | 47 +-
drivers/gpu/drm/Makefile | 1 -
drivers/gpu/drm/i810/i810_dma.c | 18 +-
drivers/gpu/drm/i810/i810_drv.c | 6 +-
drivers/gpu/drm/i830/Makefile | 8 -
drivers/gpu/drm/i830/i830_dma.c | 1560 ---------
drivers/gpu/drm/i830/i830_drv.c | 107 -
drivers/gpu/drm/i830/i830_drv.h | 295 --
drivers/gpu/drm/i830/i830_irq.c | 186 --
drivers/net/Makefile | 1 -
drivers/net/appletalk/Makefile | 7 -
drivers/scsi/megaraid/megaraid_sas_fp.c | 1 -
drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 -
drivers/staging/Kconfig | 8 +-
drivers/staging/Makefile | 4 +-
drivers/{net => staging}/appletalk/Kconfig | 1 -
{net => drivers/staging}/appletalk/Makefile | 7 +-
{net => drivers/staging}/appletalk/aarp.c | 2 +-
.../linux => drivers/staging/appletalk}/atalk.h | 0
{net => drivers/staging}/appletalk/atalk_proc.c | 2 +-
drivers/{net => staging}/appletalk/cops.c | 2 +-
drivers/{net => staging}/appletalk/cops.h | 0
drivers/{net => staging}/appletalk/cops_ffdrv.h | 0
drivers/{net => staging}/appletalk/cops_ltdrv.h | 0
{net => drivers/staging}/appletalk/ddp.c | 44 +-
{net => drivers/staging}/appletalk/dev.c | 0
drivers/{net => staging}/appletalk/ipddp.c | 2 +-
drivers/{net => staging}/appletalk/ipddp.h | 0
drivers/{net => staging}/appletalk/ltpc.c | 2 +-
drivers/{net => staging}/appletalk/ltpc.h | 0
.../staging}/appletalk/sysctl_net_atalk.c | 2 +-
drivers/staging/autofs/Kconfig | 22 -
drivers/staging/autofs/Makefile | 7 -
drivers/staging/autofs/TODO | 8 -
drivers/staging/autofs/autofs_i.h | 165 -
drivers/staging/autofs/dirhash.c | 260 --
drivers/staging/autofs/init.c | 52 -
drivers/staging/autofs/inode.c | 288 --
drivers/staging/autofs/root.c | 648 ----
drivers/staging/autofs/symlink.c | 26 -
drivers/staging/autofs/waitq.c | 205 --
drivers/staging/cx25821/Kconfig | 1 -
drivers/staging/cx25821/cx25821-alsa.c | 2 +
drivers/staging/cx25821/cx25821-core.c | 16 +-
drivers/staging/cx25821/cx25821-video.c | 9 +-
drivers/staging/cx25821/cx25821.h | 3 +-
drivers/staging/easycap/easycap.h | 1 -
drivers/staging/easycap/easycap_ioctl.c | 1 -
drivers/staging/go7007/Kconfig | 1 -
drivers/staging/go7007/s2250-loader.c | 3 -
{fs => drivers/staging}/hpfs/Kconfig | 5 +-
{fs => drivers/staging}/hpfs/Makefile | 0
drivers/staging/hpfs/TODO | 5 +
{fs => drivers/staging}/hpfs/alloc.c | 0
{fs => drivers/staging}/hpfs/anode.c | 0
{fs => drivers/staging}/hpfs/buffer.c | 0
{fs => drivers/staging}/hpfs/dentry.c | 0
{fs => drivers/staging}/hpfs/dir.c | 23 +-
{fs => drivers/staging}/hpfs/dnode.c | 0
{fs => drivers/staging}/hpfs/ea.c | 0
{fs => drivers/staging}/hpfs/file.c | 9 +-
{fs => drivers/staging}/hpfs/hpfs.h | 0
{fs => drivers/staging}/hpfs/hpfs_fn.h | 36 +
{fs => drivers/staging}/hpfs/inode.c | 9 +-
{fs => drivers/staging}/hpfs/map.c | 0
{fs => drivers/staging}/hpfs/name.c | 0
{fs => drivers/staging}/hpfs/namei.c | 49 +-
{fs => drivers/staging}/hpfs/super.c | 21 +-
drivers/staging/smbfs/Kconfig | 56 -
drivers/staging/smbfs/Makefile | 18 -
drivers/staging/smbfs/TODO | 8 -
drivers/staging/smbfs/cache.c | 208 --
drivers/staging/smbfs/dir.c | 699 ----
drivers/staging/smbfs/file.c | 456 ---
drivers/staging/smbfs/getopt.c | 64 -
drivers/staging/smbfs/getopt.h | 14 -
drivers/staging/smbfs/inode.c | 854 -----
drivers/staging/smbfs/ioctl.c | 68 -
drivers/staging/smbfs/proc.c | 3502 --------------------
drivers/staging/smbfs/proto.h | 89 -
drivers/staging/smbfs/request.c | 817 -----
drivers/staging/smbfs/request.h | 70 -
drivers/staging/smbfs/smb.h | 118 -
drivers/staging/smbfs/smb_debug.h | 34 -
drivers/staging/smbfs/smb_fs.h | 153 -
drivers/staging/smbfs/smb_fs_i.h | 37 -
drivers/staging/smbfs/smb_fs_sb.h | 100 -
drivers/staging/smbfs/smb_mount.h | 65 -
drivers/staging/smbfs/smbfs.txt | 8 -
drivers/staging/smbfs/smbiod.c | 343 --
drivers/staging/smbfs/smbno.h | 363 --
drivers/staging/smbfs/sock.c | 385 ---
drivers/staging/smbfs/symlink.c | 67 -
drivers/staging/usbip/Kconfig | 2 +-
drivers/staging/usbip/stub.h | 4 +-
drivers/staging/usbip/stub_dev.c | 13 +-
drivers/staging/usbip/stub_rx.c | 13 +-
drivers/staging/usbip/stub_tx.c | 14 +-
drivers/staging/usbip/usbip_common.c | 105 -
drivers/staging/usbip/usbip_common.h | 20 +-
drivers/staging/usbip/usbip_event.c | 31 +-
drivers/staging/usbip/vhci.h | 4 +-
drivers/staging/usbip/vhci_hcd.c | 10 +-
drivers/staging/usbip/vhci_rx.c | 16 +-
drivers/staging/usbip/vhci_sysfs.c | 9 +-
drivers/staging/usbip/vhci_tx.c | 14 +-
drivers/target/target_core_device.c | 1 -
drivers/target/target_core_fabric_lib.c | 1 -
drivers/target/target_core_file.c | 1 -
drivers/target/target_core_hba.c | 1 -
drivers/target/target_core_iblock.c | 1 -
drivers/target/target_core_pscsi.c | 1 -
drivers/target/target_core_rd.c | 1 -
drivers/target/target_core_tpg.c | 1 -
drivers/target/target_core_transport.c | 1 -
drivers/tty/n_hdlc.c | 1 -
drivers/tty/n_r3964.c | 1 -
drivers/tty/pty.c | 1 -
drivers/tty/tty_io.c | 1 -
drivers/tty/tty_ldisc.c | 2 -
drivers/tty/vt/selection.c | 1 -
drivers/tty/vt/vc_screen.c | 1 -
drivers/tty/vt/vt.c | 1 -
drivers/tty/vt/vt_ioctl.c | 1 -
fs/Kconfig | 1 -
fs/Makefile | 1 -
fs/adfs/Kconfig | 1 -
fs/adfs/dir.c | 6 -
fs/adfs/inode.c | 6 -
fs/adfs/super.c | 13 +-
fs/compat_ioctl.c | 1 -
fs/ufs/Kconfig | 1 -
fs/ufs/inode.c | 78 +-
fs/ufs/namei.c | 35 +-
fs/ufs/super.c | 55 +-
fs/ufs/truncate.c | 5 +-
fs/ufs/ufs.h | 6 +-
include/drm/Kbuild | 1 -
include/drm/i830_drm.h | 342 --
include/linux/Kbuild | 1 -
include/linux/hardirq.h | 9 +-
include/linux/smp_lock.h | 65 -
include/trace/events/bkl.h | 61 -
init/Kconfig | 5 -
kernel/rtmutex-tester.c | 39 +-
kernel/sched.c | 7 -
lib/Makefile | 1 -
lib/kernel_lock.c | 143 -
net/Kconfig | 1 -
net/Makefile | 1 -
net/ipx/Kconfig | 1 -
net/ipx/af_ipx.c | 52 +-
net/socket.c | 1 -
net/x25/Kconfig | 1 -
net/x25/af_x25.c | 61 +-
net/x25/x25_out.c | 7 +-
157 files changed, 356 insertions(+), 13722 deletions(-)
delete mode 100644 drivers/gpu/drm/i830/Makefile
delete mode 100644 drivers/gpu/drm/i830/i830_dma.c
delete mode 100644 drivers/gpu/drm/i830/i830_drv.c
delete mode 100644 drivers/gpu/drm/i830/i830_drv.h
delete mode 100644 drivers/gpu/drm/i830/i830_irq.c
delete mode 100644 drivers/net/appletalk/Makefile
rename drivers/{net => staging}/appletalk/Kconfig (98%)
rename {net => drivers/staging}/appletalk/Makefile (56%)
rename {net => drivers/staging}/appletalk/aarp.c (99%)
rename {include/linux => drivers/staging/appletalk}/atalk.h (100%)
rename {net => drivers/staging}/appletalk/atalk_proc.c (99%)
rename drivers/{net => staging}/appletalk/cops.c (99%)
rename drivers/{net => staging}/appletalk/cops.h (100%)
rename drivers/{net => staging}/appletalk/cops_ffdrv.h (100%)
rename drivers/{net => staging}/appletalk/cops_ltdrv.h (100%)
rename {net => drivers/staging}/appletalk/ddp.c (98%)
rename {net => drivers/staging}/appletalk/dev.c (100%)
rename drivers/{net => staging}/appletalk/ipddp.c (99%)
rename drivers/{net => staging}/appletalk/ipddp.h (100%)
rename drivers/{net => staging}/appletalk/ltpc.c (99%)
rename drivers/{net => staging}/appletalk/ltpc.h (100%)
rename {net => drivers/staging}/appletalk/sysctl_net_atalk.c (98%)
delete mode 100644 drivers/staging/autofs/Kconfig
delete mode 100644 drivers/staging/autofs/Makefile
delete mode 100644 drivers/staging/autofs/TODO
delete mode 100644 drivers/staging/autofs/autofs_i.h
delete mode 100644 drivers/staging/autofs/dirhash.c
delete mode 100644 drivers/staging/autofs/init.c
delete mode 100644 drivers/staging/autofs/inode.c
delete mode 100644 drivers/staging/autofs/root.c
delete mode 100644 drivers/staging/autofs/symlink.c
delete mode 100644 drivers/staging/autofs/waitq.c
rename {fs => drivers/staging}/hpfs/Kconfig (80%)
rename {fs => drivers/staging}/hpfs/Makefile (100%)
create mode 100644 drivers/staging/hpfs/TODO
rename {fs => drivers/staging}/hpfs/alloc.c (100%)
rename {fs => drivers/staging}/hpfs/anode.c (100%)
rename {fs => drivers/staging}/hpfs/buffer.c (100%)
rename {fs => drivers/staging}/hpfs/dentry.c (100%)
rename {fs => drivers/staging}/hpfs/dir.c (97%)
rename {fs => drivers/staging}/hpfs/dnode.c (100%)
rename {fs => drivers/staging}/hpfs/ea.c (100%)
rename {fs => drivers/staging}/hpfs/file.c (97%)
rename {fs => drivers/staging}/hpfs/hpfs.h (100%)
rename {fs => drivers/staging}/hpfs/hpfs_fn.h (91%)
rename {fs => drivers/staging}/hpfs/inode.c (98%)
rename {fs => drivers/staging}/hpfs/map.c (100%)
rename {fs => drivers/staging}/hpfs/name.c (100%)
rename {fs => drivers/staging}/hpfs/namei.c (96%)
rename {fs => drivers/staging}/hpfs/super.c (98%)
delete mode 100644 drivers/staging/smbfs/Kconfig
delete mode 100644 drivers/staging/smbfs/Makefile
delete mode 100644 drivers/staging/smbfs/TODO
delete mode 100644 drivers/staging/smbfs/cache.c
delete mode 100644 drivers/staging/smbfs/dir.c
delete mode 100644 drivers/staging/smbfs/file.c
delete mode 100644 drivers/staging/smbfs/getopt.c
delete mode 100644 drivers/staging/smbfs/getopt.h
delete mode 100644 drivers/staging/smbfs/inode.c
delete mode 100644 drivers/staging/smbfs/ioctl.c
delete mode 100644 drivers/staging/smbfs/proc.c
delete mode 100644 drivers/staging/smbfs/proto.h
delete mode 100644 drivers/staging/smbfs/request.c
delete mode 100644 drivers/staging/smbfs/request.h
delete mode 100644 drivers/staging/smbfs/smb.h
delete mode 100644 drivers/staging/smbfs/smb_debug.h
delete mode 100644 drivers/staging/smbfs/smb_fs.h
delete mode 100644 drivers/staging/smbfs/smb_fs_i.h
delete mode 100644 drivers/staging/smbfs/smb_fs_sb.h
delete mode 100644 drivers/staging/smbfs/smb_mount.h
delete mode 100644 drivers/staging/smbfs/smbfs.txt
delete mode 100644 drivers/staging/smbfs/smbiod.c
delete mode 100644 drivers/staging/smbfs/smbno.h
delete mode 100644 drivers/staging/smbfs/sock.c
delete mode 100644 drivers/staging/smbfs/symlink.c
delete mode 100644 include/drm/i830_drm.h
delete mode 100644 include/linux/smp_lock.h
delete mode 100644 include/trace/events/bkl.h
delete mode 100644 lib/kernel_lock.c
Cc: Andrew Hendry <andrew.hendry@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: autofs@linux.kernel.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@linux.ie>
Cc: David Airlie <airlied@linux.ie>
Cc: dri-devel@lists.freedesktop.org
Cc: Evgeniy Dushistov <dushistov@mail.ru>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ian Kent <raven@themaw.net>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Jeff Layton <jlayton@redhat.com>
Cc: linux-cifs@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-x25@vger.kernel.org
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>
Cc: netdev@vger.kernel.org
Cc: Nick Bowler <nbowler@elliptictech.com>
Cc: Palash Bandyopadhyay <palash.bandyopadhyay@conexant.com>
Cc: Ross Cohen <rcohen@snurgle.org>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: Stuart Swales <stuart.swales.croftnuisk@gmail.com>
Cc: Takahiro Hirofuchi <hirofuchi@users.sourceforge.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 12/20] x25: remove the BKL 2011-01-25 22:17 [RFC 00/20] Proposal for remaining BKL users Arnd Bergmann @ 2011-01-25 22:17 ` Arnd Bergmann 2011-01-27 10:07 ` Andrew Hendry 2011-01-26 2:22 ` [RFC 00/20] Proposal for remaining BKL users Greg KH 1 sibling, 1 reply; 12+ messages in thread From: Arnd Bergmann @ 2011-01-25 22:17 UTC (permalink / raw) To: linux-kernel; +Cc: Arnd Bergmann, Andrew Hendry, linux-x25, netdev 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 12/20] x25: remove the BKL 2011-01-25 22:17 ` [PATCH 12/20] x25: remove the BKL Arnd Bergmann @ 2011-01-27 10:07 ` Andrew Hendry 2011-01-27 12:17 ` Arnd Bergmann 0 siblings, 1 reply; 12+ messages in thread From: Andrew Hendry @ 2011-01-27 10:07 UTC (permalink / raw) To: Arnd Bergmann; +Cc: linux-kernel, linux-x25, netdev 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 12/20] x25: remove the BKL 2011-01-27 10:07 ` Andrew Hendry @ 2011-01-27 12:17 ` Arnd Bergmann 2011-01-27 12:38 ` [PATCH v2] " Arnd Bergmann 0 siblings, 1 reply; 12+ messages in thread From: Arnd Bergmann @ 2011-01-27 12:17 UTC (permalink / raw) To: Andrew Hendry; +Cc: linux-kernel, linux-x25, netdev On Thursday 27 January 2011, Andrew Hendry wrote: > 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 Is there no long above this about what problem was hit? There is normally one saying things like "Bug: unable to handle ..." Well, nevermind. It seems I could figure it out anyway: > 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 > ... > > 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; ok. > 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... It's pretty clear that %r15 is the skb in this, and from the registers in the dump, you can see that it's NULL. skb has just been returned from sock_alloc_send_skb, which means that this function failed. And indeed: > > @@ -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; I accidentally removed the error handling in my patch. No idea how that happened, it certainly wasn't intentional. Thanks a lot for the thorough testing and the detailed bug report! I'll follow up with a fixed patch that puts the error path back in. Arnd ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] x25: remove the BKL 2011-01-27 12:17 ` Arnd Bergmann @ 2011-01-27 12:38 ` Arnd Bergmann 2011-01-27 13:20 ` Eric Dumazet 0 siblings, 1 reply; 12+ messages in thread From: Arnd Bergmann @ 2011-01-27 12:38 UTC (permalink / raw) To: Andrew Hendry; +Cc: linux-kernel, linux-x25, netdev 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 --- v2: fix possible NULL-pointer dereference in x25_sendmsg net/x25/Kconfig | 1 - net/x25/af_x25.c | 58 ++++++++++++++++------------------------------------ net/x25/x25_out.c | 7 ++++- 3 files changed, 23 insertions(+), 43 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..4680b1e 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,7 +1140,9 @@ 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); + lock_sock(sk); if (!skb) goto out; X25_SKB_CB(skb)->flags = msg->msg_flags; @@ -1231,26 +1225,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 +1249,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 +1278,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 +1318,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 +1559,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 +1578,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; ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] x25: remove the BKL 2011-01-27 12:38 ` [PATCH v2] " Arnd Bergmann @ 2011-01-27 13:20 ` Eric Dumazet 2011-01-27 13:43 ` Arnd Bergmann 0 siblings, 1 reply; 12+ messages in thread From: Eric Dumazet @ 2011-01-27 13:20 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Andrew Hendry, linux-kernel, linux-x25, netdev Le jeudi 27 janvier 2011 à 13:38 +0100, Arnd Bergmann a écrit : > 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; This part looks strange : noblock variable became "const 1 : NOBLOCK" Why releasing socket if you dont block in sock_alloc_send_skb() ? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] x25: remove the BKL 2011-01-27 13:20 ` Eric Dumazet @ 2011-01-27 13:43 ` Arnd Bergmann 0 siblings, 0 replies; 12+ messages in thread From: Arnd Bergmann @ 2011-01-27 13:43 UTC (permalink / raw) To: Eric Dumazet; +Cc: Andrew Hendry, linux-kernel, linux-x25, netdev On Thursday 27 January 2011, Eric Dumazet wrote: > Le jeudi 27 janvier 2011 à 13:38 +0100, Arnd Bergmann a écrit : > > 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; > > This part looks strange : > > noblock variable became "const 1 : NOBLOCK" > > Why releasing socket if you dont block in sock_alloc_send_skb() ? Leftover from an earlier version of the patch, thanks for catching this! Originally, I wrote this as long timeo = sock_sndtimeo(sk, noblock) do { skbn = sock_alloc_send_skb(sk, frontlen + max_len, 1, &err); if (skbn) break; release_sock(sk); timeo = sock_wait_for_wmem(sk, timeo); lock_sock(sk); } while (timeo); Then I forgot to flip it back after I noticed that other protocols also just call release_sock/lock_sock around sock_alloc_send_skb. I think I'd better go over the whole series and see if there are more things that got slightly broken... Arnd ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 00/20] Proposal for remaining BKL users 2011-01-25 22:17 [RFC 00/20] Proposal for remaining BKL users Arnd Bergmann 2011-01-25 22:17 ` [PATCH 12/20] x25: remove the BKL Arnd Bergmann @ 2011-01-26 2:22 ` Greg KH 2011-01-26 11:31 ` Arnd Bergmann 1 sibling, 1 reply; 12+ messages in thread From: Greg KH @ 2011-01-26 2:22 UTC (permalink / raw) To: Arnd Bergmann Cc: Mauro Carvalho Chehab, Frederic Weisbecker, dri-devel, Mikulas Patocka, H. Peter Anvin, Ian Kent, linux-cifs, Nick Bowler, linux-x25, Takahiro Hirofuchi, Ross Cohen, Arnaldo Carvalho de Melo, Evgeniy Dushistov, Stuart Swales, Thomas Gleixner, Arjan van de Ven, autofs, Jeff Layton, netdev, linux-kernel, Palash Bandyopadhyay, linux-fsdevel, Andrew Morton On Tue, Jan 25, 2011 at 11:17:14PM +0100, Arnd Bergmann wrote: > I've gone through all the code in the kernel that > uses the big kernel lock and come up with a solution > that seems at least half-reasonable for each of them. > > The decisions are somewhat arbitrary, but here is > what I'd suggest we do: > > * Remove in 2.6.39: > i830, autofs3, smbfs I thought some people really wanted to keep i830. Or was that i810? I'll drop autofs3 and smbfs, thanks. > * Move to staging now, kill in 2.6.41 (or later): > appletalk, hpfs Sounds good to me. > * Work around in an ugly way, but keep alive: > * ufs, ipx, i810, cx25721 > > * Fix properly: > * usbip, go7007, adfs, x25 Thanks for the usbip and go7007 patches, I'll queue them up. greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 00/20] Proposal for remaining BKL users 2011-01-26 2:22 ` [RFC 00/20] Proposal for remaining BKL users Greg KH @ 2011-01-26 11:31 ` Arnd Bergmann 2011-01-26 11:58 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 12+ messages in thread From: Arnd Bergmann @ 2011-01-26 11:31 UTC (permalink / raw) To: Greg KH Cc: Mauro Carvalho Chehab, Frederic Weisbecker, dri-devel, Mikulas Patocka, H. Peter Anvin, Ian Kent, linux-cifs, Nick Bowler, linux-x25, Takahiro Hirofuchi, Ross Cohen, Arnaldo Carvalho de Melo, Evgeniy Dushistov, Stuart Swales, Thomas Gleixner, Arjan van de Ven, autofs, Jeff Layton, netdev, linux-kernel, Palash Bandyopadhyay, linux-fsdevel, Andrew Morton On Wednesday 26 January 2011, Greg KH wrote: > On Tue, Jan 25, 2011 at 11:17:14PM +0100, Arnd Bergmann wrote: > > I've gone through all the code in the kernel that > > uses the big kernel lock and come up with a solution > > that seems at least half-reasonable for each of them. > > > > The decisions are somewhat arbitrary, but here is > > what I'd suggest we do: > > > > * Remove in 2.6.39: > > i830, autofs3, smbfs > > I thought some people really wanted to keep i830. Or was that i810? > I'll drop autofs3 and smbfs, thanks. i810 needs to be kept, i830 is obsolete, see the patch changelogs there. I assume that Dave Airlie will take both patches (i810 BKL removal and i830 removal) into his 2.6.39 queue. > > * Work around in an ugly way, but keep alive: > > * ufs, ipx, i810, cx25721 > > > > * Fix properly: > > * usbip, go7007, adfs, x25 > > Thanks for the usbip and go7007 patches, I'll queue them up. It would be good if you could also take the cx25721 patch. It's not the nicest one I've done, but it's a bug fix nonetheless. Thanks, Arnd ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 00/20] Proposal for remaining BKL users 2011-01-26 11:31 ` Arnd Bergmann @ 2011-01-26 11:58 ` Mauro Carvalho Chehab 2011-01-26 13:45 ` Arnd Bergmann 0 siblings, 1 reply; 12+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-26 11:58 UTC (permalink / raw) To: Arnd Bergmann Cc: Frederic Weisbecker, dri-devel, Mikulas Patocka, H. Peter Anvin, Ian Kent, linux-cifs, Nick Bowler, linux-x25, Takahiro Hirofuchi, Palash Bandyopadhyay, Ross Cohen, Arnaldo Carvalho de Melo, Evgeniy Dushistov, Stuart Swales, Thomas Gleixner, Arjan van de Ven, autofs, Jeff Layton, netdev, Greg KH, linux-kernel, Palash Bandyopadhyay, linux-fsdevel Em 26-01-2011 09:31, Arnd Bergmann escreveu: > On Wednesday 26 January 2011, Greg KH wrote: >> On Tue, Jan 25, 2011 at 11:17:14PM +0100, Arnd Bergmann wrote: >>> I've gone through all the code in the kernel that >>> uses the big kernel lock and come up with a solution >>> that seems at least half-reasonable for each of them. >>> >>> The decisions are somewhat arbitrary, but here is >>> what I'd suggest we do: >>> >>> * Remove in 2.6.39: >>> i830, autofs3, smbfs >> >> I thought some people really wanted to keep i830. Or was that i810? >> I'll drop autofs3 and smbfs, thanks. > > i810 needs to be kept, i830 is obsolete, see the patch changelogs > there. I assume that Dave Airlie will take both patches (i810 BKL > removal and i830 removal) into his 2.6.39 queue. > >>> * Work around in an ugly way, but keep alive: >>> * ufs, ipx, i810, cx25721 >>> >>> * Fix properly: >>> * usbip, go7007, adfs, x25 >> >> Thanks for the usbip and go7007 patches, I'll queue them up. > > It would be good if you could also take the cx25721 patch. It's > not the nicest one I've done, but it's a bug fix nonetheless. I guess you're meaning cx25821, right? Palash should take a look on it and review. This is a device that allows 12 simultaneous streams, so, I suspect that he'll need to do some changes at the locking schema, to avoid performance bottlenecks. Cheers, Mauro ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 00/20] Proposal for remaining BKL users 2011-01-26 11:58 ` Mauro Carvalho Chehab @ 2011-01-26 13:45 ` Arnd Bergmann 2011-01-26 16:24 ` Palash Bandyopadhyay 0 siblings, 1 reply; 12+ messages in thread From: Arnd Bergmann @ 2011-01-26 13:45 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Frederic Weisbecker, dri-devel, Mikulas Patocka, H. Peter Anvin, Ian Kent, linux-cifs, Nick Bowler, linux-x25, Takahiro Hirofuchi, Ross Cohen, Arnaldo Carvalho de Melo, Evgeniy Dushistov, Stuart Swales, Thomas Gleixner, Arjan van de Ven, autofs, Jeff Layton, netdev, Greg KH, linux-kernel, Palash Bandyopadhyay, linux-fsdevel, Andrew Morton, Andrew On Wednesday 26 January 2011, Mauro Carvalho Chehab wrote: > I guess you're meaning cx25821, right? Right, sorry for the typo. > Palash should take a look on it and review. This is a device that allows > 12 simultaneous streams, so, I suspect that he'll need to do some > changes at the locking schema, to avoid performance bottlenecks. I would be surprised if there was any measurable performance change. The BKL was used only in the open() function to iterate the device list, and that code is nowhere on a critical path, as far as I can tell. Arnd ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [RFC 00/20] Proposal for remaining BKL users 2011-01-26 13:45 ` Arnd Bergmann @ 2011-01-26 16:24 ` Palash Bandyopadhyay 0 siblings, 0 replies; 12+ messages in thread From: Palash Bandyopadhyay @ 2011-01-26 16:24 UTC (permalink / raw) To: Arnd Bergmann, Mauro Carvalho Chehab Cc: Frederic Weisbecker, dri-devel@lists.freedesktop.org, Mikulas Patocka, H. Peter Anvin, Ian Kent, linux-cifs@vger.kernel.org, Nick Bowler, linux-x25@vger.kernel.org, Takahiro Hirofuchi, Ross Cohen, Arnaldo Carvalho de Melo, Evgeniy Dushistov, Stuart Swales, Thomas Gleixner, Arjan van de Ven, autofs@linux.kernel.org, Jeff Layton, netdev@vger.kernel.org, Greg KH, linux-kernel I think it should be ok. If we do hit any performance issue, we'll revisit this. Thanks, Palash -----Original Message----- From: Arnd Bergmann [mailto:arnd@arndb.de] Sent: Wednesday, January 26, 2011 5:45 AM To: Mauro Carvalho Chehab Cc: Greg KH; linux-kernel@vger.kernel.org; Andrew Hendry; Andrew Morton; Arjan van de Ven; Arnaldo Carvalho de Melo; autofs@linux.kernel.org; Chris Wilson; Dave Airlie; dri-devel@lists.freedesktop.org; Evgeniy Dushistov; Frederic Weisbecker; H. Peter Anvin; Ian Kent; Ingo Molnar; Jeff Layton; linux-cifs@vger.kernel.org; linux-fsdevel@vger.kernel.org; linux-x25@vger.kernel.org; Mikulas Patocka; netdev@vger.kernel.org; Nick Bowler; Palash Bandyopadhyay; Ross Cohen; Russell King; Stuart Swales; Takahiro Hirofuchi; Thomas Gleixner Subject: Re: [RFC 00/20] Proposal for remaining BKL users On Wednesday 26 January 2011, Mauro Carvalho Chehab wrote: > I guess you're meaning cx25821, right? Right, sorry for the typo. > Palash should take a look on it and review. This is a device that allows > 12 simultaneous streams, so, I suspect that he'll need to do some > changes at the locking schema, to avoid performance bottlenecks. I would be surprised if there was any measurable performance change. The BKL was used only in the open() function to iterate the device list, and that code is nowhere on a critical path, as far as I can tell. Arnd Conexant E-mail Firewall (Conexant.Com) made the following annotations --------------------------------------------------------------------- ********************** Legal Disclaimer **************************** "This email may contain confidential and privileged material for the sole use of the intended recipient. Any unauthorized review, use or distribution by others is strictly prohibited. If you have received the message in error, please advise the sender by reply email and delete the message. Thank you." ********************************************************************** --------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-01-27 13:43 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-01-25 22:17 [RFC 00/20] Proposal for remaining BKL users Arnd Bergmann 2011-01-25 22:17 ` [PATCH 12/20] x25: remove the BKL Arnd Bergmann 2011-01-27 10:07 ` Andrew Hendry 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-26 2:22 ` [RFC 00/20] Proposal for remaining BKL users Greg KH 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 16:24 ` Palash Bandyopadhyay
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox