[PATCH 0/4] Phasing out the BKL

[PATCH 0/4] Phasing out the BKL

[Arnd Bergmann]

Hi Linus,

These are the remaining patches I have for moving
the big kernel lock further out of the view of
developers. I had originally made them for 2.6.37
and they have been in linux-next since.

None of them has any real significance at this point,
since there are few people using the remaining modules
that require the BKL (ipx, appletalk, x.25, ufs,
adfs, hpfs, i830-drm, and a few bits in staging).
I expect to see patches for the first four in that
list at some point in the near future, after that
I suppose we can remove the option along with the
remaining dependencies completely.

For now, please pick any patches you like out of
these four.

Cc: Dave Airlie <airlied@linux.ie>

Arnd Bergmann (4):
  drm/i810: remove the BKL
  BKL: disable by default
  BKL: mark lock_kernel as deprecated
  BKL: move CONFIG_BKL to staging

 drivers/gpu/drm/Kconfig         |    4 ++--
 drivers/gpu/drm/i810/i810_dma.c |   18 +-----------------
 drivers/gpu/drm/i810/i810_drv.c |    6 +++++-
 drivers/staging/Kconfig         |   10 ++++++++++
 include/linux/smp_lock.h        |   23 ++++++-----------------
 lib/Kconfig.debug               |    9 ---------
 6 files changed, 24 insertions(+), 46 deletions(-)

[PATCH 1/4] drm/i810: remove the BKL

[Arnd Bergmann]

SMP i810 systems were practically nonexistent and the configuration
was not officially supported by Intel at the time when Pentium-III
was common.

With this change, it is still possible to build a distribution kernel
that has support for SMP and includes the i810 driver without the BKL.
As a precaution, check for the theoretical SMP case at run time and
refuse to load the driver.

We also need to disable CONFIG_PREEMPT builds for this driver.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Dave Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/Kconfig         |    4 ++--
 drivers/gpu/drm/i810/i810_dma.c |   18 +-----------------
 drivers/gpu/drm/i810/i810_drv.c |    6 +++++-
 3 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 64828a7..4df921e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -73,8 +73,8 @@ source "drivers/gpu/drm/radeon/Kconfig"
 
 config DRM_I810
 	tristate "Intel I810"
-	# BKL usage in order to avoid AB-BA deadlocks, may become BROKEN_ON_SMP
-	depends on DRM && AGP && AGP_INTEL && BKL
+	# !PREEMPT because of missing ioctl locking
+	depends on DRM && AGP && AGP_INTEL && (!PREEMPT || BROKEN)
 	help
 	  Choose this option if you have an Intel I810 graphics card.  If M is
 	  selected, the module will be called i810.  AGP support is required
diff --git a/drivers/gpu/drm/i810/i810_dma.c b/drivers/gpu/drm/i810/i810_dma.c
index ff33e53..8f371e8 100644
--- a/drivers/gpu/drm/i810/i810_dma.c
+++ b/drivers/gpu/drm/i810/i810_dma.c
@@ -37,7 +37,6 @@
 #include <linux/interrupt.h>	/* For task queue support */
 #include <linux/delay.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 #include <linux/pagemap.h>
 
 #define I810_BUF_FREE		2
@@ -94,7 +93,6 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 	struct drm_buf *buf;
 	drm_i810_buf_priv_t *buf_priv;
 
-	lock_kernel();
 	dev = priv->minor->dev;
 	dev_priv = dev->dev_private;
 	buf = dev_priv->mmap_buffer;
@@ -104,7 +102,6 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 	vma->vm_file = filp;
 
 	buf_priv->currently_mapped = I810_BUF_MAPPED;
-	unlock_kernel();
 
 	if (io_remap_pfn_range(vma, vma->vm_start,
 			       vma->vm_pgoff,
@@ -116,7 +113,7 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 static const struct file_operations i810_buffer_fops = {
 	.open = drm_open,
 	.release = drm_release,
-	.unlocked_ioctl = i810_ioctl,
+	.unlocked_ioctl = drm_ioctl,
 	.mmap = i810_mmap_buffers,
 	.fasync = drm_fasync,
 	.llseek = noop_llseek,
@@ -1242,19 +1239,6 @@ int i810_driver_dma_quiescent(struct drm_device *dev)
 	return 0;
 }
 
-/*
- * call the drm_ioctl under the big kernel lock because
- * to lock against the i810_mmap_buffers function.
- */
-long i810_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	int ret;
-	lock_kernel();
-	ret = drm_ioctl(file, cmd, arg);
-	unlock_kernel();
-	return ret;
-}
-
 struct drm_ioctl_desc i810_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I810_INIT, i810_dma_init, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I810_VERTEX, i810_dma_vertex, DRM_AUTH|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c
index 88bcd33..0152fa2 100644
--- a/drivers/gpu/drm/i810/i810_drv.c
+++ b/drivers/gpu/drm/i810/i810_drv.c
@@ -57,7 +57,7 @@ static struct drm_driver driver = {
 		 .owner = THIS_MODULE,
 		 .open = drm_open,
 		 .release = drm_release,
-		 .unlocked_ioctl = i810_ioctl,
+		 .unlocked_ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
 		 .fasync = drm_fasync,
@@ -79,6 +79,10 @@ static struct drm_driver driver = {
 
 static int __init i810_init(void)
 {
+	if (num_possible_cpus() > 1) {
+		pr_err("drm/i810 does not support SMP\n");
+		return -EINVAL;
+	}
 	driver.num_ioctls = i810_max_ioctl;
 	return drm_init(&driver);
 }
-- 
1.7.1

[PATCH 2/4] BKL: disable by default

[Arnd Bergmann]

As discussed at the kernel summit, this change disables
the big kernel lock by default. It is still possible to
enable it in order to build the modules that use it.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 lib/Kconfig.debug |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2d05adb..f2a399c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -472,12 +472,11 @@ config DEBUG_MUTEXES
 
 config BKL
 	bool "Big Kernel Lock" if (SMP || PREEMPT)
-	default y
 	help
 	  This is the traditional lock that is used in old code instead
-	  of proper locking. All drivers that use the BKL should depend
-	  on this symbol.
-	  Say Y here unless you are working on removing the BKL.
+	  of proper locking. The big kernel lock will go away in 2.6.39,
+	  so all modules that still depend on it need to be changed or
+	  they will be removed as well.
 
 config DEBUG_LOCK_ALLOC
 	bool "Lock debugging: detect incorrect freeing of live locks"
-- 
1.7.1

[PATCH 3/4] BKL: mark lock_kernel as deprecated

[Arnd Bergmann]

No new code should use the big kernel lock, so
we should really emit compiler warnings to make
people building out of mainline code aware of
this.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/smp_lock.h |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/include/linux/smp_lock.h b/include/linux/smp_lock.h
index 3a19882..a06eb78 100644
--- a/include/linux/smp_lock.h
+++ b/include/linux/smp_lock.h
@@ -30,24 +30,14 @@ extern void __lockfunc
 _unlock_kernel(const char *func, const char *file, int line)
 __releases(kernel_lock);
 
-#define lock_kernel() do {					\
-	_lock_kernel(__func__, __FILE__, __LINE__);		\
-} while (0)
-
-#define unlock_kernel()	do {					\
-	_unlock_kernel(__func__, __FILE__, __LINE__);		\
-} while (0)
+static inline void __deprecated lock_kernel(void)
+{
+	_lock_kernel(__func__, __FILE__, __LINE__);
+}
 
-/*
- * Various legacy drivers don't really need the BKL in a specific
- * function, but they *do* need to know that the BKL became available.
- * This function just avoids wrapping a bunch of lock/unlock pairs
- * around code which doesn't really need it.
- */
-static inline void cycle_kernel_lock(void)
+static inline void __deprecated unlock_kernel(void)
 {
-	lock_kernel();
-	unlock_kernel();
+	_unlock_kernel(__func__, __FILE__, __LINE__);
 }
 
 #else
@@ -55,7 +45,6 @@ static inline void cycle_kernel_lock(void)
 #ifdef CONFIG_BKL /* provoke build bug if not set */
 #define lock_kernel()
 #define unlock_kernel()
-#define cycle_kernel_lock()			do { } while(0)
 #endif /* CONFIG_BKL */
 
 #define release_kernel_lock(task)		do { } while(0)
-- 
1.7.1

[PATCH 4/4] BKL: move CONFIG_BKL to staging

[Arnd Bergmann]

Instead of moving the actual code to staging for stuff that
depends on CONFIG_BKL, this moves just the configuration
option it depends on.

As a consequence, the remaining non-staging drivers that use
the BKL (i830, appletalk, ipx, x25, adfs, hpfs and ufs)
implicitly depend on staging until they get fixed.

This sets 2.6.39 as the arbitrary date when we actually remove
the option along with its last users.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/staging/Kconfig |   10 ++++++++++
 lib/Kconfig.debug       |    8 --------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
index 5c8fcfc..1327ccc 100644
--- a/drivers/staging/Kconfig
+++ b/drivers/staging/Kconfig
@@ -41,6 +41,16 @@ config STAGING_EXCLUDE_BUILD
 
 if !STAGING_EXCLUDE_BUILD
 
+config BKL
+	bool "Include drivers that depend on the Big Kernel Lock" if (SMP || PREEMPT)
+	help
+	  This is the traditional lock that is used in old code instead
+	  of proper locking. All drivers that use the BKL should depend
+	  on this symbol.
+	  A small number of drivers still require this. All drivers that
+	  use the BKL should be changed to a better serialisation method
+	  or they will be removed in 2.6.39.
+
 source "drivers/staging/et131x/Kconfig"
 
 source "drivers/staging/slicoss/Kconfig"
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index f2a399c..4a78f8c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -470,14 +470,6 @@ config DEBUG_MUTEXES
 	 This feature allows mutex semantics violations to be detected and
 	 reported.
 
-config BKL
-	bool "Big Kernel Lock" if (SMP || PREEMPT)
-	help
-	  This is the traditional lock that is used in old code instead
-	  of proper locking. The big kernel lock will go away in 2.6.39,
-	  so all modules that still depend on it need to be changed or
-	  they will be removed as well.
-
 config DEBUG_LOCK_ALLOC
 	bool "Lock debugging: detect incorrect freeing of live locks"
 	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
-- 
1.7.1

Re: [PATCH 4/4] BKL: move CONFIG_BKL to staging

[Nick Bowler]

On 2011-01-18 22:17 +0100, Arnd Bergmann wrote:
> Instead of moving the actual code to staging for stuff that
> depends on CONFIG_BKL, this moves just the configuration
> option it depends on.
> 
> As a consequence, the remaining non-staging drivers that use
> the BKL (i830, appletalk, ipx, x25, adfs, hpfs and ufs)
> implicitly depend on staging until they get fixed.
> 
> This sets 2.6.39 as the arbitrary date when we actually remove
> the option along with its last users.

I think this patch is not very nice.  It will cause working kernel
configurations to turn into broken kernel configurations when the user
does 'make oldconfig', with no warning.

These drivers that use the BKL work fine.  Removing working features
with no adequate replacement available seems like a serious regression
to me.

-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: [PATCH 4/4] BKL: move CONFIG_BKL to staging

[Arnd Bergmann]

On Wednesday 19 January 2011, Nick Bowler wrote:
> I think this patch is not very nice.  It will cause working kernel
> configurations to turn into broken kernel configurations when the user
> does 'make oldconfig', with no warning.
> 
> These drivers that use the BKL work fine.  Removing working features
> with no adequate replacement available seems like a serious regression
> to me.

I wouldn't call it a serious regression since the code is still there
and both the symptom and the solution are rather obvious. What's more
important is to make any people still relying on the code aware that
it's going away unless someone fixes it, so they have the chance to
send patches or pay someone to fix it for them.

The remaining drivers that are still relying on the BKL are very
rarely used and for the less obscure ones (ufs, ipx and x.25), people
have volunteered to fix them (though I have seen no proper patches
for these yet). For the rest, I suppose if nobody complains, they
can actually go away, according to the logic that if nobody is using
them, they most likely are broken anyway and more a security risk than
they are worth.

Anyway, it seems that you're lucky this time, because I managed to miss
the merge window by being away from my test box for the most part of it.

	Arnd

Re: [PATCH 4/4] BKL: move CONFIG_BKL to staging

[Nick Bowler]

On 2011-01-19 17:17 +0100, Arnd Bergmann wrote:
> On Wednesday 19 January 2011, Nick Bowler wrote:
> > I think this patch is not very nice.  It will cause working kernel
> > configurations to turn into broken kernel configurations when the user
> > does 'make oldconfig', with no warning.
> > 
> > These drivers that use the BKL work fine.  Removing working features
> > with no adequate replacement available seems like a serious regression
> > to me.
> 
> I wouldn't call it a serious regression since the code is still there
> and both the symptom and the solution are rather obvious.

Well, the code wouldn't still be there if they're removed in 2.6.39 as
stated in your patch description.  While it may be obvious to people
like you and me who know what the BKL is, I don't it's obvious to
everyone that the cause of "my system doesn't boot anymore" is "oh,
someone moved a dependency of a driver I've been using without issue for
the past 5 years to staging!"

> What's more important is to make any people still relying on the code
> aware that it's going away unless someone fixes it, so they have the
> chance to send patches or pay someone to fix it for them.

Shouldn't the onus for fixing *working* drivers (or encouraging others
to fix them) lie with the person(s) who are so keen to kill off features
that they use?

Surely we don't need to break oldconfig just to raise awareness that
these drivers use deprecated features?

> The remaining drivers that are still relying on the BKL are very
> rarely used and for the less obscure ones (ufs, ipx and x.25), people
> have volunteered to fix them (though I have seen no proper patches
> for these yet).
>
> For the rest, I suppose if nobody complains, they can actually go
> away, according to the logic that if nobody is using them, they most
> likely are broken anyway and more a security risk than they are worth.

Notwithstanding any of the above, one release cycle hardly seems like
enough time to infer from "nobody complained" that "not a single person
uses this driver."  Surely people concerned with security issues in code
that they don't use can just... not enable it?  Nobody's forcing anyone
to use these drivers.

Neither the BKL nor any of these drivers are even mentioned in the
feature removal schedule yet.

-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: [PATCH 4/4] BKL: move CONFIG_BKL to staging

[Andrew Hendry]

Most the x.25 BKLs have patches accepted over the past few months.
There are 3 remaining in send, receive and destroy that I haven't had
time to work through. If anyone wants to take a look at them I can
help test, but I wont have time until march to focus on them myself.

Andrew

On Thu, Jan 20, 2011 at 5:21 AM, Nick Bowler <nbowler@elliptictech.com> wrote:
> On 2011-01-19 17:17 +0100, Arnd Bergmann wrote:
>> On Wednesday 19 January 2011, Nick Bowler wrote:
>> > I think this patch is not very nice.  It will cause working kernel
>> > configurations to turn into broken kernel configurations when the user
>> > does 'make oldconfig', with no warning.
>> >
>> > These drivers that use the BKL work fine.  Removing working features
>> > with no adequate replacement available seems like a serious regression
>> > to me.
>>
>> I wouldn't call it a serious regression since the code is still there
>> and both the symptom and the solution are rather obvious.
>
> Well, the code wouldn't still be there if they're removed in 2.6.39 as
> stated in your patch description.  While it may be obvious to people
> like you and me who know what the BKL is, I don't it's obvious to
> everyone that the cause of "my system doesn't boot anymore" is "oh,
> someone moved a dependency of a driver I've been using without issue for
> the past 5 years to staging!"
>
>> What's more important is to make any people still relying on the code
>> aware that it's going away unless someone fixes it, so they have the
>> chance to send patches or pay someone to fix it for them.
>
> Shouldn't the onus for fixing *working* drivers (or encouraging others
> to fix them) lie with the person(s) who are so keen to kill off features
> that they use?
>
> Surely we don't need to break oldconfig just to raise awareness that
> these drivers use deprecated features?
>
>> The remaining drivers that are still relying on the BKL are very
>> rarely used and for the less obscure ones (ufs, ipx and x.25), people
>> have volunteered to fix them (though I have seen no proper patches
>> for these yet).
>>
>> For the rest, I suppose if nobody complains, they can actually go
>> away, according to the logic that if nobody is using them, they most
>> likely are broken anyway and more a security risk than they are worth.
>
> Notwithstanding any of the above, one release cycle hardly seems like
> enough time to infer from "nobody complained" that "not a single person
> uses this driver."  Surely people concerned with security issues in code
> that they don't use can just... not enable it?  Nobody's forcing anyone
> to use these drivers.
>
> Neither the BKL nor any of these drivers are even mentioned in the
> feature removal schedule yet.
>
> --
> Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 4/4] BKL: move CONFIG_BKL to staging

[Arnd Bergmann]

On Sunday 23 January 2011, Andrew Hendry wrote:
> Most the x.25 BKLs have patches accepted over the past few months.
> There are 3 remaining in send, recieve and destroy that I haven't had time
> to work through. If anyone wants to take a look at them I can help test, but
> I wont have time until march to focus on them myself.

I've given it a try on the weekend, see my bkl.git tree on kernel.org.
I have patches for everything except IPX and UFS now, the latter was
giving me a hard time but I think I'm getting close there. IPX
should not be too hard.

	ARnd

Re: [PATCH 4/4] BKL: move CONFIG_BKL to staging

[Andrew Hendry]

Might be looking in the wrong spot, but I can't see recent x.25
changes at http://git.kernel.org/?p=linux/kernel/git/arnd/bkl.git
If that is current make sure to pickup the previous x.25 bkl patches
submitted in december via net-next to avoid duplicate effort.

Andrew.

On Tue, Jan 25, 2011 at 2:59 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday 23 January 2011, Andrew Hendry wrote:
>> Most the x.25 BKLs have patches accepted over the past few months.
>> There are 3 remaining in send, recieve and destroy that I haven't had time
>> to work through. If anyone wants to take a look at them I can help test, but
>> I wont have time until march to focus on them myself.
>
> I've given it a try on the weekend, see my bkl.git tree on kernel.org.
> I have patches for everything except IPX and UFS now, the latter was
> giving me a hard time but I think I'm getting close there. IPX
> should not be too hard.
>
>        ARnd
>

Re: [PATCH 4/4] BKL: move CONFIG_BKL to staging

[Arnd Bergmann]

On Tuesday 25 January 2011, Andrew Hendry wrote:
> Might be looking in the wrong spot, but I can't see recent x.25
> changes at http://git.kernel.org/?p=linux/kernel/git/arnd/bkl.git
> If that is current make sure to pickup the previous x.25 bkl patches
> submitted in december via net-next to avoid duplicate effort.

It's in the "final" branch:

http://git.kernel.org/?p=linux/kernel/git/arnd/bkl.git;a=shortlog;h=refs/heads/final
http://git.kernel.org/?p=linux/kernel/git/arnd/bkl.git;a=commitdiff;h=a16147906a479dda4628fc52febd49db526699d5

	Arnd