procfs: Kill the bkl in ioctl
in [Kernel]
|
Prev: procfs: Kill BKL in llseek on proc base
Next: net/wireless/libertas: do not call wiphy_unregister() w/o wiphy_register()
From: Stefan Richter on 3 Apr 2010 14:00 Arnd Bergmann wrote: > Subject: [PATCH] introduce CONFIG_BKL and default_ioctl [...] > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -58,6 +58,28 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd, > return error; > } > > +#ifdef CONFIG_BKL > +/* > + * default_ioctl - call unlocked_ioctl with BKL held [...] > + error = filp->f_op->locked_ioctl(filp->f_path.dentry->d_inode, > + filp, cmd, arg); Typo. default_ioctl - call locked_ioctl with BKL held -- Stefan Richter -=====-==-=- -=-- ---== http://arcgraph.de/sr/ -- 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 10 Apr 2010 11:30 On Thu, Apr 01, 2010 at 02:45:32PM +0200, Arnd Bergmann wrote: > On Thursday 01 April 2010, Stefan Richter wrote: > > > > > > I wonder if we should actually just turn all these into unlocked_ioctl > > > directly. And then bring a warn on ioctl, and finally schedule the removal > > > of this callback. > > > > A side note: A considerable portion of this particular commit in Arnd's > > git actually does not deal with .ioctl->.unlocked_ioctl at all, but > > purely with .llseek. Many(?) of these changes deal with .ioctl and > > .llseek together. (Arnd also says so in the last paragraph of his > > changelog.) > > > > IOW there are less .ioctl implementations left than one could think from > > a look at the diffstat. > > Given our recent discussions on the llseek topic, it's probably better to > revert most of the changes that purely deal with llseek. My current idea > is to use an explicit default_llseek only if one of the following is given: > > - we convert ioctl to unlocked_ioctl in the same file_operations, or > - the module uses the big kernel lock explicitly elsewhere. > > Even then, there may be a number of cases where we can show it not > to be necessary, e.g. when the driver does not care about f_pos. > Concurrent llseek is racy by nature, so in most drivers, using the > BKL in llseek does not gain anything over using i_mutex. > > Arnd So you mean we should attribute explicit default_llseek to the evil places instead of explicit generic_file_llseek in the safe ones? That's not a bad idea as it would result in much less changes. The problem happens the day you switch to generic_file_llseek() as the new default llseek(), how do you prove that all remaining fops that don't implement .llseek don't use the bkl? There will be hundreds of them and saying "we've looked all of them and they don't need it" will be a scary justification. On the opposite, attributing explicit generic_file_llseek or non_seekable_open on the safe places and default_llseek on the dozens of others doubtful places is easier to get a safe conclusion. But yeah we should try, at least attributing explicit default_llseek won't harm, quite the opposite. -- 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 10 Apr 2010 12:20 On Thu, Apr 01, 2010 at 02:42:38PM +0200, Arnd Bergmann wrote: > On Wednesday 31 March 2010, Frederic Weisbecker wrote: > > On Wed, Mar 31, 2010 at 10:21:23PM +0200, Arnd Bergmann wrote: > > > > > In the meantime, we can move the declaration of the .locked_ioctl callback > > > into an #ifdef CONFIG_BKL, to make sure nobody builds a driver with an > > > ioctl function that does not get called. > > > > > > Ok, now how to get this all merged? A single monolithic patch is probably > > not appropriate. > > > > The simplest is to have a single branch with the default_ioctl implemented, > > and then attributed to drivers in a set cut by subsystems/drivers. And > > push the whole for the next -rc1. > > > > The other solution is to push default_ioctl for this release and get > > the driver changes to each concerned tree. That said, I suspect a good > > part of them are unmaintained, hence the other solution looks better > > to me. > > I don't care much about the unmaintained parts, we can always have a > tree collecting all the patches for those drivers and merge it in -rc1. > > I'd say the nicest way would be to get Linus to merge the patch > below now, so we can start queuing stuff in maintainer trees on top > of it, and check against linux-next what is still missing and push > all of them from our branch. > > Arnd > > --- > Subject: [PATCH] introduce CONFIG_BKL and default_ioctl > > This is a preparation for the removal of the big kernel lock that > introduces new interfaces for device drivers still using it. > > We can start marking those device drivers as 'depends on CONFIG_BKL' > now, and make that symbol optional later, when the point has come > at which we are able to build a kernel without the BKL. > > Similarly, device drivers that currently make use of the implicit > BKL locking around the ioctl function can now get annotated by > changing > > .ioctl = foo_ioctl, > > to > > .locked_ioctl = foo_ioctl, > .unlocked_ioctl = default_ioctl, > > As soon as no driver remains using the old ioctl callback, it can > get removed. > > Signed-off-by: Arnd Bergmann <arnd(a)arndb.de> > --- > fs/ioctl.c | 22 ++++++++++++++++++++++ > include/linux/fs.h | 3 +++ > include/linux/smp_lock.h | 4 ++++ > lib/Kconfig.debug | 10 ++++++++++ > 4 files changed, 39 insertions(+), 0 deletions(-) > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 6c75110..52c2698 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -58,6 +58,28 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd, > return error; > } > > +#ifdef CONFIG_BKL > +/* > + * default_ioctl - call unlocked_ioctl with BKL held > + * > + * Setting only the the ioctl operation but not unlocked_ioctl will become > + * invalid in the future, all drivers that are not converted to unlocked_ioctl > + * should set .unlocked_ioctl = default_ioctl now. > + */ > +long default_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > +{ Do you mind if I rename this to deprecated_ioctl()? This "default" naming suggests a fallback everyone that don't need tricky things should use. -- 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 10 Apr 2010 12:20 On Thu, Apr 01, 2010 at 02:42:38PM +0200, Arnd Bergmann wrote: > On Wednesday 31 March 2010, Frederic Weisbecker wrote: > > On Wed, Mar 31, 2010 at 10:21:23PM +0200, Arnd Bergmann wrote: > > > > > In the meantime, we can move the declaration of the .locked_ioctl callback > > > into an #ifdef CONFIG_BKL, to make sure nobody builds a driver with an > > > ioctl function that does not get called. > > > > > > Ok, now how to get this all merged? A single monolithic patch is probably > > not appropriate. > > > > The simplest is to have a single branch with the default_ioctl implemented, > > and then attributed to drivers in a set cut by subsystems/drivers. And > > push the whole for the next -rc1. > > > > The other solution is to push default_ioctl for this release and get > > the driver changes to each concerned tree. That said, I suspect a good > > part of them are unmaintained, hence the other solution looks better > > to me. > > I don't care much about the unmaintained parts, we can always have a > tree collecting all the patches for those drivers and merge it in -rc1. > > I'd say the nicest way would be to get Linus to merge the patch > below now, so we can start queuing stuff in maintainer trees on top > of it, and check against linux-next what is still missing and push > all of them from our branch. > > Arnd > > --- > Subject: [PATCH] introduce CONFIG_BKL and default_ioctl > > This is a preparation for the removal of the big kernel lock that > introduces new interfaces for device drivers still using it. > > We can start marking those device drivers as 'depends on CONFIG_BKL' > now, and make that symbol optional later, when the point has come > at which we are able to build a kernel without the BKL. > > Similarly, device drivers that currently make use of the implicit > BKL locking around the ioctl function can now get annotated by > changing > > .ioctl = foo_ioctl, > > to > > .locked_ioctl = foo_ioctl, > .unlocked_ioctl = default_ioctl, > > As soon as no driver remains using the old ioctl callback, it can > get removed. > > Signed-off-by: Arnd Bergmann <arnd(a)arndb.de> Al, we would like to make this for 2.6.34, so that we can start converting the drivers that use bkl'ed ioctl to this new scheme in the relevant maintainers trees. Can we get your ack? Thanks. -- 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 10 Apr 2010 12:30
On Thu, Apr 01, 2010 at 02:42:38PM +0200, Arnd Bergmann wrote: > On Wednesday 31 March 2010, Frederic Weisbecker wrote: > > On Wed, Mar 31, 2010 at 10:21:23PM +0200, Arnd Bergmann wrote: > > > > > In the meantime, we can move the declaration of the .locked_ioctl callback > > > into an #ifdef CONFIG_BKL, to make sure nobody builds a driver with an > > > ioctl function that does not get called. > > > > > > Ok, now how to get this all merged? A single monolithic patch is probably > > not appropriate. > > > > The simplest is to have a single branch with the default_ioctl implemented, > > and then attributed to drivers in a set cut by subsystems/drivers. And > > push the whole for the next -rc1. > > > > The other solution is to push default_ioctl for this release and get > > the driver changes to each concerned tree. That said, I suspect a good > > part of them are unmaintained, hence the other solution looks better > > to me. > > I don't care much about the unmaintained parts, we can always have a > tree collecting all the patches for those drivers and merge it in -rc1. > > I'd say the nicest way would be to get Linus to merge the patch > below now, so we can start queuing stuff in maintainer trees on top > of it, and check against linux-next what is still missing and push > all of them from our branch. > > Arnd > > --- > Subject: [PATCH] introduce CONFIG_BKL and default_ioctl > > This is a preparation for the removal of the big kernel lock that > introduces new interfaces for device drivers still using it. > > We can start marking those device drivers as 'depends on CONFIG_BKL' > now, and make that symbol optional later, when the point has come > at which we are able to build a kernel without the BKL. > > Similarly, device drivers that currently make use of the implicit > BKL locking around the ioctl function can now get annotated by > changing > > .ioctl = foo_ioctl, > > to > > .locked_ioctl = foo_ioctl, > .unlocked_ioctl = default_ioctl, > > As soon as no driver remains using the old ioctl callback, it can > get removed. > > Signed-off-by: Arnd Bergmann <arnd(a)arndb.de> Al, we would like to make this for 2.6.34, so that we can start converting the drivers that use bkl'ed ioctl to this new scheme in the relevant maintainers trees. Can we get your ack? Thanks. -- 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/ |