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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
PS: We have the exact same pattern in arch/x86/kernel/msr.c:msr_open()
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 [email protected] 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 <[email protected]>
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);
+
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 [email protected] 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 <[email protected]>
> 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.
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 <[email protected]>
Cc: John Kacur <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Sven-Thorsten Dietrich <[email protected]>
---
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
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
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 <[email protected]>
> 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 <[email protected]>
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.
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 <[email protected]>
> Cc: John Kacur <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Sven-Thorsten Dietrich <[email protected]>
> ---
> 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 <[email protected]>
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
Commit-ID: d6c304055b3cecd4ca865769ac7cea97a320727b
Gitweb: http://git.kernel.org/tip/d6c304055b3cecd4ca865769ac7cea97a320727b
Author: Frederic Weisbecker <[email protected]>
AuthorDate: Wed, 7 Oct 2009 21:43:22 +0200
Committer: H. Peter Anvin <[email protected]>
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 <[email protected]>
Cc: John Kacur <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Sven-Thorsten Dietrich <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
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;
}
/*
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 :-)
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?
Commit-ID: 170a0bc3808909d8ea0f3f9c725c6565efe7f9c4
Gitweb: http://git.kernel.org/tip/170a0bc3808909d8ea0f3f9c725c6565efe7f9c4
Author: John Kacur <[email protected]>
AuthorDate: Wed, 7 Oct 2009 20:19:32 +0200
Committer: H. Peter Anvin <[email protected]>
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 <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
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;
}
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
----- "Arnd Bergmann" <[email protected]> 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
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 :)