[PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Huang Ying]

Fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec().

Signed-off-by: Huang Ying <ying.huang@intel.com>

---
 kernel/kexec.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -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;
 }

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Andrew Morton]

On Wed, 13 Aug 2008 17:12:40 +0800 Huang Ying <ying.huang@intel.com> wrote:

> Fix compiling warning on xchg(&kexec_lock, 0) in kernel_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.

> 
> ---
>  kernel/kexec.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -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;
>  }
> 

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 <ying.huang@intel.com>

kernel/kexec.c: In function 'kernel_kexec':
kernel/kexec.c:1506: warning: value computed is not used

Signed-off-by: Huang Ying <ying.huang@intel.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/kexec.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -puN kernel/kexec.c~kexec-jump-fix-compiling-warning-on-xchgkexec_lock-0-in-kernel_kexec kernel/kexec.c
--- a/kernel/kexec.c~kexec-jump-fix-compiling-warning-on-xchgkexec_lock-0-in-kernel_kexec
+++ a/kernel/kexec.c
@@ -1503,7 +1503,8 @@ int kernel_kexec(void)
 	}
 
  Unlock:
-	xchg(&kexec_lock, 0);
+	if (!xchg(&kexec_lock, 0))
+		BUG();
 
 	return error;
 }
_

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Linus Torvalds]

On Wed, 13 Aug 2008, Andrew Morton wrote:
>
> 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

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Andrew Morton]

On Wed, 13 Aug 2008 10:01:13 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Wed, 13 Aug 2008, Andrew Morton wrote:
> >
> > 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!

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

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.

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

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

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Linus Torvalds]

On Wed, 13 Aug 2008, Huang Ying wrote:
>
> -	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

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Eric W. Biederman]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, 13 Aug 2008, Huang Ying wrote:
>>
>> -	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.

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

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Vivek Goyal]

On Wed, Aug 13, 2008 at 11:12:48AM -0700, Eric W. Biederman wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Wed, 13 Aug 2008, Huang Ying wrote:
> >>
> >> -	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.
> 
> 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.
> 

Hi Eric,

Are there any issues with usage of test_and_set_bit() or usage of spinlock
primitives?

Thanks
Vivek

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Andrew Morton]

On Wed, 13 Aug 2008 11:12:48 -0700
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Wed, 13 Aug 2008, Huang Ying wrote:
> >>
> >> -	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.
> 
> 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.
> 

- 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 <akpm@linux-foundation.org>

Functionally the same, but more conventional.

Cc: Huang Ying <ying.huang@intel.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/kexec.c |   41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff -puN kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg kernel/kexec.c
--- a/kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg
+++ a/kernel/kexec.c
@@ -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(un
 
 void 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;
 }
_

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Linus Torvalds]

On Wed, 13 Aug 2008, Andrew Morton wrote:
> - * 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);

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

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Andrew Morton]

On Wed, 13 Aug 2008 12:50:57 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Wed, 13 Aug 2008, Andrew Morton wrote:
> > - * 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);
> 
> Nope. That needs to be an "unsigned long".

It is.

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

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

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Linus Torvalds]

On Wed, 13 Aug 2008, Andrew Morton wrote:
> > 
> > Nope. That needs to be an "unsigned long".
> 
> It is.

Gaah. I just misread the patch, sorry.

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

> 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

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Andrew Morton]

On Wed, 13 Aug 2008 13:13:13 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

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

#2:

(The xchg(kexec_crash_image) stuff is still in there)

--- a/kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg
+++ a/kernel/kexec.c
@@ -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(un
 
 void 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;
 }
_

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Linus Torvalds]

On Wed, 13 Aug 2008, Andrew Morton wrote:
> 
> #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

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Andrew Morton]

On Wed, 13 Aug 2008 13:31:24 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 13 Aug 2008, Andrew Morton wrote:
> > 
> > #2:
> 
> I thought you said there were things that want to sleep in the region?

be reasonable - that was over five minutes ago.

--- a/kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg
+++ a/kernel/kexec.c
@@ -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(un
 
 void 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;
 }
_

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Vivek Goyal]

On Wed, Aug 13, 2008 at 01:41:18PM -0700, Andrew Morton wrote:
> On Wed, 13 Aug 2008 13:31:24 -0700 (PDT)
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Wed, 13 Aug 2008, Andrew Morton wrote:
> > > 
> > > #2:
> > 
> > I thought you said there were things that want to sleep in the region?
> 
> be reasonable - that was over five minutes ago.
> 
> --- a/kernel/kexec.c~kexec-use-a-bitop-for-locking-rather-than-xchg
> +++ a/kernel/kexec.c
> @@ -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(un
>  
>  void 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;
>  }

I tested kexec and kudmp on 2.6.27-rc3 with this patch. Works for me on a
32 bit machine.

Thanks
Vivek

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Linus Torvalds]

On Wed, 13 Aug 2008, Andrew Morton wrote:

> On Wed, 13 Aug 2008 13:31:24 -0700 (PDT)
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > 
> > 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

Re: [PATCH] kexec jump: fix compiling warning on xchg(&kexec_lock, 0) in kernel_kexec()

[Trond Myklebust]

On Wed, 2008-08-13 at 12:44 -0700, Andrew Morton wrote:

> - Is xchg() guaranteed to be atomic?  That's what atomic_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().

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