Preparation for BKL'ed ioctl removal
in [Kernel]
|
Prev: mac8390: fix pr_info() calls, was Re: another cleanup patch gone wrong
Next: [PATCH] blkio: Initialize blkg->stats_lock for the root cfqg too
From: Ingo Molnar on 26 Apr 2010 03:30 * Frederic Weisbecker <fweisbec(a)gmail.com> wrote: > > And don't try to conflate the issue of ioctl and BKL. There are still > > code-paths that do lock_kernel() without the ioctl's, so the whole ioctl > > renaming has _zero_ to do with CONFIG_BKL. > > It's true, but once it gets pushed down/dropped from every core parts (which > is what we are working on currently in parallel), lock_kernel() and > .bkl_ioctl is only going to be used by unmaintained drivers. This is the > time where having a CONFIG_BKL is going to make sense. And it won't be a > question of saving some bytes but improve efficiency of schedule() for those > who don't need such old or unmaintained drivers. The scheduler will be helped most by getting rid of the BKL altogether. We are in reaching distance of that now ... CONFIG_BKL would really just elongate the migration period, unnecessarily so. > May be we should only start to focus on this new config once this state is > reached. Once that state is achived we can just get rid of the BKL and mass-push per-driver mutexes into those remaining drivers - in a possibly scripted way. Something like: foo-driver.c DEFINE_MUTEX(foo_mutex); foo_ioctl() { mutex_lock(&foo_mutex); ... mutex_unlock(&foo_mutex); } foo_open() { mutex_lock(&foo_mutex); ... mutex_unlock(&foo_mutex); } This could be done all automated for a hundred old drivers if need to be. There would be no bkl_ioctl's left. That, even if it looks somewhat coarse is still better than having _yet another_ 'temporary transition'. The Big Kernel Lock was supposed to be transitionary to begin with. It's been 10+ years and counting :-) Ingo -- 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 26 Apr 2010 04:40 On Sunday 25 April 2010 19:49:51 Linus Torvalds wrote: > In the long run (this is a year from now, when we rename "unlocked_ioctl" > back to just "ioctl"), the vfs_ioctl code will just do > > struct file_operations *fops = filp->f_op; > > if (!fops) > return -ENOTTY; > > if (fops->ioctl) { > int error = fops->ioctl(...) > if (error == -ENOIOCTLCMD) > error = -EINVAL; > return error; > } > #ifdef CONFIG_BKL > if (fops->bkl_ioctl) { > int error; > lock_kernel(); > error = fops->bkl_ioctl(...) > unlock_kernel(); > return error; > } > #endif > return -ENOTTY; We could also stop playing games with with this and just kill the locked variant of ioctl right away. No rename to bkl_ioctl, no helper functions. It's served it's purpose and we now have the list of 157 files that still use fops->ioctl, so if we just push the BKL into those files and make them use unlocked_ioctl, we will be able to remove ->ioctl for good. We can do the rename of ->unlocked_ioctl to ->ioctl right after that if you like, or in a year from now, I don't care. 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: Arnd Bergmann on 26 Apr 2010 07:40 On Monday 26 April 2010, Ingo Molnar wrote: > This could be done all automated for a hundred old drivers if need to be. > There would be no bkl_ioctl's left. I don't think it can be fully automated. For the majority of the modules, your approach would work fine, but there are still the well-known pitfalls in corner cases: - recursive uses in functions outside of ioctl (possibly none left after the TTY layer is done, but who knows) - lock-order problems with other mutexes (see DRM) - code that depends on autorelease to allow one ioctl while another is sleeping. (a small number of drivers) Semi-automated should be fine though. These rules are relatively easy to check, so we can mass-convert all the trivial cases. Examples for nontrivial modules are mostly file systems, see ncpfs, afs, hpfs, ... > That, even if it looks somewhat coarse is still better than having _yet > another_ 'temporary transition'. The Big Kernel Lock was supposed to be > transitionary to begin with. It's been 10+ years and counting :-) I think the immediate goal should be to get the BKL out of everthing that's either used by real people or that's reasonably easy to do. We have patches for almost all of these now [1], and I've been running a kernel with CONFIG_BKL=n for a few weeks now. As we progress through the remaining modules, an increasing number of systems can run without this. I see the next steps as: 1. make it possible to build a kernel without BKL, by removing the BKL from all core components for good, and making all users depend on CONFIG_BKL. Make it default y and put it under CONFIG_DEBUG. 2. Remove the BKL from all modules that are either easy to convert to a private mutex or that are relevant to real users (e.g. definitely DRM, but maybe not hpfs). 3. Change CONFIG_BKL to "default n, depends on DEPRECATED" 4. Remove the remaining modules that nobody knows how to fix or cares about. Possibly the number of modules here is close to zero. 5. Remove the implementation of the BKL, since no users are left. Getting stage 1 done for 2.6.36 or even 2.6.35 should be possible but still needs a lot of work. I think we should focus on that for now and then see how much is left to do for the other stages. This is still a temporary transition, but since we can't do all at once, I don't see better way. Arnd [1] http://kernelnewbies.org/BigKernelLock -- 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 26 Apr 2010 15:20 On Monday 26 April 2010 20:08:39 Linus Torvalds wrote: > I wouldn't mind that. But we've not found people to do it so far, so... I'll have another go at this tonight, see how far I get doing it the simplest way for each file. > NOTE! This has gone through a "allmodconfig" (with staging drivers), but > only on x86-64. There are quite probably missing architecture conversions > and/or drivers. But this should be the bulk of them. Yes, looks good. I've compared it to a previous series of mine that changed all the files in a different way. A number of those changes have found their way into your tree by now, and there were only two left that you didn't catch (see below), and one false positive where I patched a struct v4l2_file_operations. > It doesn't look that bad. Just about 100 files affected. The v4l2_file_operations is actually another can of worms, I count 77 more files with a locked ioctl function in there. I'll put that on the list of things to do before the BKL is gone. Arnd --- Subject: Missing bits from ->ioctl to ->bkl_ioctl conversion Feel free to fold this into your patch. Signed-off-by: Arnd Bergmann <arnd(a)arndb.de> diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt index 3de2f32..4890683 100644 --- a/Documentation/filesystems/vfs.txt +++ b/Documentation/filesystems/vfs.txt @@ -722,7 +722,7 @@ struct file_operations { ssize_t (*aio_write) (struct kiocb *, const struct iovec *, unsigned long, loff_t); int (*readdir) (struct file *, void *, filldir_t); unsigned int (*poll) (struct file *, struct poll_table_struct *); - int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long); + int (*bkl_ioctl) (struct inode *, struct file *, unsigned int, unsigned long); long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long); long (*compat_ioctl) (struct file *, unsigned int, unsigned long); int (*mmap) (struct file *, struct vm_area_struct *); diff --git a/sound/oss/au1550_ac97.c b/sound/oss/au1550_ac97.c index c1070e3..7295781 100644 --- a/sound/oss/au1550_ac97.c +++ b/sound/oss/au1550_ac97.c @@ -835,11 +835,11 @@ au1550_ioctl_mixdev(struct inode *inode, struct file *file, } static /*const */ struct file_operations au1550_mixer_fops = { - owner:THIS_MODULE, - llseek:au1550_llseek, - ioctl:au1550_ioctl_mixdev, - open:au1550_open_mixdev, - release:au1550_release_mixdev, + .owner = THIS_MODULE, + .llseek = au1550_llseek, + .bkl_ioctl = au1550_ioctl_mixdev, + .open = au1550_open_mixdev, + .release = au1550_release_mixdev, }; static int -- 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: David Miller on 26 Apr 2010 16:50
From: Linus Torvalds <torvalds(a)linux-foundation.org> Date: Mon, 26 Apr 2010 11:08:39 -0700 (PDT) > NOTE! This has gone through a "allmodconfig" (with staging drivers), but > only on x86-64. There are quite probably missing architecture conversions > and/or drivers. But this should be the bulk of them. "allmodconfig" on sparc64 passes with this patch too, just FYI... -- 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/ |