Re: [PATCH, RFC] Char dev BKL pushdown v2

From: Stefan Richter <stefanr@...>

To: Jonathan Corbet <corbet@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Monday, May 19, 2008 - 1:46 pm

I have yet to look at these drivers in detail whether they need BKL or 
drivers/firewire/fw-cdev.c::fw_device_op_open() does not need the BKL.

--

Stefan Richter

-=====-==--- -=-= =--==

http://arcgraph.de/sr/

--

From: Stefan Richter <stefanr@...>

To: Jonathan Corbet <corbet@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Monday, May 19, 2008 - 3:27 pm

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/

--

From: Stefan Richter <stefanr@...>

To: Jonathan Corbet <corbet@...>, <linux1394-devel@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Monday, May 19, 2008 - 4:07 pm

From: Stefan Richter <stefanr@s5r6.in-berlin.de>

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.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

---

 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

===================================================================

--- linux.orig/drivers/ieee1394/highlevel.c

+++ 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

===================================================================

--- linux.orig/drivers/ieee1394/highlevel.h

+++ 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);

...
[ message continues ]

From: Ingo Molnar <mingo@...>

To: Jonathan Corbet <corbet@...>

Cc: Linus Torvalds <torvalds@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Monday, May 19, 2008 - 9:17 am

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
--

From: Stephen Rothwell <sfr@...>

To: Jonathan Corbet <corbet@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>

Date: Monday, May 19, 2008 - 12:03 am

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?

--=20
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

To: Stephen Rothwell <sfr@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>

Date: Monday, May 19, 2008 - 9:46 am

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
--

From: Roland Dreier <rdreier@...>

To: Jonathan Corbet <corbet@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Monday, May 19, 2008 - 12:00 am

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

> 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 +

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
--

To: Roland Dreier <rdreier@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Monday, May 19, 2008 - 9:37 am

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
--

From: Roland Dreier <rdreier@...>

To: Jonathan Corbet <corbet@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Monday, May 19, 2008 - 4:38 pm

> 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.

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.
--

To: Roland Dreier <rdreier@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Monday, May 19, 2008 - 4:42 pm

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
--

From: Roland Dreier <rdreier@...>

To: Jonathan Corbet <corbet@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Monday, May 19, 2008 - 6:18 pm

> 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.
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

--

From: Alan Cox <alan@...>

To: Roland Dreier <rdreier@...>

Cc: Jonathan Corbet <corbet@...>, Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Tuesday, May 20, 2008 - 4:26 am

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,

Exactly.

Alan
--

To: Roland Dreier <rdreier@...>

Cc: Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>

Date: Monday, May 19, 2008 - 6:56 pm

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

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
--

From: Jeff Dike <jdike@...>

To: Jonathan Corbet <corbet@...>

Cc: Roland Dreier <rdreier@...>, Linus Torvalds <torvalds@...>, Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Peter Zijlstra <a.p.zijlstra@...>, Thomas Gleixner <tglx@...>, Alan Cox <alan@...>, Alexander Viro <viro@...>, <linux-kernel@...>, Stephen Rothwell <sfr@...>