RE: [BUG] Variable stopmachine_state should be volatile

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

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

[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

[Zhang, Yanmin]

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

>>-----Original Message-----
>>From: linux-ia64-owner@vger.kernel.org
>>[mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Zhang, Yanmin
>>Sent: 2005年12月12日 15:07
>>To: Luck, Tony; 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
>>
>>>>-----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.
Tony,

Here is the patch against 2.6.15-rc5.

The latest intel compiler on ia64 also adds some other intrinsic, but I am not sure if they're useful in kernel except __hint, so I just enabled ia64_hint.

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


[-- Attachment #2: ia64_hint_2.6.15-rc5.patch --]
[-- Type: application/octet-stream, Size: 1074 bytes --]

diff -Nraup linux-2.6.15-rc5/include/asm-ia64/intel_intrin.h linux-2.6.15-rc5_fix/include/asm-ia64/intel_intrin.h
--- linux-2.6.15-rc5/include/asm-ia64/intel_intrin.h	2005-08-29 07:41:01.000000000 +0800
+++ linux-2.6.15-rc5_fix/include/asm-ia64/intel_intrin.h	2005-12-14 00:56:10.000000000 +0800
@@ -113,6 +113,13 @@ __s64 _m64_dep_mi(const int v, __s64 s, 
 __s64 _m64_shrp(__s64 a, __s64 b, const int count);
 __s64 _m64_popcnt(__s64 a);
 
+/* value for arguments to the hint instruction */
+#define ia64_hint_pause	0
+
+/* generates the itanium hint instruction */
+void __hint(const int);
+
+
 #define ia64_barrier()		__memory_barrier()
 
 #define ia64_stop()	/* Nothing: As of now stop bit is generated for each
@@ -122,7 +129,14 @@ __s64 _m64_popcnt(__s64 a);
 #define ia64_getreg		__getReg
 #define ia64_setreg		__setReg
 
-#define ia64_hint(x)
+#define ia64_hint(mode)						\
+({								\
+	switch (mode) {						\
+	case ia64_hint_pause:					\
+		__hint(ia64_hint_pause);			\
+		break;						\
+	}							\
+})
 
 #define ia64_mux1_brcst	 0
 #define ia64_mux1_mix		 8