[PATCH RFC] BKL not necessary in cpuid_open

[PATCH RFC] BKL not necessary in cpuid_open

[John Kacur]

I've been staring at the BKL lock in cpuid_open, and I can't see what it 
is protecting. However, I may have missed something - even something 
obvious, so comments are welcome.

>From 25c0f07b3ec5533c0e690e06198baa4300ee4a8c Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Wed, 7 Oct 2009 20:06:12 +0200
Subject: [PATCH] The BKL is not necessary in cpuid_open
 Most of the variables are local to the function. It IS possible that for
 struct cpuinfo_x86 *c
 c could point to the same area. However, this is used read only.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 arch/x86/kernel/cpuid.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpuid.c b/arch/x86/kernel/cpuid.c
index 6a52d4b..8bb8401 100644
--- a/arch/x86/kernel/cpuid.c
+++ b/arch/x86/kernel/cpuid.c
@@ -118,8 +118,6 @@ static int cpuid_open(struct inode *inode, struct file *file)
 	struct cpuinfo_x86 *c;
 	int ret = 0;
 
-	lock_kernel();
-
 	cpu = iminor(file->f_path.dentry->d_inode);
 	if (cpu >= nr_cpu_ids || !cpu_online(cpu)) {
 		ret = -ENXIO;	/* No such CPU */
@@ -129,7 +127,6 @@ static int cpuid_open(struct inode *inode, struct file *file)
 	if (c->cpuid_level < 0)
 		ret = -EIO;	/* CPUID not supported */
 out:
-	unlock_kernel();
 	return ret;
 }
 
-- 
1.6.0.6

Re: [PATCH RFC] BKL not necessary in cpuid_open

[Frederic Weisbecker]

On Wed, Oct 07, 2009 at 08:19:32PM +0200, John Kacur wrote:
> 
> I've been staring at the BKL lock in cpuid_open, and I can't see what it 
> is protecting. However, I may have missed something - even something 
> obvious, so comments are welcome.
> 
> From 25c0f07b3ec5533c0e690e06198baa4300ee4a8c Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Wed, 7 Oct 2009 20:06:12 +0200
> Subject: [PATCH] The BKL is not necessary in cpuid_open
>  Most of the variables are local to the function. It IS possible that for
>  struct cpuinfo_x86 *c
>  c could point to the same area. However, this is used read only.
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
>  arch/x86/kernel/cpuid.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpuid.c b/arch/x86/kernel/cpuid.c
> index 6a52d4b..8bb8401 100644
> --- a/arch/x86/kernel/cpuid.c
> +++ b/arch/x86/kernel/cpuid.c
> @@ -118,8 +118,6 @@ static int cpuid_open(struct inode *inode, struct file *file)
>  	struct cpuinfo_x86 *c;
>  	int ret = 0;
>  
> -	lock_kernel();
> -
>  	cpu = iminor(file->f_path.dentry->d_inode);
>  	if (cpu >= nr_cpu_ids || !cpu_online(cpu)) {
>  		ret = -ENXIO;	/* No such CPU */
> @@ -129,7 +127,6 @@ static int cpuid_open(struct inode *inode, struct file *file)
>  	if (c->cpuid_level < 0)
>  		ret = -EIO;	/* CPUID not supported */
>  out:
> -	unlock_kernel();
>  	return ret;
>  }



Everything look safe there.

Looking at what happens if the targeted cpu is removed:
I don't know if device_destroy() waits for the last reader to
complete on the given device file, but if it does, it's really
safe, if not:

- The cpu_data(cpu) won't be released anyway, only some values inside
  c will be changed, wich is not that a big issue, and the bkl
  doesn't help about that either

- We just checked if the cpu is online. It can be put offline
  just after. Whatever the bkl or not, it can also be put offline
  between open and read calls.

So I think this bkl doesn't protect anything.

Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>

PS: We have the exact same pattern in arch/x86/kernel/msr.c:msr_open()

Re: [PATCH RFC] BKL not necessary in cpuid_open

[Sven-Thorsten Dietrich]

On Wed, 2009-10-07 at 20:19 +0200, John Kacur wrote:
> I've been staring at the BKL lock in cpuid_open, and I can't see what it 
> is protecting. However, I may have missed something - even something 
> obvious, so comments are welcome.

I have been using this patch to first see if the BKL is being used
simply as mutex, which would allow easier break-down.

Sven

Subject: Introduce the BKL-MUTEX, to allow proving the code that
From: Sven-Thorsten Dietrich sdietrich@suse.de Sat Oct 3 01:26:01 2009 -0700
Date: Sat Oct 3 01:26:01 2009 -0700:
Git: 4bfea26de550100d193c49278d657485e792dfe5

just needs a global kernel mutex, without all the funky
preemption bits...

Acked-by: Sven-Thorsten Dietrich <sdietrich@suse.de>
diff --git a/include/linux/smp_mutex.h b/include/linux/smp_mutex.h
new file mode 100644
index 0000000..6c45a96
--- /dev/null
+++ b/include/linux/smp_mutex.h
@@ -0,0 +1,22 @@
+#ifndef __LINUX_SMPMUTEX_H
+#define __LINUX_SMPMUTEX_H
+
+#ifdef CONFIG_LOCK_KERNEL
+
+extern void lock_kernel_mutex(void);
+extern void unlock_kernel_mutex(void);
+
+static inline void cycle_kernel_mutex(void)
+{
+	lock_kernel_mutex();
+	unlock_kernel_mutex();
+}
+
+#else
+
+#define lock_kernel_mutex()				do { } while(0)
+#define unlock_kernel_mutex()				do { } while(0)
+#define cycle_kernel_mutex()			do { } while(0)
+
+#endif /* CONFIG_LOCK_KERNEL */
+#endif /* __LINUX_SMPMUTEX_H */
diff --git a/lib/Makefile b/lib/Makefile
index 2e78277..c71ffdc 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -40,7 +40,7 @@ lib-$(CONFIG_GENERIC_FIND_FIRST_BIT) += find_next_bit.o
 lib-$(CONFIG_GENERIC_FIND_NEXT_BIT) += find_next_bit.o
 obj-$(CONFIG_GENERIC_FIND_LAST_BIT) += find_last_bit.o
 obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
-obj-$(CONFIG_LOCK_KERNEL) += kernel_lock.o
+obj-$(CONFIG_LOCK_KERNEL) += kernel_lock.o kernel_mutex.o
 obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
 obj-$(CONFIG_DEBUG_LIST) += list_debug.o
 obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
diff --git a/lib/kernel_mutex.c b/lib/kernel_mutex.c
new file mode 100644
index 0000000..1d587a8
--- /dev/null
+++ b/lib/kernel_mutex.c
@@ -0,0 +1,37 @@
+/*
+ * lib/kernel_mutex.c
+ *
+ * This is the preemptible BKL - To be used transitionally to
+ * prove those subsystems still using lock_kernel, but really
+ * requiring a global mutex.
+ */
+#include <linux/smp_mutex.h>
+#include <linux/module.h>
+#include <linux/kallsyms.h>
+#include <linux/semaphore.h>
+#include <linux/mutex.h>
+
+/*
+ * The 'big kernel mutex'
+ *
+ * Don't use in new code.
+ */
+static  __cacheline_aligned_in_smp DEFINE_MUTEX(kernel_mutex);
+
+
+/*
+ * Getting the kernel mutex.
+ */
+void __lockfunc lock_kernel_mutex(void)
+{
+	mutex_lock(&kernel_mutex);
+}
+
+void __lockfunc unlock_kernel_mutex(void)
+{
+	mutex_unlock(&kernel_mutex);
+}
+
+EXPORT_SYMBOL(lock_kernel_mutex);
+EXPORT_SYMBOL(unlock_kernel_mutex);
+

Re: [PATCH RFC] BKL not necessary in cpuid_open

[John Kacur]

On Wed, 7 Oct 2009, Sven-Thorsten Dietrich wrote:

> On Wed, 2009-10-07 at 20:19 +0200, John Kacur wrote:
> > I've been staring at the BKL lock in cpuid_open, and I can't see what it 
> > is protecting. However, I may have missed something - even something 
> > obvious, so comments are welcome.
> 
> I have been using this patch to first see if the BKL is being used
> simply as mutex, which would allow easier break-down.
> 
> Sven
> 
> Subject: Introduce the BKL-MUTEX, to allow proving the code that
> From: Sven-Thorsten Dietrich sdietrich@suse.de Sat Oct 3 01:26:01 2009 -0700
> Date: Sat Oct 3 01:26:01 2009 -0700:
> Git: 4bfea26de550100d193c49278d657485e792dfe5
> 
> just needs a global kernel mutex, without all the funky
> preemption bits...
> 
> Acked-by: Sven-Thorsten Dietrich <sdietrich@suse.de>
> diff --git a/include/linux/smp_mutex.h b/include/linux/smp_mutex.h
> new file mode 100644
> index 0000000..6c45a96
> --- /dev/null
> +++ b/include/linux/smp_mutex.h
> @@ -0,0 +1,22 @@
> +#ifndef __LINUX_SMPMUTEX_H
> +#define __LINUX_SMPMUTEX_H
> +
> +#ifdef CONFIG_LOCK_KERNEL
> +
> +extern void lock_kernel_mutex(void);
> +extern void unlock_kernel_mutex(void);
> +
> +static inline void cycle_kernel_mutex(void)
> +{
> +	lock_kernel_mutex();
> +	unlock_kernel_mutex();
> +}
> +
> +#else
> +
> +#define lock_kernel_mutex()				do { } while(0)
> +#define unlock_kernel_mutex()				do { } while(0)
> +#define cycle_kernel_mutex()			do { } while(0)
> +
> +#endif /* CONFIG_LOCK_KERNEL */
> +#endif /* __LINUX_SMPMUTEX_H */
> diff --git a/lib/Makefile b/lib/Makefile
> index 2e78277..c71ffdc 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -40,7 +40,7 @@ lib-$(CONFIG_GENERIC_FIND_FIRST_BIT) += find_next_bit.o
>  lib-$(CONFIG_GENERIC_FIND_NEXT_BIT) += find_next_bit.o
>  obj-$(CONFIG_GENERIC_FIND_LAST_BIT) += find_last_bit.o
>  obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
> -obj-$(CONFIG_LOCK_KERNEL) += kernel_lock.o
> +obj-$(CONFIG_LOCK_KERNEL) += kernel_lock.o kernel_mutex.o
>  obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
>  obj-$(CONFIG_DEBUG_LIST) += list_debug.o
>  obj-$(CONFIG_DEBUG_OBJECTS) += debugobjects.o
> diff --git a/lib/kernel_mutex.c b/lib/kernel_mutex.c
> new file mode 100644
> index 0000000..1d587a8
> --- /dev/null
> +++ b/lib/kernel_mutex.c
> @@ -0,0 +1,37 @@
> +/*
> + * lib/kernel_mutex.c
> + *
> + * This is the preemptible BKL - To be used transitionally to
> + * prove those subsystems still using lock_kernel, but really
> + * requiring a global mutex.
> + */
> +#include <linux/smp_mutex.h>
> +#include <linux/module.h>
> +#include <linux/kallsyms.h>
> +#include <linux/semaphore.h>
> +#include <linux/mutex.h>
> +
> +/*
> + * The 'big kernel mutex'
> + *
> + * Don't use in new code.
> + */
> +static  __cacheline_aligned_in_smp DEFINE_MUTEX(kernel_mutex);
> +
> +
> +/*
> + * Getting the kernel mutex.
> + */
> +void __lockfunc lock_kernel_mutex(void)
> +{
> +	mutex_lock(&kernel_mutex);
> +}
> +
> +void __lockfunc unlock_kernel_mutex(void)
> +{
> +	mutex_unlock(&kernel_mutex);
> +}
> +
> +EXPORT_SYMBOL(lock_kernel_mutex);
> +EXPORT_SYMBOL(unlock_kernel_mutex);
> +
> 

Cool! Seems like an excellent experiment. However this is a separate patch
from the one initially proposed in this thread. I'm willing to risk just 
removing it in this case without any intermediary step. However, if anyone 
points out to me why I'm a knuckle head and missed something obvious - I'll 
listen. Otherwise, let's use your patch as a separate tactic to kill BKL.

Re: [PATCH RFC] BKL not necessary in cpuid_open

[Sven-Thorsten Dietrich]

On Wed, 2009-10-07 at 21:31 +0200, John Kacur wrote:
> 
> On Wed, 7 Oct 2009, Sven-Thorsten Dietrich wrote:
> 
> > On Wed, 2009-10-07 at 20:19 +0200, John Kacur wrote:
> > > I've been staring at the BKL lock in cpuid_open, and I can't see what it 
> > > is protecting. However, I may have missed something - even something 
> > > obvious, so comments are welcome.
> > 
> > I have been using this patch to first see if the BKL is being used
> > simply as mutex, which would allow easier break-down.
> > 
> > Sven
> > 

> Cool! Seems like an excellent experiment. However this is a separate patch
> from the one initially proposed in this thread. I'm willing to risk just 
> removing it in this case without any intermediary step. However, if anyone 
> points out to me why I'm a knuckle head and missed something obvious - I'll 
> listen. Otherwise, let's use your patch as a separate tactic to kill BKL.
> 

Yes, I meant to send this out as RFC Monday, but got side-tracked with
catch-up work, so you prompted me to just reply to your patch.

I was also looking at the cycle_kernel_lock() call in 
arch/x86/kernel/microcode_core.c, which is not obvious to me.

I converted that to cycle_kernel_mutex() using the patch I sent earlier,
but have not had time to actually boot and test.

In any case, I see bkl accesses all over various driver open() and
ioctl() calls.

I think that a number of these are safe to remove, as I still fail to
understand why its necessary to take BKL during any driver open()
routing.

So if its fine for the cpuid_open() call, then I would assume its ok for
others.

Sven

[PATCH] x86: Remove the bkl from msr_open()

[Frederic Weisbecker]

Remove the big kernel lock from msr_open() as it doesn't protect
anything there.

The only racy event that can happen here is a concurrent cpu shutdown.

So let's look at what could be racy during/after the above event:

- The cpu_online() check is racy, but the bkl doesn't help about
  that anyway it disables preemption but we may be chcking another
  cpu than the current one.
  Also the cpu can still become offlined between open and read calls.

- The cpu_data(cpu) returns a safe pointer too. It won't be released on
  cpu offlining. But some fields can be changed from
  arch/x86/kernel/smpboot.c:remove_siblinginfo() :

	- phys_proc_id
	- cpu_core_id

  Those are not read from msr_open(). What we are checking is the
  x86_capability that is left untouched on offlining.

So this removal looks safe.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: John Kacur <jkacur@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Sven-Thorsten Dietrich <sdietrich@suse.de>
---
 arch/x86/kernel/msr.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index 6a3cefc..5534499 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -174,21 +174,17 @@ static int msr_open(struct inode *inode, struct file *file)
 {
 	unsigned int cpu = iminor(file->f_path.dentry->d_inode);
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
-	int ret = 0;
 
-	lock_kernel();
 	cpu = iminor(file->f_path.dentry->d_inode);
 
-	if (cpu >= nr_cpu_ids || !cpu_online(cpu)) {
-		ret = -ENXIO;	/* No such CPU */
-		goto out;
-	}
+	if (cpu >= nr_cpu_ids || !cpu_online(cpu))
+		return -ENXIO;	/* No such CPU */
+
 	c = &cpu_data(cpu);
 	if (!cpu_has(c, X86_FEATURE_MSR))
-		ret = -EIO;	/* MSR not supported */
-out:
-	unlock_kernel();
-	return ret;
+		return -EIO;	/* MSR not supported */
+
+	return 0;
 }
 
 /*
-- 
1.6.2.3

Re: [PATCH] x86: Remove the bkl from msr_open()

[John Kacur]

On Wed, 7 Oct 2009, Frederic Weisbecker wrote:

> Remove the big kernel lock from msr_open() as it doesn't protect
> anything there.
> 
> The only racy event that can happen here is a concurrent cpu shutdown.
> 
> So let's look at what could be racy during/after the above event:
> 
> - The cpu_online() check is racy, but the bkl doesn't help about
>   that anyway it disables preemption but we may be chcking another
>   cpu than the current one.
>   Also the cpu can still become offlined between open and read calls.
> 
> - The cpu_data(cpu) returns a safe pointer too. It won't be released on
>   cpu offlining. But some fields can be changed from
>   arch/x86/kernel/smpboot.c:remove_siblinginfo() :
> 
> 	- phys_proc_id
> 	- cpu_core_id
> 
>   Those are not read from msr_open(). What we are checking is the
>   x86_capability that is left untouched on offlining.
> 
> So this removal looks safe.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: John Kacur <jkacur@redhat.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Sven-Thorsten Dietrich <sdietrich@suse.de>
> ---
>  arch/x86/kernel/msr.c |   16 ++++++----------
>  1 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
> index 6a3cefc..5534499 100644
> --- a/arch/x86/kernel/msr.c
> +++ b/arch/x86/kernel/msr.c
> @@ -174,21 +174,17 @@ static int msr_open(struct inode *inode, struct file *file)
>  {
>  	unsigned int cpu = iminor(file->f_path.dentry->d_inode);
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
> -	int ret = 0;
>  
> -	lock_kernel();
>  	cpu = iminor(file->f_path.dentry->d_inode);
>  
> -	if (cpu >= nr_cpu_ids || !cpu_online(cpu)) {
> -		ret = -ENXIO;	/* No such CPU */
> -		goto out;
> -	}
> +	if (cpu >= nr_cpu_ids || !cpu_online(cpu))
> +		return -ENXIO;	/* No such CPU */
> +
>  	c = &cpu_data(cpu);
>  	if (!cpu_has(c, X86_FEATURE_MSR))
> -		ret = -EIO;	/* MSR not supported */
> -out:
> -	unlock_kernel();
> -	return ret;
> +		return -EIO;	/* MSR not supported */
> +
> +	return 0;
>  }
>  
>  /*
> -- 
> 1.6.2.3
> 
> 

This case looks very similar to the cpuid_open one.
Reviewed-by: John Kacur <jkacur@redhat.com>

[tip:x86/cpu] x86, msr: Remove the bkl from msr_open()

[tip-bot for Frederic Weisbecker]

Commit-ID:  d6c304055b3cecd4ca865769ac7cea97a320727b
Gitweb:     http://git.kernel.org/tip/d6c304055b3cecd4ca865769ac7cea97a320727b
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Wed, 7 Oct 2009 21:43:22 +0200
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Wed, 7 Oct 2009 13:47:19 -0700

x86, msr: Remove the bkl from msr_open()

Remove the big kernel lock from msr_open() as it doesn't protect
anything there.

The only racy event that can happen here is a concurrent cpu shutdown.

So let's look at what could be racy during/after the above event:

- The cpu_online() check is racy, but the bkl doesn't help about
  that anyway it disables preemption but we may be chcking another
  cpu than the current one.
  Also the cpu can still become offlined between open and read calls.

- The cpu_data(cpu) returns a safe pointer too. It won't be released on
  cpu offlining. But some fields can be changed from
  arch/x86/kernel/smpboot.c:remove_siblinginfo() :

	- phys_proc_id
	- cpu_core_id

  Those are not read from msr_open(). What we are checking is the
  x86_capability that is left untouched on offlining.

So this removal looks safe.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: John Kacur <jkacur@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sven-Thorsten Dietrich <sdietrich@suse.de>
LKML-Reference: <1254944602-7382-1-git-send-email-fweisbec@gmail.com>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/kernel/msr.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index 7dd9500..c006109 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -174,21 +174,17 @@ static int msr_open(struct inode *inode, struct file *file)
 {
 	unsigned int cpu = iminor(file->f_path.dentry->d_inode);
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
-	int ret = 0;
 
-	lock_kernel();
 	cpu = iminor(file->f_path.dentry->d_inode);
 
-	if (cpu >= nr_cpu_ids || !cpu_online(cpu)) {
-		ret = -ENXIO;	/* No such CPU */
-		goto out;
-	}
+	if (cpu >= nr_cpu_ids || !cpu_online(cpu))
+		return -ENXIO;	/* No such CPU */
+
 	c = &cpu_data(cpu);
 	if (!cpu_has(c, X86_FEATURE_MSR))
-		ret = -EIO;	/* MSR not supported */
-out:
-	unlock_kernel();
-	return ret;
+		return -EIO;	/* MSR not supported */
+
+	return 0;
 }
 
 /*

Re: [PATCH RFC] BKL not necessary in cpuid_open

[Frederic Weisbecker]

On Wed, Oct 07, 2009 at 08:19:32PM +0200, John Kacur wrote:
> 
> I've been staring at the BKL lock in cpuid_open, and I can't see what it 
> is protecting. However, I may have missed something - even something 
> obvious, so comments are welcome.
> 
> From 25c0f07b3ec5533c0e690e06198baa4300ee4a8c Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Wed, 7 Oct 2009 20:06:12 +0200
> Subject: [PATCH] The BKL is not necessary in cpuid_open
>  Most of the variables are local to the function. It IS possible that for
>  struct cpuinfo_x86 *c
>  c could point to the same area. However, this is used read only.
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>


Hmm, I'm discovering that in tip:rt/kill-the-bkl

http://git.kernel.org/?p=linux/kernel/git/tip/linux-2.6-tip.git;a=commitdiff;h=55968ede164ae523692f00717f50cd926f1382a0

Looks like we have overlaped.

Thomas it would be nice to post these patches on LKML (or I missed
them?) and may be to merge them into tip:master, so that they are
visible and then we lower the risk of any duplicate works in this area.

Thanks.

Re: [PATCH RFC] BKL not necessary in cpuid_open

[Thomas Gleixner]

On Wed, 7 Oct 2009, Frederic Weisbecker wrote:
> Hmm, I'm discovering that in tip:rt/kill-the-bkl
> 
> http://git.kernel.org/?p=linux/kernel/git/tip/linux-2.6-tip.git;a=commitdiff;h=55968ede164ae523692f00717f50cd926f1382a0
> 
> Looks like we have overlaped.
> 
> Thomas it would be nice to post these patches on LKML (or I missed
> them?) and may be to merge them into tip:master, so that they are
> visible and then we lower the risk of any duplicate works in this area.

Now guess why I mentioned this branch sevaral times at the rt summit
last week. It's even documented in Jonathans excellent meeting
minutes on lwn.net :)

Thanks,

	tglx

Re: [PATCH RFC] BKL not necessary in cpuid_open

[Frederic Weisbecker]

On Wed, Oct 07, 2009 at 11:01:23PM +0200, Thomas Gleixner wrote:
> On Wed, 7 Oct 2009, Frederic Weisbecker wrote:
> > Hmm, I'm discovering that in tip:rt/kill-the-bkl
> > 
> > http://git.kernel.org/?p=linux/kernel/git/tip/linux-2.6-tip.git;a=commitdiff;h=55968ede164ae523692f00717f50cd926f1382a0
> > 
> > Looks like we have overlaped.
> > 
> > Thomas it would be nice to post these patches on LKML (or I missed
> > them?) and may be to merge them into tip:master, so that they are
> > visible and then we lower the risk of any duplicate works in this area.
> 
> Now guess why I mentioned this branch sevaral times at the rt summit
> last week. It's even documented in Jonathans excellent meeting
> minutes on lwn.net :)
> 
> Thanks,
> 
> 	tglx



Yeah, but you know, I very good when it comes to find/remember
something obvious only right after posting a patch (I rarely
posted a patch not followed by a v2 lately). A kind of
post-posting only memory, something that becomes common with me,
I guess I need to start learning how to live with it :)

That said, some developers who don't read lwn (or other kind of
people gifted by the post-patch-posting memory power) may come and try
to remove the bkl there, since it's not visible in lkml archives
or in the master branch of the x86 tree.

Ah, for example hpa has just committed my patch :-)

Re: [PATCH RFC] BKL not necessary in cpuid_open

[John Kacur]

On Wed, 7 Oct 2009, Thomas Gleixner wrote:

> On Wed, 7 Oct 2009, Frederic Weisbecker wrote:
> > Hmm, I'm discovering that in tip:rt/kill-the-bkl
> > 
> > http://git.kernel.org/?p=linux/kernel/git/tip/linux-2.6-tip.git;a=commitdiff;h=55968ede164ae523692f00717f50cd926f1382a0
> > 
> > Looks like we have overlaped.
> > 
> > Thomas it would be nice to post these patches on LKML (or I missed
> > them?) and may be to merge them into tip:master, so that they are
> > visible and then we lower the risk of any duplicate works in this area.
> 
> Now guess why I mentioned this branch sevaral times at the rt summit
> last week. It's even documented in Jonathans excellent meeting
> minutes on lwn.net :)
> 

Well, the overlap is unfortunate but I did do a git diff with 
tip/core/kill-the-BKL BEFORE I posted.

not sure what the difference is between
tip/kill-the-BKL
and 
tip/core/kill-the-BKL
btw

Also
git describe 55968ede164ae523692f00717f50cd926f1382a0
v2.6.31-rc6-4-g55968ed

Not sure what to make of that either, since my patch was against the 
latest linus updated this morning.

Frederic: what was your method to discover that patch from Thomas?

Re: [PATCH RFC] BKL not necessary in cpuid_open

[Frederic Weisbecker]

On Wed, Oct 07, 2009 at 11:58:20PM +0200, John Kacur wrote:
> 
> 
> On Wed, 7 Oct 2009, Thomas Gleixner wrote:
> 
> > On Wed, 7 Oct 2009, Frederic Weisbecker wrote:
> > > Hmm, I'm discovering that in tip:rt/kill-the-bkl
> > > 
> > > http://git.kernel.org/?p=linux/kernel/git/tip/linux-2.6-tip.git;a=commitdiff;h=55968ede164ae523692f00717f50cd926f1382a0
> > > 
> > > Looks like we have overlaped.
> > > 
> > > Thomas it would be nice to post these patches on LKML (or I missed
> > > them?) and may be to merge them into tip:master, so that they are
> > > visible and then we lower the risk of any duplicate works in this area.
> > 
> > Now guess why I mentioned this branch sevaral times at the rt summit
> > last week. It's even documented in Jonathans excellent meeting
> > minutes on lwn.net :)
> > 
> 
> Well, the overlap is unfortunate but I did do a git diff with 
> tip/core/kill-the-BKL BEFORE I posted.
> 
> not sure what the difference is between
> tip/kill-the-BKL
> and 
> tip/core/kill-the-BKL
> btw


Ah yeah, may be you've made a confusion.

tip/kill-bkl = tip/core/bkl = a branch created by Ingo inside which the
bkl has been turned into a regular mutex. This branch was helpful to find
the points where the bkl was acquired recursively, and how it
gained bad dependencies once turned into a regular sleeping lock because
it makes it analyzable by lockdep.

We did some work on this tree (various bkl pushdown and removals, also
the removal in reiserfs started there).
But this tree is not active currently, although it could still be (and its
key piece makes it useful to debug the bkl).

But currently, the tree that gathers the work in this area is rt/kill-the-bkl

Although it's possible Thomas might choose another name if it's scheduled
for .33 ?


 
> Also
> git describe 55968ede164ae523692f00717f50cd926f1382a0
> v2.6.31-rc6-4-g55968ed
> 
> Not sure what to make of that either, since my patch was against the 
> latest linus updated this morning.
> 
> Frederic: what was your method to discover that patch from Thomas?


I looked at the rt/kill-the-bkl in my local git :)

[tip:x86/cpu] x86, cpuid: Remove the bkl from cpuid_open()

[tip-bot for John Kacur]

Commit-ID:  170a0bc3808909d8ea0f3f9c725c6565efe7f9c4
Gitweb:     http://git.kernel.org/tip/170a0bc3808909d8ea0f3f9c725c6565efe7f9c4
Author:     John Kacur <jkacur@redhat.com>
AuthorDate: Wed, 7 Oct 2009 20:19:32 +0200
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Wed, 7 Oct 2009 15:41:21 -0700

x86, cpuid: Remove the bkl from cpuid_open()

Most of the variables are local to the function. It IS possible that
for struct cpuinfo_x86 *c c could point to the same area. However,
this is used read only.

Signed-off-by: John Kacur <jkacur@redhat.com>
LKML-Reference: <alpine.LFD.2.00.0910072016190.15183@localhost.localdomain>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/kernel/cpuid.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpuid.c b/arch/x86/kernel/cpuid.c
index b07af88..ef69284 100644
--- a/arch/x86/kernel/cpuid.c
+++ b/arch/x86/kernel/cpuid.c
@@ -118,8 +118,6 @@ static int cpuid_open(struct inode *inode, struct file *file)
 	struct cpuinfo_x86 *c;
 	int ret = 0;
 
-	lock_kernel();
-
 	cpu = iminor(file->f_path.dentry->d_inode);
 	if (cpu >= nr_cpu_ids || !cpu_online(cpu)) {
 		ret = -ENXIO;	/* No such CPU */
@@ -129,7 +127,6 @@ static int cpuid_open(struct inode *inode, struct file *file)
 	if (c->cpuid_level < 0)
 		ret = -EIO;	/* CPUID not supported */
 out:
-	unlock_kernel();
 	return ret;
 }
 

Re: [PATCH RFC] BKL not necessary in cpuid_open

[Arnd Bergmann]

On Wednesday 07 October 2009, John Kacur wrote:
> 
> I've been staring at the BKL lock in cpuid_open, and I can't see what it 
> is protecting. However, I may have missed something - even something 
> obvious, so comments are welcome.
> 

Hi John,

In general, the lock_kernel() calls in any chardev open() file operation
are the result of the BKL pushdown by Jon Corbet and others, which has happened
some time last year[1]. I'd assume that the vast majority is not needed
at all, so these are an easy target for removal.

	Arnd

[1] http://lkml.indiana.edu/hypermail/linux/kernel/0805.2/0257.html

Re: [PATCH RFC] BKL not necessary in cpuid_open

[John Kacur]

----- "Arnd Bergmann" <arnd@arndb.de> wrote:

> On Wednesday 07 October 2009, John Kacur wrote:
> > 
> > I've been staring at the BKL lock in cpuid_open, and I can't see
> what it 
> > is protecting. However, I may have missed something - even something
> 
> > obvious, so comments are welcome.
> > 
> 
> Hi John,
> 
> In general, the lock_kernel() calls in any chardev open() file
> operation
> are the result of the BKL pushdown by Jon Corbet and others, which has
> happened
> some time last year[1]. I'd assume that the vast majority is not
> needed
> at all, so these are an easy target for removal.
> 
> 	Arnd
> 
> [1] http://lkml.indiana.edu/hypermail/linux/kernel/0805.2/0257.html

yup - I'm aware of all the hard work that many people put in before I ever looked at this.
Thomas asked the -rt folks to have a look at removing the bkl look during the rt-mini summit
in Dresden. I was looking for low hanging fruit to get started. The confusion was over what
was already implemented in tip/rt/bkl. I merely looked at the latest linus/master build.
The result was a good one though, because basically Thomas' work got pushed from tip/rt/bkl
into tip/master, and can hopefully be pushed upstream during the next merge window.

Even if getting starting means just reviewing what is in rt/bkl and not yet in linus/master,
I think that would be positive. I'm sure Thomas will stomp on me if I start to annoy him. :)

Thanks

John