Killing the Big Kernel Lock
in [Kernel]
|
Prev: Darlehen bieten bei 2% Zins (Union beschränkt Finanzierung)
Next: [PATCH] rt2500usb: improve powersaving reliability
From: Arnd Bergmann on 28 Mar 2010 17:40 On Sunday 28 March 2010, Frederic Weisbecker wrote: > On Sun, Mar 28, 2010 at 09:05:50PM +0100, Arnd Bergmann wrote: > > > General thoughts: > > > > > > ".llseek = NULL," so far meant "do the Right Thing on lseek() and > > > friends, as far as the fs core can tell". Shouldn't we keep it that > > > way? It's as close to other ".method = NULL," as it can get, which > > > either mean "silently skip this method if it doesn't matter" (e.g. > > > .flush) or "fail attempts to use this method with a fitting errno" (e.g. > > > .write). > > > > My series changes the default from 'default_llseek' to 'generic_file_llseek', > > which is almost identical, except for taking the inode mutex instead of the > > BKL. > > > What if another file operation changes the file pointer while holding the bkl? > You're not protected anymore in this case. > Exactly, that's why I changed all the drivers to set default_llseek explicitly. Even this is very likely not needed in more than a handful of drivers (if any), for a number of reasons: - sys_read/sys_write *never* hold any locks while calling file_pos_write(), which is the only place they get updated for regular files. - concurrent llseek plus other file operations on the same file descriptor usually already have an undefined outcome. - when I started inspecting drivers that look at file->f_pos themselves (not the read/write operation arguments), I found that practically all of them are doing this in a totally broken way! - The only think we'd probably ever want to lock against in llseek is readdir, which is not used in any drivers, but only in file systems. 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 28 Mar 2010 18:00 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? Except for those two guys in core/*.c: /* keep API that guarantees BKL */ lock_kernel(); retval = driver->ioctl(intf, ctl->ioctl_code, buf); unlock_kernel(); if (retval == -ENOIOCTLCMD) retval = -ENOTTY; I guess that could be just moved into the low level modules with unlocked_ioctl And one use in the usbfs which seems quite bogus and can be probably removed. -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: Frederic Weisbecker on 28 Mar 2010 19:20 On Wed, Mar 24, 2010 at 10:40:54PM +0100, Arnd Bergmann wrote: > Arnd Bergmann (44): > [...] > procfs: kill BKL in llseek About this one, there is a "sensible" part: @@ -1943,7 +1949,7 @@ static ssize_t proc_fdinfo_read(struct file *file, char __user *buf, } static const struct file_operations proc_fdinfo_file_operations = { - .open = nonseekable_open, + .llseek = generic_file_llseek, .read = proc_fdinfo_read, }; Replacing default_llseek() by generic_file_llseek() as you did for most of the other parts is fine. But the above changes the semantics as it makes it seekable. Why not just keeping it as is? It just ends up in no_llseek(). -- 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: Frederic Weisbecker on 28 Mar 2010 19:30 On Sun, Mar 28, 2010 at 10:34:54PM +0100, Arnd Bergmann wrote: > On Sunday 28 March 2010, Frederic Weisbecker wrote: > > On Sun, Mar 28, 2010 at 09:05:50PM +0100, Arnd Bergmann wrote: > > > > General thoughts: > > > > > > > > ".llseek = NULL," so far meant "do the Right Thing on lseek() and > > > > friends, as far as the fs core can tell". Shouldn't we keep it that > > > > way? It's as close to other ".method = NULL," as it can get, which > > > > either mean "silently skip this method if it doesn't matter" (e.g. > > > > .flush) or "fail attempts to use this method with a fitting errno" (e.g. > > > > .write). > > > > > > My series changes the default from 'default_llseek' to 'generic_file_llseek', > > > which is almost identical, except for taking the inode mutex instead of the > > > BKL. > > > > > > What if another file operation changes the file pointer while holding the bkl? > > You're not protected anymore in this case. > > > > Exactly, that's why I changed all the drivers to set default_llseek explicitly. Ah ok. > Even this is very likely not needed in more than a handful of drivers (if any), > for a number of reasons: > > - sys_read/sys_write *never* hold any locks while calling file_pos_write(), > which is the only place they get updated for regular files. Yeah sure. But the pushdown (or step by step replacement with generic_file_llseek) is still necessary to ensure every places are fine. > - concurrent llseek plus other file operations on the same file descriptor > usually already have an undefined outcome. Yeah. > - when I started inspecting drivers that look at file->f_pos themselves (not > the read/write operation arguments), I found that practically all of them > are doing this in a totally broken way! Hehe :) > - The only think we'd probably ever want to lock against in llseek > is readdir, which is not used in any drivers, but only in file systems. Right. -- 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: Frederic Weisbecker on 28 Mar 2010 19:40
On Mon, Mar 29, 2010 at 01:18:48AM +0200, Frederic Weisbecker wrote: > On Wed, Mar 24, 2010 at 10:40:54PM +0100, Arnd Bergmann wrote: > > Arnd Bergmann (44): > > [...] > > procfs: kill BKL in llseek > > > About this one, there is a "sensible" part: > > > @@ -1943,7 +1949,7 @@ static ssize_t proc_fdinfo_read(struct file *file, char __user *buf, > } > > static const struct file_operations proc_fdinfo_file_operations = { > - .open = nonseekable_open, > + .llseek = generic_file_llseek, > .read = proc_fdinfo_read, > }; > > > Replacing default_llseek() by generic_file_llseek() as you > did for most of the other parts is fine. > > But the above changes the semantics as it makes it seekable. > Why not just keeping it as is? It just ends up in no_llseek(). > There is also the ioctl part that takes the bkl in procfs. I'll just check nothing weird happens there wrt file pos. We probably first need to pushdown the bkl in the procfs ioctl handlers. -- 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/ |