kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()
Subsystems (3)
PATCH 1/1 kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()
Fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec().
--- kernel/kexec.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
kernel/kexec.c +3 -1
@@ -1433,6 +1433,7 @@ module_init(crash_save_vmcoreinfo_init)int kernel_kexec (void ) {int error =0 ; +int locked;if (xchg (&kexec_lock,1 ))return -EBUSY ;
@@ -1498,7 +1499,8 @@ int kernel_kexec(void)#endif Unlock: -xchg (&kexec_lock,0 ); + locked =xchg (&kexec_lock,0 ); +BUG_ON (!locked);return error; }
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Andrew Morton
maintainerQuoted 2 lines · On Wed, 13 Aug 2008 17:12:40 +0800 Huang Ying <[email protected]> wrote:
Fix compiling warning on
xchg(&kexec_lock, 0)inkernel_kexec().
Would prefer that thi code not use such a peculair idiom. I don't believe that it needs to.
I guess that's a separate activity.
Quoted 17 lines
--- kernel/kexec.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)kernel/kexec.c +1
-0@@ -1433,6 +1433,7 @@ module_init(crash_save_vmcoreinfo_init)int kernel_kexec (void ) {int error =0 ; +int locked;if (xchg (&kexec_lock,1 ))return -EBUSY ;
kernel/kexec.c int kernel_kexec(void) · line 1506
Quoted 9 lines
kernel/kexec.c +2
-1@@ -1498,7 +1499,8 @@ int kernel_kexec(void)#endif Unlock: -xchg (&kexec_lock,0 ); + locked =xchg (&kexec_lock,0 ); +BUG_ON (!locked);return error; }
Please always quote the compiler output in the changelog when fixing warnings and build errors.
The patch is titled "kexec jump: ..." whereas this is just a plain old kexec fix, which is applicable to mainline.
We don't need to create that local. I queued this:
Subject: kexec: fix compilation warning on xchg(&kexec_lock, 0) in kernel_kexec()
From: Huang Ying <[email protected]>
kernel/kexec.c: In function 'kernel_kexec':kernel/kexec.c:1506: warning: value computed is not used
---
kernel/kexec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/kexec.c renamed from kernel/kexec.c~kexec-jump-fix-compiling-warning-on-xchgkexec_lock-0-in-kernel_kexec +2 -1
@@ -1503,7 +1503,8 @@ int kernel_kexec(void) } Unlock: -xchg (&kexec_lock,0 ); +if (!xchg (&kexec_lock,0 )) +BUG ();return error; }
_
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Quoted 2 lines · Andrew Morton
We don't need to create that local. I queued this:
No, please don't.
Just don't take this whole patch-series until it's cleaned up. There is absolutely no excuse for using xchg as a locking primitive. Nothing like this should be queued anywhere, it should be burned and the ashes should be scattered over the atlantic so that nobody will ever see them again.
F*ck me with a spoon, if you have to use xchg() to do a trylock, why the hell isn't the unlock sequence then
smp_mb(); var = 0;
instead? Not that that's really right either, but at least it avoids the _ridiculous_ crap. The real solution is probably to use a spinlock and trylock/unlock.
Linus
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Andrew Morton
maintainerQuoted 9 lines · On Wed, 13 Aug 2008 10:01:13 -0700 (PDT) Linus Torvalds…
Quoted 2 lines · Andrew Morton
We don't need to create that local. I queued this:
No, please don't.
Just don't take this whole patch-series until it's cleaned up.
We already took it - in 2.6.13!
Quoted 8 lines
There is
absolutely no excuse for using xchg as a locking primitive. Nothing like this should be queued anywhere, it should be burned and the ashes should be scattered over the atlantic so that nobody will ever see them again.F*ck me with a spoon, if you have to use
xchg()to do a trylock, why the hell isn't the unlock sequence then
Quoted 7 lines
smp_mb(); var = 0;instead? Not that that's really right either, but at least it avoids the _ridiculous_ crap. The real solution is probably to use a spinlock and trylock/unlock.
Or test_and_set_bit(). That's what I've been saying too, only differently ;)
But cleaning up the long-standing silly usage of xchg() is a different activity from suppressing this recently-added compile warning.
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Quoted 9 lines · Andrew Morton
Quoted 3 lines
instead? Not that that's really right either, but at least it avoids the _ridiculous_ crap. The real solution is probably to use a spinlock and trylock/unlock.
Or
test_and_set_bit(). That's what I've been saying too, only differently ;)But cleaning up the long-standing silly usage of
xchg()is a different activity from suppressing this recently-added compile warning.
actually, in this case i disagree: the warning here is a canary that there's something wrong about this code - i.e. gcc is _right_ about warning us. The warning is also totally harmless - the warning shows us the suckiness of the code structure - and squashing the warning doesnt fix that.
So im coal-mine analogies, i disagree with squashing the canary, we should find and fix the shaft that emits the smelly methane instead ;-)
Ingo
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
kernel/kexec.c int kernel_kexec(void) · line 1503
Quoted 4 lines · Huang Ying
kernel/kexec.c +2
-1-xchg (&kexec_lock,0 ); + locked =xchg (&kexec_lock,0 ); +BUG_ON (!locked);
Why do you want to do this at all?
And why do you implement your locks with xchg() in the first place? That's total and utter crap.
Hint: we have _real_ locking primitives in the kernel.
Linus
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Quoted 8 lines · Linus Torvalds
kernel/kexec.c int kernel_kexec(void) · line 1503
Quoted 4 lines · Huang Ying
kernel/kexec.c +2
-1-xchg (&kexec_lock,0 ); + locked =xchg (&kexec_lock,0 ); +BUG_ON (!locked);Why do you want to do this at all?
Quoted 4 lines
And why do you implement your locks with
xchg()in the first place? That's total and utter crap.Hint: we have _real_ locking primitives in the kernel.
This part certainly.
The way the code should work, and the way it has in the past is:
image = xchg(&kexec_image, NULL)
if (!image)
return -EINVAL;
Very simple and very obvious and very easy to get right, and it has been that way for years.
Eric
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Quoted 17 lines · Eric W. Biederman
Quoted 8 lines · Linus Torvalds
kernel/kexec.c int kernel_kexec(void) · line 1503
Quoted 4 lines · Huang Ying
kernel/kexec.c +2
-1-xchg (&kexec_lock,0 ); + locked =xchg (&kexec_lock,0 ); +BUG_ON (!locked);Why do you want to do this at all?
Quoted 4 lines
And why do you implement your locks with
xchg()in the first place? That's total and utter crap.Hint: we have _real_ locking primitives in the kernel.
This part certainly.
Quoted 8 lines
The way the code should work, and the way it has in the past is:
image =xchg(&kexec_image, NULL)
if (!image)
return-EINVAL;Very simple and very obvious and very easy to get right, and it has been that way for years.
Hi Eric,
Are there any issues with usage of test_and_set_bit() or usage of spinlock primitives?
Thanks
Vivek
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Andrew Morton
maintainerQuoted 17 lines · On Wed, 13 Aug 2008 11:12:48 -0700 [email protected] (Eric W. Biederman)…
Quoted 8 lines · Linus Torvalds
Review
Quoted 4 lines · Huang Ying
(quoted diff) +2
-1- xchg(&kexec_lock, 0); + locked = xchg(&kexec_lock, 0); + BUG_ON(!locked);Why do you want to do this at all?
Quoted 4 lines
And why do you implement your locks with
xchg()in the first place? That's total and utter crap.Hint: we have _real_ locking primitives in the kernel.
This part certainly.
Quoted 8 lines
The way the code should work, and the way it has in the past is:
image =xchg(&kexec_image, NULL)
if (!image)
return-EINVAL;Very simple and very obvious and very easy to get right, and it has been that way for years.
- We're talking about kexec_lock here, not kexec_image
- afacit all manipulations of kexec_image happen under kexec_lock, so they don't need to be atomic, do they?
- Is xchg() guaranteed to be atomic? That's what atomic_xchg() is for.
- xchg() isn't guaranteed to exist on all architectures. atomic_xchg() is.
Could someone please review and test this? It's on top of
kexec-jump-clean-up-ifdef-and-comments.patch kexec-jump-rename-kexec_control_code_size-to-kexec_control_page_size.patch kexec-jump-check-code-size-in-control-page.patch kexec-jump-check-code-size-in-control-page-fix.patch kexec-jump-remove-duplication-of-kexec_restart_prepare.patch kexec-jump-in-sync-with-hibernation-implementation.patch kexec-jump-__ftrace_enabled_save-restore.patch kexec-jump-fix-for-ftrace.patch
Subject: kexec: use a bitop for locking rather than xchg()
From: Andrew Morton <[email protected]>
Functionally the same, but more conventional.
---
kernel/kexec.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-)
kernel/kexec.c renamed from kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg +21 -20
@@ -9,6 +9,7 @@#include <linux/capability.h> #include <linux/mm.h> #include <linux/file.h> +#include <linux/bitops.h> #include <linux/slab.h> #include <linux/fs.h> #include <linux/kexec.h>
@@ -924,19 +925,28 @@ static int kimage_load_segment(struct ki*/ struct kimage *kexec_image;struct kimage *kexec_crash_image; + +static unsigned long kexec_bitlock; +/* -* A home grown binary mutex. -* Nothing can wait so this mutex is safe to use -* in interrupt context :) +* Return true if we acquired the lock */ -static int kexec_lock; +static inline bool kexec_trylock (void ) +{ +return !test_and_set_bit (0 , &kexec_bitlock); +} + +static void kexec_unlock (void ) +{ +if (!test_and_clear_bit (0 , &kexec_bitlock)) +WARN_ON (1 ); +}asmlinkage long sys_kexec_load (unsigned long entry,unsigned long nr_segments,struct kexec_segment __user *segments,unsigned long flags) {struct kimage **dest_image, *image; -int locked;int result;/* We only trust the superuser with rebooting the system. */
@@ -972,8 +982,7 @@ asmlinkage long sys_kexec_load(unsigned* * KISS: always take the mutex. */ - locked =xchg (&kexec_lock,1 ); -if (locked) +if (!kexec_trylock ())return -EBUSY ; dest_image = &kexec_image;
@@ -1015,8 +1024,7 @@ asmlinkage long sys_kexec_load(unsigned image =xchg (dest_image, image); out: - locked =xchg (&kexec_lock,0 );/* Release the mutex */ -BUG_ON (!locked); +kexec_unlock ();kimage_free (image);return result;
@@ -1063,9 +1071,6 @@ asmlinkage long compat_sys_kexec_load(unvoid crash_kexec (struct pt_regs *regs) { -int locked; - -/* Take the kexec_lock here to prevent sys_kexec_load * running on one cpu from replacing the crash kernel * we are using after a panic on a different cpu.
@@ -1074,8 +1079,7 @@ void crash_kexec(struct pt_regs *regs)* of memory the xchg(&kexec_crash_image) would be * sufficient. But since I reuse the memory... */ - locked =xchg (&kexec_lock,1 ); -if (!locked) { +if (kexec_trylock ()) {if (kexec_crash_image) {struct pt_regs fixed_regs;crash_setup_regs (&fixed_regs, regs);
@@ -1083,8 +1087,7 @@ void crash_kexec(struct pt_regs *regs)machine_crash_shutdown (&fixed_regs);machine_kexec (kexec_crash_image); } - locked =xchg (&kexec_lock,0 ); -BUG_ON (!locked); +kexec_unlock (); } }
@@ -1434,7 +1437,7 @@ int kernel_kexec(void) {int error =0 ; -if (xchg (&kexec_lock,1 )) +if (!kexec_trylock ())return -EBUSY ;if (!kexec_image) { error = -EINVAL ;
@@ -1498,8 +1501,6 @@ int kernel_kexec(void)#endif Unlock: -if (!xchg (&kexec_lock,0 )) -BUG (); - +kexec_unlock ();return error; }
_
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
kernel/kexec.c static int kimage_load_segment(struct ki · line 936
Quoted 7 lines · Andrew Morton
- * in interrupt context :)
+ * Return true if we acquired the lock
*/kernel/kexec.c renamed from kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg +3
-1-static int kexec_lock; +static inline bool kexec_trylock (void ) +{ +return !test_and_set_bit (0 , &kexec_bitlock);
Nope. That needs to be an "unsigned long".
But more importantl, why not just make it a lock in the first place?
static DEFINE_SPINLOCK(kexec_lock); #define kexec_trylock() spin_trylock(&kexec_lock) #define kexec_unlock() spin_unlock(&kexec_lock)
and then you get it all right and clear and obvious.
Yeah, and I didn't check whether there is anything that is supposed to be able to sleep. If there is, use a mutex instead of a spinlock, of course.
Linus
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Andrew Morton
maintainerQuoted 12 lines · On Wed, 13 Aug 2008 12:50:57 -0700 (PDT) Linus Torvalds…
kernel/kexec.c static int kimage_load_segment(struct ki · line 936
Quoted 7 lines · Andrew Morton
- * in interrupt context :)
+ * Return true if we acquired the lock
*/kernel/kexec.c renamed from kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg +3
-1-static int kexec_lock; +static inline bool kexec_trylock (void ) +{ +return !test_and_set_bit (0 , &kexec_bitlock);Nope. That needs to be an "unsigned long".
It is.
Quoted 8 lines
But more importantl, why not just make it a lock in the first place?
static DEFINE_SPINLOCK(kexec_lock); #define kexec_trylock() spin_trylock(&kexec_lock) #define kexec_unlock() spin_unlock(&kexec_lock)and then you get it all right and clear and obvious.
Used a bitop to preserve the runtime checking in there. spin_unlock() doesn't return the previous lockedness.
Presumably lockdep will whine about spun_unlock(unlocked_lock) though.
Quoted 2 lines
Yeah, and I didn't check whether there is anything that is supposed to be able to sleep. If there is, use a mutex instead of a spinlock, of course.
Yes, it does sleepy things inside the lock.
A bitop seems a better fit to me. We never spin on that lock (it always uses test_and_set), so why use a "spin"lock?
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Quoted 4 lines · Andrew Morton
Quoted 2 lines
Nope. That needs to be an "unsigned long".
It is.
Gaah. I just misread the patch, sorry.
Quoted 2 lines
Used a bitop to preserve the runtime checking in there.
spin_unlock()doesn't return the previous lockedness.
Umm. spin_unlock does a lot more when you have lock debugging on, and doesn't do useless crap when it isn't.
Quoted 2 lines
A bitop seems a better fit to me. We never spin on that lock (it always uses
test_and_set), so why use a "spin"lock?
..because an atomic bitop is not the same as a lock.
The memory ordering guarantees are different. Yes, they are sufficient, but that's because we've had to make them so to account for CRAP CODE that uses bit operations as if they were locks.
Don't continue that. It's WRONG.
Linus
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Andrew Morton
maintainerQuoted 9 lines · On Wed, 13 Aug 2008 13:13:13 -0700 (PDT) Linus Torvalds…
Quoted 2 lines
Used a bitop to preserve the runtime checking in there.
spin_unlock()doesn't return the previous lockedness.Umm.
spin_unlockdoes a lot more when you have lock debugging on, and doesn't do useless crap when it isn't.Quoted 2 lines
A bitop seems a better fit to me. We never spin on that lock (it always uses
test_and_set), so why use a "spin"lock?
Quoted 7 lines
..because an atomic bitop is not the same as a lock.
The memory ordering guarantees are different. Yes, they are sufficient, but that's because we've had to make them so to account for CRAP CODE that uses bit operations as if they were locks.
Don't continue that. It's WRONG.
#2:
(The xchg(kexec_crash_image) stuff is still in there)
kernel/kexec.c renamed from kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg +8 -22
@@ -924,19 +924,14 @@ static int kimage_load_segment(struct ki*/ struct kimage *kexec_image;struct kimage *kexec_crash_image; -/* -* A home grown binary mutex. -* Nothing can wait so this mutex is safe to use -* in interrupt context :) -*/ -static int kexec_lock; + +static DEFINE_SPINLOCK (kexec_lock);asmlinkage long sys_kexec_load (unsigned long entry,unsigned long nr_segments,struct kexec_segment __user *segments,unsigned long flags) {struct kimage **dest_image, *image; -int locked;int result;/* We only trust the superuser with rebooting the system. */
@@ -972,8 +967,7 @@ asmlinkage long sys_kexec_load(unsigned* * KISS: always take the mutex. */ - locked =xchg (&kexec_lock,1 ); -if (locked) +if (!spin_trylock (&kexec_lock))return -EBUSY ; dest_image = &kexec_image;
@@ -1015,8 +1009,7 @@ asmlinkage long sys_kexec_load(unsigned image =xchg (dest_image, image); out: - locked =xchg (&kexec_lock,0 );/* Release the mutex */ -BUG_ON (!locked); +spin_unlock (&kexec_lock);kimage_free (image);return result;
@@ -1063,9 +1056,6 @@ asmlinkage long compat_sys_kexec_load(unvoid crash_kexec (struct pt_regs *regs) { -int locked; - -/* Take the kexec_lock here to prevent sys_kexec_load * running on one cpu from replacing the crash kernel * we are using after a panic on a different cpu.
@@ -1074,8 +1064,7 @@ void crash_kexec(struct pt_regs *regs)* of memory the xchg(&kexec_crash_image) would be * sufficient. But since I reuse the memory... */ - locked =xchg (&kexec_lock,1 ); -if (!locked) { +if (spin_trylock (&kexec_lock)) {if (kexec_crash_image) {struct pt_regs fixed_regs;crash_setup_regs (&fixed_regs, regs);
@@ -1083,8 +1072,7 @@ void crash_kexec(struct pt_regs *regs)machine_crash_shutdown (&fixed_regs);machine_kexec (kexec_crash_image); } - locked =xchg (&kexec_lock,0 ); -BUG_ON (!locked); +spin_unlock (&kexec_lock); } }
@@ -1434,7 +1422,7 @@ int kernel_kexec(void) {int error =0 ; -if (xchg (&kexec_lock,1 )) +if (!spin_trylock (&kexec_lock))return -EBUSY ;if (!kexec_image) { error = -EINVAL ;
@@ -1498,8 +1486,6 @@ int kernel_kexec(void)#endif Unlock: -if (!xchg (&kexec_lock,0 )) -BUG (); - +spin_unlock (&kexec_lock);return error; }
_
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Quoted 2 lines · Andrew Morton
#2:
I thought you said there were things that want to sleep in the region?
If so, spinlocks will work as long as you don't have CONFIG_PREEMPT or lock validation (there's no way to deadlock thanks to all the lock getters using the "trylock" variant), but will blow up because a successful trylock will obviously also disable preemption and/or trigger all the lock detection.
So if there are potential sleepers, you'd need the mutex instead.
Linus
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Andrew Morton
maintainerQuoted 5 lines · On Wed, 13 Aug 2008 13:31:24 -0700 (PDT) Linus Torvalds…
Quoted 2 lines · Andrew Morton
#2:
I thought you said there were things that want to sleep in the region?
be reasonable - that was over five minutes ago.
kernel/kexec.c renamed from kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg +10 -24
@@ -12,7 +12,7 @@#include <linux/slab.h> #include <linux/fs.h> #include <linux/kexec.h> -#include <linux/spinlock.h> +#include <linux/mutex.h> #include <linux/list.h> #include <linux/highmem.h> #include <linux/syscalls.h>
@@ -924,19 +924,14 @@ static int kimage_load_segment(struct ki*/ struct kimage *kexec_image;struct kimage *kexec_crash_image; -/* -* A home grown binary mutex. -* Nothing can wait so this mutex is safe to use -* in interrupt context :) -*/ -static int kexec_lock; + +static DEFINE_MUTEX (kexec_mutex);asmlinkage long sys_kexec_load (unsigned long entry,unsigned long nr_segments,struct kexec_segment __user *segments,unsigned long flags) {struct kimage **dest_image, *image; -int locked;int result;/* We only trust the superuser with rebooting the system. */
@@ -972,8 +967,7 @@ asmlinkage long sys_kexec_load(unsigned* * KISS: always take the mutex. */ - locked =xchg (&kexec_lock,1 ); -if (locked) +if (!mutex_trylock (&kexec_mutex))return -EBUSY ; dest_image = &kexec_image;
@@ -1015,8 +1009,7 @@ asmlinkage long sys_kexec_load(unsigned image =xchg (dest_image, image); out: - locked =xchg (&kexec_lock,0 );/* Release the mutex */ -BUG_ON (!locked); +mutex_unlock (&kexec_mutex);kimage_free (image);return result;
@@ -1063,10 +1056,7 @@ asmlinkage long compat_sys_kexec_load(unvoid crash_kexec (struct pt_regs *regs) { -int locked; - - -/* Take the kexec_lock here to prevent sys_kexec_load +/* Take the kexec_mutex here to prevent sys_kexec_load * running on one cpu from replacing the crash kernel * we are using after a panic on a different cpu. *
@@ -1074,8 +1064,7 @@ void crash_kexec(struct pt_regs *regs)* of memory the xchg(&kexec_crash_image) would be * sufficient. But since I reuse the memory... */ - locked =xchg (&kexec_lock,1 ); -if (!locked) { +if (mutex_trylock (&kexec_mutex)) {if (kexec_crash_image) {struct pt_regs fixed_regs;crash_setup_regs (&fixed_regs, regs);
@@ -1083,8 +1072,7 @@ void crash_kexec(struct pt_regs *regs)machine_crash_shutdown (&fixed_regs);machine_kexec (kexec_crash_image); } - locked =xchg (&kexec_lock,0 ); -BUG_ON (!locked); +mutex_unlock (&kexec_mutex); } }
@@ -1434,7 +1422,7 @@ int kernel_kexec(void) {int error =0 ; -if (xchg (&kexec_lock,1 )) +if (!mutex_trylock (&kexec_mutex))return -EBUSY ;if (!kexec_image) { error = -EINVAL ;
@@ -1498,8 +1486,6 @@ int kernel_kexec(void)#endif Unlock: -if (!xchg (&kexec_lock,0 )) -BUG (); - +mutex_unlock (&kexec_mutex);return error; }
_
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Quoted 106 lines · Andrew Morton
Quoted 5 lines · On Wed, 13 Aug 2008 13:31:24 -0700 (PDT) Linus Torvalds…
Quoted 2 lines · Andrew Morton
#2:
I thought you said there were things that want to sleep in the region?
be reasonable - that was over five minutes ago.
kernel/kexec.c renamed from kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg +9
-21@@ -12,7 +12,7 @@#include <linux/slab.h> #include <linux/fs.h> #include <linux/kexec.h> -#include <linux/spinlock.h> +#include <linux/mutex.h> #include <linux/list.h> #include <linux/highmem.h> #include <linux/syscalls.h> @@ -924,19 +924,14 @@ static int kimage_load_segment(struct ki*/ struct kimage *kexec_image;struct kimage *kexec_crash_image; -/* -* A home grown binary mutex. -* Nothing can wait so this mutex is safe to use -* in interrupt context :) -*/ -static int kexec_lock; + +static DEFINE_MUTEX (kexec_mutex);asmlinkage long sys_kexec_load (unsigned long entry,unsigned long nr_segments,struct kexec_segment __user *segments,unsigned long flags) {struct kimage **dest_image, *image; -int locked;int result;/* We only trust the superuser with rebooting the system. */ @@ -972,8 +967,7 @@ asmlinkage long sys_kexec_load(unsigned* * KISS: always take the mutex. */ - locked =xchg (&kexec_lock,1 ); -if (locked) +if (!mutex_trylock (&kexec_mutex))return -EBUSY ; dest_image = &kexec_image;@@ -1015,8 +1009,7 @@ asmlinkage long sys_kexec_load(unsigned image =xchg (dest_image, image); out: - locked =xchg (&kexec_lock,0 );/* Release the mutex */ -BUG_ON (!locked); +mutex_unlock (&kexec_mutex);kimage_free (image);return result;@@ -1063,10 +1056,7 @@ asmlinkage long compat_sys_kexec_load(unvoid crash_kexec (struct pt_regs *regs) { -int locked; - - -/* Take the kexec_lock here to prevent sys_kexec_load +/* Take the kexec_mutex here to prevent sys_kexec_load * running on one cpu from replacing the crash kernel * we are using after a panic on a different cpu. * @@ -1074,8 +1064,7 @@ void crash_kexec(struct pt_regs *regs)* of memory the xchg(&kexec_crash_image) would be * sufficient. But since I reuse the memory... */ - locked =xchg (&kexec_lock,1 ); -if (!locked) { +if (mutex_trylock (&kexec_mutex)) {if (kexec_crash_image) {struct pt_regs fixed_regs;crash_setup_regs (&fixed_regs, regs);@@ -1083,8 +1072,7 @@ void crash_kexec(struct pt_regs *regs)machine_crash_shutdown (&fixed_regs);machine_kexec (kexec_crash_image); } - locked =xchg (&kexec_lock,0 ); -BUG_ON (!locked); +mutex_unlock (&kexec_mutex); } }@@ -1434,7 +1422,7 @@ int kernel_kexec(void) {int error =0 ; -if (xchg (&kexec_lock,1 )) +if (!mutex_trylock (&kexec_mutex))return -EBUSY ;if (!kexec_image) { error = -EINVAL ;
kernel/kexec.c int kernel_kexec(void) · line 1491
Quoted 9 lines
kernel/kexec.c renamed from kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg +1
-3@@ -1498,8 +1486,6 @@ int kernel_kexec(void)#endif Unlock: -if (!xchg (&kexec_lock,0 )) -BUG (); - +mutex_unlock (&kexec_mutex);return error; }
I tested kexec and kudmp on 2.6.27-rc3 with this patch. Works for me on a 32 bit machine.
Thanks
Vivek
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Quoted 6 lines · Andrew Morton
Quoted 2 lines · On Wed, 13 Aug 2008 13:31:24 -0700 (PDT) Linus Torvalds…
I thought you said there were things that want to sleep in the region?
be reasonable - that was over five minutes ago.
LOL. Anyway, I can't find anything wrong with this last one, however difficult I try to be.
Linus
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Quoted 1 line · Andrew Morton
- Is
xchg()guaranteed to be atomic? That's whatatomic_xchg()is for.
Yes, xchg() is guaranteed to be atomic. atomic_xchg() applies only to the atomic_t type, and is almost always #defined to xchg().
Quoted 1 line
-
xchg()isn't guaranteed to exist on all architectures.atomic_xchg()is.
You appear to be confusing xchg() with cmpxchg(). AFAIK, xchg() exists on all architectures, and is used in several instances of generic code. It is particularly extensively used in the networking layer.
Trond
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec