Killing the Big Kernel Lock II
in [Kernel]
|
Prev: Another Linux Patent Issued [7,684,347 method and apparatus for network packet capture distributed storage system]
Next: linux-next: build warning after merge of the tree
From: Andi Kleen on 28 Mar 2010 21:10 Andi Kleen <andi(a)firstfloor.org> writes: > Jiri Kosina <jkosina(a)suse.cz> writes: > >> On Wed, 24 Mar 2010, Arnd Bergmann wrote: >> >>> I've spent some time continuing the work of the people on Cc and many others >>> to remove the big kernel lock from Linux and I now have bkl-removal branch >>> in my git tree at git://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git >>> that lets me run a kernel on my quad-core machine with the only users of the BKL >>> being mostly obscure device driver modules. >> >> config USB >> tristate "Support for Host-side USB" >> depends on USB_ARCH_HAS_HCD && BKL >> >> Well, that's very interesting definition of "obscure" :) > > From a quick grep at least EHCI doesn't seem to need it? As a followup: I killed some time by going through the various BKL uses in USB and came up with this git tree to address them . Feel free to integrate into your tree. With this only some obscure USB low level drivers still need to depend on BKL, the majority is clean. Gadgetfs also still needs it for now. I ended up also fixing some minor races in usb serial registration/ unregistration. Opens: - The usb serial ioctl entry needs to become a unlocked_ioctl, but I think that needs your tree first. The code below it doesn't need it anymore. - The seek function in uhci-debug.c probably is still racy. Only lightly tested. Some more reviewing would be appreciated -Andi The following changes since commit b72c40949b0f04728f2993a1434598d3bad094ea: Linus Torvalds (1): Merge branch 'for-linus' of git://git.kernel.org/.../jbarnes/pci-2.6 are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-misc-2.6.git usb-bkl Andi Kleen (12): USB-BKL: Remove lock_kernel in usbfs update_sb() USB-BKL: Remove BKL from usb serial drivers ioctl handlers USB-BKL: Convert usb_driver ioctl to unlocked_ioctl USB-BKL: Remove BKL use for usb serial driver probing USB-BKL: Make usb monitor depend on BKL USB-BKL: Make usb lcd driver depend on BKL USB-BKL: Make usb sisvga driver depend on BKL USB-BKL: Make usb rio500 driver depend on BKL USB-BKL: Make usb iowarrior driver depend on BKL USB-BKL: Remove BKL use in uhci-debug USB-BKL: Make usb gadget fs depend on BKL USB-BKL: Make usb gadget printer depend on BKL drivers/usb/core/devio.c | 7 +---- drivers/usb/core/hub.c | 3 +- drivers/usb/core/inode.c | 4 --- drivers/usb/gadget/Kconfig | 3 +- drivers/usb/host/uhci-debug.c | 17 +++++---------- drivers/usb/misc/Kconfig | 6 ++-- drivers/usb/misc/sisusbvga/Kconfig | 2 +- drivers/usb/misc/usbtest.c | 3 +- drivers/usb/mon/Kconfig | 2 +- drivers/usb/serial/ark3116.c | 3 +- drivers/usb/serial/ch341.c | 3 +- drivers/usb/serial/cypress_m8.c | 8 +++--- drivers/usb/serial/ftdi_sio.c | 4 +- drivers/usb/serial/io_tables.h | 8 +++--- drivers/usb/serial/io_ti.c | 5 ++- drivers/usb/serial/kobil_sct.c | 3 +- drivers/usb/serial/mos7720.c | 3 +- drivers/usb/serial/mos7840.c | 3 +- drivers/usb/serial/opticon.c | 3 +- drivers/usb/serial/oti6858.c | 3 +- drivers/usb/serial/pl2303.c | 3 +- drivers/usb/serial/spcp8x5.c | 2 +- drivers/usb/serial/ti_usb_3410_5052.c | 6 ++-- drivers/usb/serial/usb-serial.c | 36 ++++++++++++++++---------------- drivers/usb/serial/whiteheat.c | 4 +- include/linux/usb.h | 2 +- include/linux/usb/serial.h | 2 +- 27 files changed, 74 insertions(+), 74 deletions(-) -- ak(a)linux.intel.com -- Speaking for myself only. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 29 Mar 2010 07:50 On Monday 29 March 2010, Andi Kleen wrote: > Andi Kleen <andi(a)firstfloor.org> writes: > > > > From a quick grep at least EHCI doesn't seem to need it? Right. > As a followup: > > I killed some time by going through the various BKL uses in USB and > came up with this git tree to address them . Feel free to integrate > into your tree. Great, I was hoping someone picks up the missing pieces. Maybe someone else can do the same for the sound drivers ;-). > With this only some obscure USB low level drivers still need to > depend on BKL, the majority is clean. Gadgetfs also still needs > it for now. Sure, that absolutely makes sense. > I ended up also fixing some minor races in usb serial registration/ > unregistration. > > Opens: > - The usb serial ioctl entry needs to become a unlocked_ioctl, > but I think that needs your tree first. The code below it doesn't > need it anymore. The usb-serial driver has a few instances where it takes the BKL in the mainline code, but this gets converted to the Big TTY Mutex in my series. The ioctl method in there is fine as far as I can tell. > - The seek function in uhci-debug.c probably is still racy. That function could be removed in favor of using generic_file_ioctl and setting i_size to up->size. Also, the race is only between concurrent calls of llseek on the same file descriptor, which is undefined anyway. The current code also doesn't protect you against partial updates of f_pos during ->read() on 32 bit systems (nothing ever does), and it even fails to protect against the concurrent llseek race because the assignment is done outside of the f_pos update. >commit 7160dc70d8a3e5a45c11eadfba05b9996966c6b4 >Author: Andi Kleen <ak(a)linux.intel.com> >Date: Mon Mar 29 01:26:35 2010 +0200 > > USB-BKL: Convert usb_driver ioctl to unlocked_ioctl > > And audit all the users. None needed the BKL. That was easy > because there was only very few around. > > Tested with allmodconfig build on x86-64 > > Signed-off-by: Andi Kleen <ak(a)linux.intel.com> >diff --git a/include/linux/usb.h b/include/linux/usb.h >index ce1323c..2803ca4 100644 >--- a/include/linux/usb.h >+++ b/include/linux/usb.h >@@ -846,7 +846,7 @@ struct usb_driver { > > void (*disconnect) (struct usb_interface *intf); > >- int (*ioctl) (struct usb_interface *intf, unsigned int code, >+ int (*unlocked_ioctl) (struct usb_interface *intf, unsigned int code, > void *buf); > > int (*suspend) (struct usb_interface *intf, pm_message_t message); The patch looks correct, but I probably wouldn't bother with the rename, and simply drop the BKL in the caller. >commit 89361b2e8d3dc3de83e105d60e49d5cdd9a68973 >Author: Andi Kleen <ak(a)linux.intel.com> >Date: Mon Mar 29 01:15:32 2010 +0200 > > USB-BKL: Remove BKL from usb serial drivers ioctl handlers > > I audited all the low level serial ioctl handlers and none of them > actually need the BKL. > > To make sure all code is checked change the usb_serial_driver ->ioctl > field to ->unlocked_ioctl > > Note this is still called for now with BKL held because tty drivers > don't have a ->unlocked_ioctl from the tty layer in mainline. > This could be trivially changed now though. > > Signed-off-by: Andi Kleen <ak(a)linux.intel.com> The serial_ioctl function is already called without the BKL, depite the name. tty_operations->ioctl was converted a long time ago, so I guess this patch can be dropped from your series. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andi Kleen on 29 Mar 2010 08:40 Arnd Bergmann <arnd(a)arndb.de> writes: >> - The seek function in uhci-debug.c probably is still racy. > > That function could be removed in favor of using generic_file_ioctl > and setting i_size to up->size. Does that lock against read in libfs? > Also, the race is only between concurrent calls of llseek on > the same file descriptor, which is undefined anyway. > The current code also doesn't protect you against partial updates > of f_pos during ->read() on 32 bit systems (nothing ever does), That is not what I meant. > and it even fails to protect against the concurrent llseek race > because the assignment is done outside of the f_pos update. I wasn't sure it would protect against parallel reads. Does it? > The patch looks correct, but I probably wouldn't bother with the rename, > and simply drop the BKL in the caller. I think a rename is better, I take compile errors over subtle breakage any day. >>Author: Andi Kleen <ak(a)linux.intel.com> >>Date: Mon Mar 29 01:15:32 2010 +0200 >> >> USB-BKL: Remove BKL from usb serial drivers ioctl handlers >> >> I audited all the low level serial ioctl handlers and none of them >> actually need the BKL. >> >> To make sure all code is checked change the usb_serial_driver ->ioctl >> field to ->unlocked_ioctl >> >> Note this is still called for now with BKL held because tty drivers >> don't have a ->unlocked_ioctl from the tty layer in mainline. >> This could be trivially changed now though. >> >> Signed-off-by: Andi Kleen <ak(a)linux.intel.com> > > The serial_ioctl function is already called without the BKL, depite the > name. tty_operations->ioctl was converted a long time ago, so I guess this > patch can be dropped from your series. Ok. -Andi -- ak(a)linux.intel.com -- Speaking for myself only. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 29 Mar 2010 10:50 On Monday 29 March 2010, Andi Kleen wrote: > Arnd Bergmann <arnd(a)arndb.de> writes: > > >> - The seek function in uhci-debug.c probably is still racy. > > > > That function could be removed in favor of using generic_file_ioctl > > and setting i_size to up->size. > > Does that lock against read in libfs? No. > > Also, the race is only between concurrent calls of llseek on > > the same file descriptor, which is undefined anyway. > > The current code also doesn't protect you against partial updates > > of f_pos during ->read() on 32 bit systems (nothing ever does), > > That is not what I meant. > > > and it even fails to protect against the concurrent llseek race > > because the assignment is done outside of the f_pos update. > > I wasn't sure it would protect against parallel reads. > > Does it? There is no way for any driver or file system right now to protect against that, nor has there been for a long time[1]. The sys_read and sys_write functions use file_pos_write() to update the file->f_pos without taking any lock, and they pass a local variable into the *ppos argument of the ->read/->write file operations, which means that the file operation itself cannot add locking to the update either. We never do in-place updates of file->f_pos, but on architectures where a 64 bit load can see incorrect data from a 64 bit store, any concurrent read/write/llseek combinations may cause problems, except for two concurrent lseek. Also, llseek is usually serialized with readdir/getdents for file systems. > > The patch looks correct, but I probably wouldn't bother with the rename, > > and simply drop the BKL in the caller. > > I think a rename is better, I take compile errors over subtle > breakage any day. ok, fine with me. Arnd [1] http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commitdiff;h=55f09ec0087c160533eab791607d92c9ce6222ae was merged in linux-2.6.8, which opened this race. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andi Kleen on 29 Mar 2010 16:20
> > > The patch looks correct, but I probably wouldn't bother with the rename, > > > and simply drop the BKL in the caller. > > > > I think a rename is better, I take compile errors over subtle > > breakage any day. > > ok, fine with me. I updated the git tree removing the unneeded serial change. Feel free to pull. git://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-misc-2.6.git usb-bkl -Andi -- ak(a)linux.intel.com -- Speaking for myself only. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |