Char dev BKL pushdown v2

OK, since the previous announcement, I've revisited all of the open() functions which didn't get lock_kernel() calls the first time around. Alan pointed out that even a completely empty open() might, in fact, need to acquire the BKL, so now they all do. Hopefully, this completes this work (at this level - there's plenty of down-pushing to do within subsystems).

There's a new tree with this stuff:

	git://git.lwn.net/linux-2.6.git bkl-removal

Stephen, might it be about time to pull this into linux-next and see what explodes?

If others have BKL-removal patches which look like 2.6.27 material, I'll happily collect them in this tree.

Thanks,

jon

P.S. Here's what's in it now:

 arch/cris/arch-v10/drivers/eeprom.c          |    4 +-
 arch/cris/arch-v10/drivers/gpio.c            |    3 ++
 arch/cris/arch-v10/drivers/i2c.c             |    2 +
 arch/cris/arch-v10/drivers/sync_serial.c     |   34 ++++++++++++++----------
 arch/cris/arch-v32/drivers/cryptocop.c       |    3 +-
 arch/cris/arch-v32/drivers/i2c.c             |    2 +
 arch/cris/arch-v32/drivers/mach-a3/gpio.c    |    4 ++
 arch/cris/arch-v32/drivers/mach-fs/gpio.c    |    5 ++-
 arch/cris/arch-v32/drivers/sync_serial.c     |   33 ++++++++++++++---------
 arch/mips/kernel/rtlx.c                      |    7 ++++
 arch/mips/kernel/vpe.c                       |   12 ++++++--
 arch/mips/sibyte/common/sb_tbprof.c          |   25 ++++++++++++-----
 arch/sh/boards/landisk/gio.c                 |   10 ++++---
 arch/x86/kernel/cpuid.c                      |   25 ++++++++++++-----
 arch/x86/kernel/msr.c                        |   16 ++++++++---
 block/bsg.c                                  |    7 ++++
 drivers/block/aoe/aoechr.c                   |    7 ++++
 drivers/block/paride/pg.c                    |   22 +++++++++++----
 drivers/block/paride/pt.c                    |    8 ++++-
 drivers/char/cs5535_gpio.c                   |    2 +
 drivers/char/drm/drm_fops.c                  |    9 ++++--
 drivers/char/dsp56k.c                        |   14 +++++++--
 drivers/char/dtlk.c                          |    3 ++
 drivers/char/ip2/ip2main.c                   |   34 +-----------------------
 drivers/char/ipmi/ipmi_devintf.c             |    8 ++++-
 drivers/char/lp.c                            |   38 ++++++++++++++++++---------
 drivers/char/mbcs.c                          |    5 +++
 drivers/char/mem.c                           |   10 +++++--
 drivers/char/misc.c                          |    3 ++
 drivers/char/pc8736x_gpio.c                  |    2 +
 drivers/char/pcmcia/cm4000_cs.c              |   26 +++++++++++++-----
 drivers/char/pcmcia/cm4040_cs.c              |   23 ++++++++++++----
 drivers/char/ppdev.c                         |    2 +
 drivers/char/raw.c                           |    3 ++
 drivers/char/scx200_gpio.c                   |    2 +
 drivers/char/snsc.c                          |    5 ++-
 drivers/char/tb0219.c                        |    2 +
 drivers/char/tlclk.c                         |   19 ++++++++-----
 drivers/char/tty_io.c                        |   27 +++++++++++++++++--
 drivers/char/vc_screen.c                     |    9 ++++--
 drivers/char/viotape.c                       |    3 ++
 drivers/char/vr41xx_giu.c                    |    2 +
 drivers/char/xilinx_hwicap/xilinx_hwicap.c   |    6 +++-
 drivers/firewire/fw-cdev.c                   |   16 ++++++++---
 drivers/hid/hidraw.c                         |    3 ++
 drivers/i2c/i2c-dev.c                        |   22 +++++++++++----
 drivers/ide/ide-tape.c                       |    7 ++++
 drivers/ieee1394/dv1394.c                    |    6 +++-
 drivers/ieee1394/raw1394.c                   |    3 ++
 drivers/ieee1394/video1394.c                 |   18 +++++++++---
 drivers/infiniband/core/ucm.c                |    2 +
 drivers/infiniband/core/user_mad.c           |    7 ++++
 drivers/infiniband/core/uverbs_main.c        |    9 ++++--
 drivers/infiniband/hw/ipath/ipath_file_ops.c |    2 +
 drivers/input/input.c                        |   16 ++++++++---
 drivers/isdn/capi/capi.c                     |   17 +++++++-----
 drivers/isdn/hardware/eicon/divamnt.c        |   16 +++++++----
 drivers/isdn/hardware/eicon/divasi.c         |    2 +
 drivers/isdn/hardware/eicon/divasmain.c      |    2 +
 drivers/isdn/i4l/isdn_common.c               |    3 +-
 drivers/macintosh/adb.c                      |   18 +++++++++---
 drivers/media/dvb/dvb-core/dvbdev.c          |    4 ++
 drivers/media/video/videodev.c               |    4 ++
 drivers/misc/phantom.c                       |    9 ++++--
 drivers/mtd/mtdchar.c                        |   22 +++++++++++----
 drivers/mtd/ubi/cdev.c                       |    7 ++++
 drivers/net/ppp_generic.c                    |    2 +
 drivers/net/wan/cosa.c                       |   22 +++++++++++----
 drivers/pcmcia/pcmcia_ioctl.c                |   25 ++++++++++++-----
 drivers/rtc/rtc-dev.c                        |   12 ++++++--
 drivers/s390/char/fs3270.c                   |   23 +++++++++++-----
 drivers/s390/char/tape_char.c                |   12 ++++++--
 drivers/s390/char/vmlogrdr.c                 |    8 ++++-
 drivers/s390/char/vmur.c                     |   12 ++++++--
 drivers/sbus/char/bpp.c                      |    3 ++
 drivers/sbus/char/vfc_dev.c                  |    5 +++
 drivers/scsi/3w-9xxx.c                       |    3 ++
 drivers/scsi/3w-xxxx.c                       |    3 ++
 drivers/scsi/aacraid/linit.c                 |    3 ++
 drivers/scsi/ch.c                            |    4 ++
 drivers/scsi/dpt_i2o.c                       |    5 +++
 drivers/scsi/gdth.c                          |    3 ++
 drivers/scsi/megaraid.c                      |    5 ++-
 drivers/scsi/megaraid/megaraid_sas.c         |    2 +
 drivers/scsi/osst.c                          |   15 +++++++++-
 drivers/scsi/sg.c                            |   16 +++++++++--
 drivers/scsi/st.c                            |   11 ++++++-
 drivers/spi/spidev.c                         |    3 ++
 drivers/telephony/phonedev.c                 |    3 ++
 drivers/uio/uio.c                            |   17 ++++++++----
 drivers/usb/core/devio.c                     |    2 +
 drivers/usb/core/file.c                      |    3 ++
 drivers/usb/gadget/printer.c                 |    3 +-
 drivers/usb/mon/mon_bin.c                    |    6 ++++
 drivers/video/fbmem.c                        |   15 +++++++---
 fs/char_dev.c                                |    7 ++--
 include/linux/smp_lock.h                     |   13 +++++++++
 sound/core/sound.c                           |   15 +++++++++-
 sound/sound_core.c                           |    5 +++
 99 files changed, 739 insertions(+), 248 deletions(-)

Jonathan Corbet (65):
bsg: cdev lock_kernel() pushdown
cris: cdev lock_kernel() pushdown
mips: cdev lock_kernel() pushdown
sh: cdev lock_kernel() pushdown
x86: cdev lock_kernel() pushdown
i2c: cdev lock_kernel() pushdown
cosa: cdev lock_kernel() pushdown
pcmcia: cdev lock_kernel() pushdown
ieee1394: cdev lock_kernel() pushdown
rtc: cdev lock_kernel() pushdown drivers/s390: cdev lock_kernel() pushdown
AoE: cdev lock_kernel() pushdown
paride: cdev lock_kernel() pushdown
mtdchar: cdev lock_kernel() pushdown
UBI: cdev lock_kernel() pushdown
firewire: cdev lock_kernel() pushdown
HID: cdev lock_kernel() pushdown
Input: cdev lock_kernel() pushdown
UIO: cdev lock_kernel() pushdown
cm40x0: cdev lock_kernel() pushdown
ipmi: cdev lock_kernel() pushdown
mem: cdev lock_kernel() pushdown
misc: cdev lock_kernel() pushdown
viotape: cdev lock_kernel pushdown ()
mbcs: cdev lock_kernel() pushdown
lp: cdev lock_kernel() pushdown
drm: cdev lock_kernel() pushdown
phonedev: cdev lock_kernel() pushdown
ide-tape: cdev lock_kernel() pushdown
sg: cdev lock_kernel() pushdown
osst: cdev lock_kernel() pushdown.
aacraid: cdev lock_kernel() pushdown
st: cdev lock_kernel() pushdown
gdth: cdev lock_kernel() pushdown
isdn: cdev lock_kernel() pushdown
usbcore: cdev lock_kernel() pushdown
dvb: cdev lock_kernel() pushdown
fbmem: cdev lock_kernel() pushdown
sound: cdev lock_kernel() pushdown
snsc: cdev lock_kernel() pushdown
tty: cdev lock_kernel() pushdown
Add "no BKL needed" comments to several drivers
spidev: BKL pushdown
vcs: BKL pushdown
xilinx icap: BKL pushdown
tlckl: BKL pushdown
raw: BKL pushdown
dsp56k: BKL pushdown
infiniband: more BKL pushdown
phantom: BKL pushdown
bpp: bkl pushdown
videopix: BKL pushdown
dpt_i20: BKL pushdown
changer: BKL pushdown
CAPI: BKL pushdown
divamnt: BKL pushdown
adb: BKL pushdown
printer gadget: BKL pushdown
USB Monitor: BKL pushdown
usbdev: BKL pushdown
videodev: BKL pushdown
Add cycle_kernel_lock()
Add a bunch of cycle_kernel_lock() calls Add a comment in chrdev_open()
Remove the lock_kernel() call from chrdev_open()

Great work, Jon! It's really cool to see some real momentum towards getting rid of the BKL at last.

All of these changes look fine from a pure "push the BKL down" point of view. However I am 99% sure no BKL use is required in any of these (and I will think deeper to get another .9% surer tomorrow).

Is the plan that we have a pure "push the BKL down" changeset merged, and then I can merge BKL removal patches for these places that never needed the BKL? (I guess I can send you such a patch to base on top of your tree for when Linus pulls it? Is 2.6.27 the plan?) The alternative is to never add the BKL to these places as part of this patch -- which seems to be a bad, risky plan, since if any mistakes are made, then bisection just lands on some giant patch.

Thanks,
Roland

If you're sure that this code doesn't need the BKL (and it kind of looked that way to me), the preferred approach seems to be to put in a comment to that effect so that it's clear that the code has been looked at. So sending me a patch which does this would be great. Otherwise, if you're willing to swear on top of a stack of Knuth output that the BKL is not needed for specific open functions, I can revert my patch back out and put in the comment - whichever you prefer.

jon

OK, will send such a patch after auditing more carefully. Just to be very clear, the issue is to make sure the locking is sufficient to protect against multiple racing open calls to the same character device?

- R.

That's part of it, but, as Alan pointed out, there's more. The BKL currently protects open() calls against concurrency with other opens, with ioctl(), and with driver initialization as well. So it's a matter of having one's locking and ordering act together in general.

jon

Thanks. Just to be super-explicit, ioctl() cannot be called on a given file until the open() for that particular file has returned, right?

And the point about driver initialization is that open() can be called as soon as the file operations are registered, even if the module_init function has not returned?

Thanks,
Roland

ioctl() will not be called on a given file descriptor before open() is done, no. If there are other file descriptors open, though, somebody can be calling ioctl() on them while the open() for the new one is executing.

That is the point, yes. The key there is to avoid registering the device before everything has been set up to actually manage the device.

jon

There's the case where one thread is calling ioctl on the new descriptor before open (in other thread) has returned (it's malicious and trying to oops you, it's accidentally trying to operate on a closed descriptor, etc). It might hit the window between the descripor being installed and open returning.

Jeff

Work email - jdike at linux dot intel dot com

Right. Or two ioctls against each other unless one sleepers, or against random other parts of the kernel which still use the BKL - eg fasync, bits of procfs ...

Exactly.

Alan

Hi Jon,

I have added that to today's build right at the end so that if it explodes too badly, it will be removed again.

I haven't been following this too closely, so how much of this stuff has been posted/review/tested?

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

This is the second posting (some of the changesets are new this time around). It all builds and runs for me, but a lot of this work affects some of the oldest, most cobweb-filled code around, and I sure don't have that hardware. Some others have looked at, but I'm not sure how deeply.

The basic concept of this work is straightforward, so I have good reason to hope I've not done anything really stupid.

jon

cool stuff! :-)

i'm wondering how this should best interact with the kill-the-BKL bits that attack it all from the infrastructure side. Your bits are the safer ones, so i guess i'll try to track your changes from my branch to increase testing of it all?

Ingo

...

...

I have yet to look at these drivers in detail whether they need BKL or not. They likely don't.

drivers/firewire/fw-cdev.c::fw_device_op_open() does not need the BKL.

Stefan Richter
-=====-==--- -=-= =--==
http://arcgraph.de/sr/

video1394 needs it to serialize module init vs. open. The race condition there can be prevented by splitting hpsb_register_highlevel() into an _init and a _register function. I will follow up with a patch.

dv1394 and raw1394 do not need the BKL in their open() methods AFAICS.

Stefan Richter
-=====-==--- -=-= =--==
http://arcgraph.de/sr/

On 19 May, Stefan Richter wrote at LKML:

From: Stefan Richter <[email protected]>
Subject: ieee1394: video1394: reorder module init, prepare BKL removal

This prepares video1394 for removal of the BKL (big kernel lock):
It allows video1394_open() to be called while video1394_init_module() is still in progress.

---
 drivers/ieee1394/highlevel.c |    4 +---
 drivers/ieee1394/highlevel.h |   13 ++++++++++++-
 drivers/ieee1394/video1394.c |    2 ++
 3 files changed, 15 insertions(+), 4 deletions(-)

Index: linux/drivers/ieee1394/highlevel.c
===================================================================
@@ -228,10 +228,8 @@ void hpsb_register_highlevel(struct hpsb
 {
 	unsigned long flags;
 
+	hpsb_init_highlevel(hl);
 	INIT_LIST_HEAD(&hl->addr_list);
-	INIT_LIST_HEAD(&hl->host_info_list);
-
-	rwlock_init(&hl->host_info_lock);
 
 	down_write(&hl_drivers_sem);
 	list_add_tail(&hl->hl_list, &hl_drivers);

Index: linux/drivers/ieee1394/highlevel.h
===================================================================
@@ -2,7 +2,7 @@
 #define IEEE1394_HIGHLEVEL_H
 
 #include <linux/list.h>
-#include <linux/spinlock_types.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 
 struct module;

@@ -103,6 +103,17 @@ int highlevel_lock64(struct hpsb_host *h
 void highlevel_fcp_request(struct hpsb_host *host, int nodeid, int direction,
 			   void *data, size_t length);
 
+/**
+ * hpsb_init_highlevel - initialize a struct hpsb_highlevel
+ *
+ * This is only necessary if hpsb_get_hostinfo_bykey can be called
+ * before hpsb_register_highlevel.
+ */
+static inline void hpsb_init_highlevel(struct hpsb_highlevel *hl)
+{
+	rwlock_init(&hl->host_info_lock);
+	INIT_LIST_HEAD(&hl->host_info_list);
+}
 void hpsb_register_highlevel(struct hpsb_highlevel *hl);
 void hpsb_unregister_highlevel(struct hpsb_highlevel *hl);
 

Index: linux/drivers/ieee1394/video1394.c
===================================================================
@@ -1503,6 +1503,8 @@ static int __init video1394_init_module 
 {
 	int ret;
 
+	hpsb_init_highlevel(&video1394_highlevel);
+
 	cdev_init(&video1394_cdev, &video1394_fops);
 	video1394_cdev.owner = THIS_MODULE;
 	ret = cdev_add(&video1394_cdev, IEEE1394_VIDEO1394_DEV, 16);

Stefan Richter
-=====-==--- -=-= =--==
http://arcgraph.de/sr/