RE: [BUG] Variable stopmachine_state should be volatile

RE: [BUG] Variable stopmachine_state should be volatile

[Zhang, Yanmin]

>>-----Original Message-----
>>From: Pavel Machek [mailto:pavel@ucw.cz]
>>Sent: 2005年12月1日 6:20
>>To: Zhang, Yanmin
>>Cc: linux-kernel@vger.kernel.org; Pallipadi, Venkatesh; Shah, Rajesh
>>Subject: Re: [BUG] Variable stopmachine_state should be volatile
>>
>>Hi!
>>
>>> The model to access variable stopmachine_state is that a main thread
>>> writes it and other threads read it. Its declaration has no sign
>>> volatile. In the while loop in function stopmachine, this variable is
>>> read, and compiler might optimize it by reading it once before the loop
>>> and not reading it again in the loop, so the thread might enter dead
>>> loop.
>>
>>No. volatile may look like a solution, but it usually is not. You may
>>need some barriers, atomic_t or locking.
>>								Pavel
The original functions already use smp_mb/smp_wmb. My patch just tells compiler not to optimize by bringing the reading of stopmachine_state out of the while loop.

Re: [BUG] Variable stopmachine_state should be volatile

[Pavel Machek]

Hi!

> >>Hi!
> >>
> >>> The model to access variable stopmachine_state is that a main thread
> >>> writes it and other threads read it. Its declaration has no sign
> >>> volatile. In the while loop in function stopmachine, this variable is
> >>> read, and compiler might optimize it by reading it once before the loop
> >>> and not reading it again in the loop, so the thread might enter dead
> >>> loop.
> >>
> >>No. volatile may look like a solution, but it usually is not. You may
> >>need some barriers, atomic_t or locking.
> >>								Pavel
> The original functions already use smp_mb/smp_wmb. My patch just
>tells compiler not to optimize by bringing the reading of
>stopmachine_state out of the while loop.

Those barriers should already prevent compiler optimalization, no? If
they do not, just use some barriers that do.
								Pavel


-- 
Thanks, Sharp!

RE: [BUG] Variable stopmachine_state should be volatile

[Zhang, Yanmin]

>>-----Original Message-----
>>From: linux-ia64-owner@vger.kernel.org
>>[mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Luck, Tony
>>Sent: 2005年12月9日 2:53
>>To: Zhang, Yanmin; Arjan van de Ven; Pavel Machek
>>Cc: linux-kernel@vger.kernel.org; Pallipadi, Venkatesh; Shah, Rajesh;
>>linux-ia64@vger.kernel.org
>>Subject: RE: [BUG] Variable stopmachine_state should be volatile
>>
>>> The right approach is to define ia64_hint to ia64_barrier in file
>>> include/asm-ia64/intel_intrin.h. I tested the new approach and it
>>> does work.
>>
>>Does that get you a "hint@pause" instruction inside the loop?  If not, then
>>it isn't all the way to the "right" approach.

Tony,

The approach just fixes compiler scheduling barrier problem of cpu_relax, and there is no hint@pause inside the loop.

Today I tried the latest icc, 9.0.027. It provides __hint, a new intrinsic, to support hint@pause. __hint also has the meaning of compiler scheduling barrier. So the best solution is to use __hint(0). The disassembled result showed the new intrinsic did work.

RE: [BUG] Variable stopmachine_state should be volatile

[Luck, Tony]

> The right approach is to define ia64_hint to ia64_barrier in file
> include/asm-ia64/intel_intrin.h. I tested the new approach and it
> does work.

Does that get you a "hint@pause" instruction inside the loop?  If not, then
it isn't all the way to the "right" approach.

-Tony

RE: [BUG] Variable stopmachine_state should be volatile

[Zhang, Yanmin]

>>-----Original Message-----
>>From: Arjan van de Ven [mailto:arjan@infradead.org]
>>Sent: 2005年12月2日 16:04
>>To: Zhang, Yanmin
>>Cc: linux-kernel@vger.kernel.org; Pallipadi, Venkatesh; Shah, Rajesh
>>Subject: Re: [BUG] Variable stopmachine_state should be volatile
>>
>>On Wed, 2005-11-30 at 10:04 +0800, Zhang, Yanmin wrote:
>>> The model to access variable stopmachine_state is that a main thread
>>> writes it and other threads read it. Its declaration has no sign
>>> volatile. In the while loop in function stopmachine, this variable is
>>> read, and compiler might optimize it by reading it once before the loop
>>> and not reading it again in the loop, so the thread might enter dead
>>> loop.
>>
>>cpu_relax() includes a compiler barier..... so... what's wrong with the
>>compiler that it ignores such barriers?

You are right. I hit the problem when I compiled kernel 2.6.9 on IA64 by intel compiler.
cpu_relax has the compiler barrier if we use gcc, but cpu_relax becomes just ia64_hint which is null when I use intel compiler to compile kernel on ia64. file include/asm-ia64/intel_intrin.h defines ia64_hint as null.

Function stopmachine in kernel/stop_machine.c uses cpu_relax to prevent compiler from moving the reading of stopmachine_state out of the while loop. But when we use intel compiler, cpu_relax doesn't work because it is just null.

The right approach is to define ia64_hint to ia64_barrier in file include/asm-ia64/intel_intrin.h. I tested the new approach and it does work.

Thank Arjan, Pavel, and Venki.

Re: [BUG] Variable stopmachine_state should be volatile

[Christoph Hellwig]

> You are right. I hit the problem when I compiled kernel 2.6.9 on IA64 by intel compiler.
> cpu_relax has the compiler barrier if we use gcc, but cpu_relax becomes just ia64_hint which is null when I use intel compiler to compile kernel on ia64. file include/asm-ia64/intel_intrin.h defines ia64_hint as null.

Please report all bugs using ICC to intel.  We can't and don't want to debug
this mess.

Re: [BUG] Variable stopmachine_state should be volatile

[Pavel Machek]

Hi!

> >Thanks. The functions are not performance sensitive, so 
> >atomic_t is ok. Here is the new patch.
> >
> 
> The only reason atomic_t will help in this case is because it uses volatile for internal counter, as it does nothing additional on atomic_read(). I don't get what is the issue with using volatile directly. May be I am missing something. Pavel can you elaborate please.
> 

Look at atomic_t again. It is definitely not "just volatile",
definitely not on non-i386 architectures.

> The code here is doing something like this
> 
> While (variable != specific_value)
> 	cpu_relax();

So either do 

while (variable != value)
	mb();

or better use atomic_t.

> So, making variable atomic or declaring variable as volatile should have the same impact here. Note there is only one CPU setting this variable and rest of the CPUs just read it.
> 
> If volatile is not good, probably we need to find something other than atomic as well.
> 

atomic_t works, is used across the kernel, and works on all
architectures. volatile int does not. Do not use it.

							Pavel
-- 
Thanks, Sharp!

RE: [BUG] Variable stopmachine_state should be volatile

[Zhang, Yanmin]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

>>-----Original Message-----
>>From: Pavel Machek [mailto:pavel@ucw.cz]
>>Sent: 2005年12月1日 18:26
>>To: Zhang, Yanmin
>>Cc: linux-kernel@vger.kernel.org; Pallipadi, Venkatesh; Shah, Rajesh
>>Subject: Re: [BUG] Variable stopmachine_state should be volatile
>>
>>Hi!
>>
>>> >>Those barriers should already prevent compiler optimalization, no? If
>>> >>they do not, just use some barriers that do.
>>>
>>>
>>> I hit the problem when compiling 2.6 kernel by intel compiler.
>>> How about to change the type of stopmachine_state to atomic_t?
>>
>>That is certainly safest / very conservative solution.
>>								Pavel
Thanks. The functions are not performance sensitive, so atomic_t is ok. Here is the new patch.


[-- Attachment #2: stopmachine_state_volatile_2.6.15_rc3_v2.patch --]
[-- Type: application/octet-stream, Size: 2708 bytes --]

--- linux-2.6.15-rc3/kernel/stop_machine.c	2005-11-29 00:02:44.000000000 +0800
+++ linux-2.6.15-rc3_fix/kernel/stop_machine.c	2005-11-30 18:41:55.000000000 +0800
@@ -13,14 +13,12 @@
  * kthread. */
 
 /* Thread to stop each CPU in user context. */
-enum stopmachine_state {
-	STOPMACHINE_WAIT,
-	STOPMACHINE_PREPARE,
-	STOPMACHINE_DISABLE_IRQ,
-	STOPMACHINE_EXIT,
-};
+#define	STOPMACHINE_WAIT		0
+#define	STOPMACHINE_PREPARE		1
+#define	STOPMACHINE_DISABLE_IRQ		2
+#define	STOPMACHINE_EXIT		3
 
-static enum stopmachine_state stopmachine_state;
+static atomic_t stopmachine_state;
 static unsigned int stopmachine_num_threads;
 static atomic_t stopmachine_thread_ack;
 static DECLARE_MUTEX(stopmachine_mutex);
@@ -33,24 +31,21 @@ static int stopmachine(void *cpu)
 	set_cpus_allowed(current, cpumask_of_cpu((int)(long)cpu));
 
 	/* Ack: we are alive */
-	smp_mb(); /* Theoretically the ack = 0 might not be on this CPU yet. */
 	atomic_inc(&stopmachine_thread_ack);
 
 	/* Simple state machine */
-	while (stopmachine_state != STOPMACHINE_EXIT) {
-		if (stopmachine_state == STOPMACHINE_DISABLE_IRQ 
+	while (atomic_read(&stopmachine_state) != STOPMACHINE_EXIT) {
+		if (atomic_read(&stopmachine_state) == STOPMACHINE_DISABLE_IRQ 
 		    && !irqs_disabled) {
 			local_irq_disable();
 			irqs_disabled = 1;
 			/* Ack: irqs disabled. */
-			smp_mb(); /* Must read state first. */
 			atomic_inc(&stopmachine_thread_ack);
-		} else if (stopmachine_state == STOPMACHINE_PREPARE
-			   && !prepared) {
+		} else if (atomic_read(&stopmachine_state)
+				== STOPMACHINE_PREPARE && !prepared) {
 			/* Everyone is in place, hold CPU. */
 			preempt_disable();
 			prepared = 1;
-			smp_mb(); /* Must read state first. */
 			atomic_inc(&stopmachine_thread_ack);
 		}
 		/* Yield in first stage: migration threads need to
@@ -62,7 +57,6 @@ static int stopmachine(void *cpu)
 	}
 
 	/* Ack: we are exiting. */
-	smp_mb(); /* Must read state first. */
 	atomic_inc(&stopmachine_thread_ack);
 
 	if (irqs_disabled)
@@ -74,11 +68,10 @@ static int stopmachine(void *cpu)
 }
 
 /* Change the thread state */
-static void stopmachine_set_state(enum stopmachine_state state)
+static void stopmachine_set_state(int state)
 {
 	atomic_set(&stopmachine_thread_ack, 0);
-	smp_wmb();
-	stopmachine_state = state;
+	atomic_set(&stopmachine_state, state);
 	while (atomic_read(&stopmachine_thread_ack) != stopmachine_num_threads)
 		cpu_relax();
 }
@@ -97,7 +90,7 @@ static int stop_machine(void)
 
 	atomic_set(&stopmachine_thread_ack, 0);
 	stopmachine_num_threads = 0;
-	stopmachine_state = STOPMACHINE_WAIT;
+	atomic_set(&stopmachine_state, STOPMACHINE_WAIT);
 
 	for_each_online_cpu(i) {
 		if (i == raw_smp_processor_id())

RE: [BUG] Variable stopmachine_state should be volatile

[Zhang, Yanmin]

>>-----Original Message-----
>>From: Pavel Machek [mailto:pavel@ucw.cz]
>>Sent: 2005年12月1日 11:26
>>To: Zhang, Yanmin
>>Cc: linux-kernel@vger.kernel.org; Pallipadi, Venkatesh; Shah, Rajesh
>>Subject: Re: [BUG] Variable stopmachine_state should be volatile
>>
>>Hi!
>>
>>> >>Hi!
>>> >>
>>> >>> The model to access variable stopmachine_state is that a main thread
>>> >>> writes it and other threads read it. Its declaration has no sign
>>> >>> volatile. In the while loop in function stopmachine, this variable is
>>> >>> read, and compiler might optimize it by reading it once before the loop
>>> >>> and not reading it again in the loop, so the thread might enter dead
>>> >>> loop.
>>> >>
>>> >>No. volatile may look like a solution, but it usually is not. You may
>>> >>need some barriers, atomic_t or locking.
>>> >>								Pavel
>>> The original functions already use smp_mb/smp_wmb. My patch just
>>>tells compiler not to optimize by bringing the reading of
>>>stopmachine_state out of the while loop.
>>
>>Those barriers should already prevent compiler optimalization, no? If
>>they do not, just use some barriers that do.


I hit the problem when compiling 2.6 kernel by intel compiler.
How about to change the type of stopmachine_state to atomic_t?

Re: [BUG] Variable stopmachine_state should be volatile

[Pavel Machek]

Hi!

> >>Those barriers should already prevent compiler optimalization, no? If
> >>they do not, just use some barriers that do.
> 
> 
> I hit the problem when compiling 2.6 kernel by intel compiler.
> How about to change the type of stopmachine_state to atomic_t?

That is certainly safest / very conservative solution.
								Pavel
-- 
Thanks, Sharp!

[BUG] Variable stopmachine_state should be volatile

[Zhang, Yanmin]

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]

The model to access variable stopmachine_state is that a main thread
writes it and other threads read it. Its declaration has no sign
volatile. In the while loop in function stopmachine, this variable is
read, and compiler might optimize it by reading it once before the loop
and not reading it again in the loop, so the thread might enter dead
loop.

Here is the patch to fix it.

Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>



[-- Attachment #2: stopmachine_state_volatile_2.6.15_rc3.patch --]
[-- Type: application/octet-stream, Size: 563 bytes --]

diff -Nraup linux-2.6.15-rc3/kernel/stop_machine.c linux-2.6.15-rc3_fix/kernel/stop_machine.c
--- linux-2.6.15-rc3/kernel/stop_machine.c	2005-11-29 00:02:44.000000000 +0800
+++ linux-2.6.15-rc3_fix/kernel/stop_machine.c	2005-11-29 00:03:47.000000000 +0800
@@ -20,7 +20,7 @@ enum stopmachine_state {
 	STOPMACHINE_EXIT,
 };
 
-static enum stopmachine_state stopmachine_state;
+static volatile enum stopmachine_state stopmachine_state;
 static unsigned int stopmachine_num_threads;
 static atomic_t stopmachine_thread_ack;
 static DECLARE_MUTEX(stopmachine_mutex);

Re: [BUG] Variable stopmachine_state should be volatile

[Pavel Machek]

Hi!

> The model to access variable stopmachine_state is that a main thread
> writes it and other threads read it. Its declaration has no sign
> volatile. In the while loop in function stopmachine, this variable is
> read, and compiler might optimize it by reading it once before the loop
> and not reading it again in the loop, so the thread might enter dead
> loop.

No. volatile may look like a solution, but it usually is not. You may
need some barriers, atomic_t or locking.
								Pavel
-- 
Thanks, Sharp!

Re: [BUG] Variable stopmachine_state should be volatile

[Arjan van de Ven]

On Wed, 2005-11-30 at 10:04 +0800, Zhang, Yanmin wrote:
> The model to access variable stopmachine_state is that a main thread
> writes it and other threads read it. Its declaration has no sign
> volatile. In the while loop in function stopmachine, this variable is
> read, and compiler might optimize it by reading it once before the loop
> and not reading it again in the loop, so the thread might enter dead
> loop.

cpu_relax() includes a compiler barier..... so... what's wrong with the
compiler that it ignores such barriers?