Cr4 is valid on some 486s

So some 486 processors do have CR4 register. Allow them to present it in register dumps by using the old fault technique rather than testing processor family.

Thanks to Maciej for noticing this.

Index: linux-2.6.14/arch/i386/kernel/process.c
===================================================================
@@ -314,9 +314,7 @@
 	cr0 = read_cr0();
 	cr2 = read_cr2();
 	cr3 = read_cr3();
-	if (current_cpu_data.x86 > 4) {
-		cr4 = read_cr4();
-	}
+	cr4 = read_cr4_safe();
 	printk("CR0: %08lx CR2: %08lx CR3: %08lx CR4: %08lx\n", cr0, cr2, cr3, cr4);
 	show_trace(NULL, &regs->esp);
 }

Index: linux-2.6.14/include/asm-i386/system.h
===================================================================
@@ -140,6 +140,19 @@
 		:"=r" (__dummy)); \
 	__dummy; \
 })
+
+#define read_cr4_safe() ({			      \
+	unsigned int __dummy;			      \
+	/* This could fault if %cr4 does not exist */ \
+	__asm__("1: movl %%cr4, %0		\n"   \
+		"2:				\n"   \
+		".section __ex_table,\"a\"	\n"   \
+		".long 1b,2b			\n"   \
+		".previous			\n"   \
+		: "=r" (__dummy): "0" (0));	      \
+	__dummy;				      \
+})
+
 #define write_cr4(x) \
 	__asm__ __volatile__("movl %0,%%cr4": :"r" (x));
 #define stts() write_cr0(8 | read_cr0())

Hi!

I thought Andi commented this as "way too risky", for little good. Nested exceptions are evil.
Pavel

Thanks, Sharp!

I think the 486's that have CR4 are the same that have CPUID, and thus can be tested for by the presence of the ID flag.

-hpa

That's correct; for our purposes a 486 that would implement CR4 but not
CPUID would not be interesting anyway, as we don't use CR4 elsewhere but for features discovered through CPUID. And I don't think there's ever been an implementation that had CPUID but no CR4.

Maciej

I didn't see Andi's comment to that effect. I may have originally argued that when I made CR4 reads depend on CPU family. But I think it is useful to know if PSE is enabled, especially on 486s that do support it.

Agree nested exceptions are evil. But where is this called from execption context?

1) softlockup_tick appears to be perfectly safe call site to handle exceptions
2) sysrq-p is also a fine site.

I tested this by assembling a hacked safe_read_cr1() macro, and dumped the contents of my non-existant CR1 regsiter in show_regs to prove the fault handling correct (although the code already _looks_ correct, I thought someone might ask the question you just did. :)

Zach

We have really nice ways of handling these things, so we should just use them.

For example, you can do

	static inline void read_cr4(void)
	{
		unsigned long cr4;
		alternative_input("xorl %0,%0",
				  "movl %%cr4,%0",
				  X86_FEATURE_CR4,
				  "r" (cr4));
		return cr4;
	}

and then just add that feature-flag discovery early on in boot (it needs to be pretty early, since the alternative instruction rewriting happens early).

We have several "calculated" features already. Things like X86_FEATURE_P4 etc.

Linus

Yes, this is fine, but is it worth writing the feature discovery code? I suppose it doesn't matter, as it gets jettisoned after init. I guess it is just preference.

Considering run time code size, the alternative approach wins, has no extra branches, and is just nicer. The faulting technique requires two extra dwords of space that can not be jettisonned. So obviously, I must do it (the alternative approach).

Could we consider doing the same with LOCK prefix for SMP kernels booted on UP? Evil grin.

Zach

Well, you could do the feature discovery by trying to take a fault early at boot-time. That's how we verify that write-protect works, and how we check that math exceptions come in the right way..

Not so evil - I think it's been discussed. Not with alternates (not worth it), but it wouldn't be hard to do: just add a new section for "lock address", and have each inline asm that does a lock prefix do basically

	1:
		lock ; xyzzy

	.section .lock.address
	.long 1b
	.previous

and then just walk the ".lock.address" thing and turn all locks into 0x90 (nop).

Linus

Looks like the Ubuntu people already did this...

http://www.kernel.org/git/?p=linux/kernel/git/bcollins/ubuntu-2.6.git;a=commitdiff;h=048985336e32efe665cddd348e92e4a4a5351415;hp=1cb630c2b5aaad7cedaa78aa135e6cecf5ab91ac

Dave

It's probably not needed. At least AMD K7/K8 has a SYSCFG MSR bit to do this (or rather they disable bus cycles for locks that makes them very cheap) Intel has one too in a different MSR that looks similar. With some luck they're even already set by the BIOS on UP systems. I know they are on some AMD systems.

But overall the feature doesn't help longer term because single threaded CPUs are on their way out.

-Andi

I'd hope the vendors are not doing that by default because we have kernel code that uses lock against not other processors but other bus masters. The ECC code is one example. Is there any good info on the AMD one so I can make the EDAC code put the processor back in x86 compatible mode so that it behaves safely when scrubbing.

Alan

Check out the AMD's BIOS and Kernel Programmer Guide for the K8. The appropriate bits are documented, although the documentation is quite terse.

Eric

It's a bad hack anyways. Better would be probably to use a uncached WC write. I would rather use that.

-Andi

For read modify write?

The point is to make the cache line dirty so that the memory controller will write the data back.

The interesting sequence is:
lock; addl $0, %(reg)

I'm not actually sure the lock is even necessary. Mostly this is for brain-dead chipsets, chipsets you can't trust, or at least chipsets that won't do a background scrub for you.

I don't think it is possible to do an uncached read modify write?

Eric

I'm not clear that anything but lock operations have the required guarantee of atomicity relative to bus masters which are not processors. Especially so on intel.

The thing is, we wouldn't ever remove _all_ lock prefixes. Only the ones that already depend on SMP.

So the memory barriers etc that have lock prefixes even on UP would be totally untouched.

Linus

That much makes sense. Having some magic MSR reloaded to turn lock effects off is a bit more of a problem for ECC scrubbing however.

well... you can expect many bioses to have done the MSR hack for you already... so if you can't cope with that you have to set the MSR to the value you want it to have regardless.

I can't speak about AMD, but on Transmeta's CPUs operations against cached memory are *always* atomic; the atomicity is guaranteed by the cache hierarchy. The LOCK prefix does have effects against uncached memory.

-hpa

Yeah, that looks like a sane patch, although I dislike the #ifdef config option thing (either it works or it doesn't).

It also does it the right way: using LOCK_PREFIX means that you catch exactly the users that depend on SMP, and not _all_ "lock" prefixes (as mentioned, some of the lock prefixes are there as memory fences and are valid and needed even on UP). So me likee.

The only question being whether you'd actually want to nop out the spinlock instructions _entirely_ (in addition to changing the nops on things like semaphores). Without the lock, they're not that expensive, but hey, it's still a useless (memory-modifying) instruction.

Linus

Actually, that may turn out to be a dangerous idea.

Sad but true: There's a few tests like

	#define assert_spin_locked(x)  BUG_ON(!spin_is_locked(x))

and

	#define __raw_spin_unlock_wait(lock) \
		do { while (__raw_spin_is_locked(lock)) cpu_relax(); } while (0)

that would also need to be nopped out if we nop out the code that updates the spinlock (right now they are just disabled entirely on UP, exactly because tests like this don't work without the lock being instantiated).

But it would be wonderful if we could just nop out the whole call to the spinlock (most of them are out-of-line). It would help I$ footprint, and likely help improve dynamic scheduling around that call on many CPU's too.

So we can easily remove the lock prefix on the spinlock ops, but sadly we can't do some other "obvious" optimizations.

We _could_ nop out the actual conditional on the lock result for a spinlock, and turn

	lock ; decb %0
	js ...

into

nop ; decb %0
multi-byte-nop

which would help avoid some unnecessary branch prediction etc.

Linus

Hi,

Throwing another patch into the discussion ;)

Comes from some xen guy. If I read the thing correctly it builds a elf section containing a table with both smp and up versions of the code path, then patching in the one needed at runtime. Allows patching both directions (up->smp, smp->up) at runtime, for hotplugging (virtual) CPU's. I'm not a inline asm expert though ...

Comments on that one?

Gerd

diff -Naur linux-2.6.12/arch/i386/Kconfig linux-2.6.12.post/arch/i386/Kconfig
@@ -487,6 +487,19 @@
 
 	  If you don't know what to do here, say N.
 
+config SMP_ALTERNATIVES
+	bool "SMP alternatives support (EXPERIMENTAL)"
+	depends on SMP && EXPERIMENTAL
+	help
+	  Try to reduce the overhead of running an SMP kernel on a uniprocessor
+	  host slightly by replacing certain key instruction sequences
+	  according to whether we currently have more than one CPU available.
+	  This should provide a noticeable boost to performance when
+	  running SMP kernels on UP machines, and have negligible impact
+	  when running on an true SMP host.
+
+          If unsure, say N.
+	  
 config NR_CPUS
 	int "Maximum number of CPUs (2-255)"
 	range 2 255

diff -Naur linux-2.6.12/arch/i386/kernel/Makefile linux-2.6.12.post/arch/i386/kernel/Makefile
@@ -33,6 +33,7 @@
 obj-$(CONFIG_HPET_TIMER) 	+= time_hpet.o
 obj-$(CONFIG_EFI) 		+= efi.o efi_stub.o
 obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
+obj-$(CONFIG_SMP_ALTERNATIVES)  += smpalts.o
 
 EXTRA_AFLAGS   := -traditional
 

diff -Naur linux-2.6.12/arch/i386/kernel/smpboot.c linux-2.6.12.post/arch/i386/kernel/smpboot.c
@@ -1001,6 +1001,11 @@
 		if (max_cpus <= cpucount+1)
 			continue;
 
+#ifdef CONFIG_SMP_ALTERNATIVES
+		if (kicked == 1)
+			prepare_for_smp();
+#endif
+
 		if (do_boot_cpu(apicid))
 			printk("CPU #%d not responding - cannot use it.\n",
 								apicid);

@@ -1130,6 +1135,11 @@
 		return -EIO;
 	}
 
+#ifdef CONFIG_SMP_ALTERNATIVES
+	if (num_online_cpus() == 1)
+		prepare_for_smp();
+#endif
+
 	local_irq_enable();
 	/* Unleash the CPU! */
 	cpu_set(cpu, smp_commenced_mask);

diff -Naur linux-2.6.12/arch/i386/kernel/vmlinux.lds.S linux-2.6.12.post/arch/i386/kernel/vmlinux.lds.S
@@ -30,6 +30,13 @@
   __ex_table : { *(__ex_table) }
   __stop___ex_table = .;
 
+  . = ALIGN(16);
+  __start_smp_alternatives_table = .;
+  __smp_alternatives : { *(__smp_alternatives) }
+  __stop_smp_alternatives_table = .;
+
+  __smp_replacements : { *(__smp_replacements) }
+
   RODATA
 
   /* writeable */

diff -Naur linux-2.6.12/include/asm-i386/atomic.h linux-2.6.12.post/include/asm-i386/atomic.h
@@ -4,18 +4,13 @@
 #include <linux/config.h>
 #include <linux/compiler.h>
 #include <asm/processor.h>
+#include <asm/smp_alt.h>
 
 /*
  * Atomic operations that C can't guarantee us.  Useful for
  * resource counting etc..
  */
 
-#ifdef CONFIG_SMP
-#define LOCK "lock ; "
-#else
-#define LOCK ""
-#endif
-
 /*
  * Make sure gcc doesn't try to be clever and move things around
  * on us. We need to use _exactly_ the address the user gave us,

diff -Naur linux-2.6.12/include/asm-i386/bitops.h linux-2.6.12.post/include/asm-i386/bitops.h
@@ -7,6 +7,7 @@
 
 #include <linux/config.h>
 #include <linux/compiler.h>
+#include <asm/smp_alt.h>
 
 /*
  * These have to be done with inline assembly: that way the bit-setting

@@ -16,12 +17,6 @@
  * bit 0 is the LSB of addr; bit 32 is the LSB of (addr+1).
  */
 
-#ifdef CONFIG_SMP
-#define LOCK_PREFIX "lock ; "
-#else
-#define LOCK_PREFIX ""
-#endif
-
 #define ADDR (*(volatile long *) addr)
 
 /**

@@ -41,7 +36,7 @@
  */
 static inline void set_bit(int nr, volatile unsigned long * addr)
 {
-	__asm__ __volatile__( LOCK_PREFIX
+	__asm__ __volatile__( LOCK
 		"btsl %1,%0"
 		:"=m" (ADDR)
 		:"Ir" (nr));

@@ -76,7 +71,7 @@
  */
 static inline void clear_bit(int nr, volatile unsigned long * addr)
 {
-	__asm__ __volatile__( LOCK_PREFIX
+	__asm__ __volatile__( LOCK
 		"btrl %1,%0"
 		:"=m" (ADDR)
 		:"Ir" (nr));

@@ -121,7 +116,7 @@
  */
 static inline void change_bit(int nr, volatile unsigned long * addr)
 {
-	__asm__ __volatile__( LOCK_PREFIX
+	__asm__ __volatile__( LOCK
 		"btcl %1,%0"
 		:"=m" (ADDR)
 		:"Ir" (nr));

@@ -140,7 +135,7 @@
 {
 	int oldbit;
 
-	__asm__ __volatile__( LOCK_PREFIX
+	__asm__ __volatile__( LOCK
 		"btsl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
 		:"Ir" (nr) : "memory");

@@ -180,7 +175,7 @@
 {
 	int oldbit;
 
-	__asm__ __volatile__( LOCK_PREFIX
+	__asm__ __volatile__( LOCK
 		"btrl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
 		:"Ir" (nr) : "memory");

@@ -231,7 +226,7 @@
 {
 	int oldbit;
 
-	__asm__ __volatile__( LOCK_PREFIX
+	__asm__ __volatile__( LOCK
 		"btcl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
 		:"Ir" (nr) : "memory");

diff -Naur linux-2.6.12/include/asm-i386/rwsem.h linux-2.6.12.post/include/asm-i386/rwsem.h
@@ -40,6 +40,7 @@
 
 #include <linux/list.h>
 #include <linux/spinlock.h>
+#include <asm/smp_alt.h>
 
 struct rwsem_waiter;
 

@@ -99,7 +100,7 @@
 {
 	__asm__ __volatile__(
 		"# beginning down_read\n\t"
-LOCK_PREFIX	"  incl      (%%eax)\n\t" /* adds 0x00000001, returns the old value */
+LOCK	        "  incl      (%%eax)\n\t" /* adds 0x00000001, returns the old value */
 		"  js        2f\n\t" /* jump if we weren't granted the lock */
 		"1:\n\t"
 		LOCK_SECTION_START("")

@@ -130,7 +131,7 @@
 		"  movl	     %1,%2\n\t"
 		"  addl      %3,%2\n\t"
 		"  jle	     2f\n\t"
-LOCK_PREFIX	"  cmpxchgl  %2,%0\n\t"
+LOCK	        "  cmpxchgl  %2,%0\n\t"
 		"  jnz	     1b\n\t"
 		"2:\n\t"
 		"# ending __down_read_trylock\n\t"

@@ -150,7 +151,7 @@
 	tmp = RWSEM_ACTIVE_WRITE_BIAS;
 	__asm__ __volatile__(
 		"# beginning down_write\n\t"
-LOCK_PREFIX	"  xadd      %%edx,(%%eax)\n\t" /* subtract 0x0000ffff, returns the old value */
+LOCK	        "  xadd      %%edx,(%%eax)\n\t" /* subtract 0x0000ffff, returns the old value */
 		"  testl     %%edx,%%edx\n\t" /* was the count 0 before? */
 		"  jnz       2f\n\t" /* jump if we weren't granted the lock */
 		"1:\n\t"

@@ -188,7 +189,7 @@
 	__s32 tmp = -RWSEM_ACTIVE_READ_BIAS;
 	__asm__ __volatile__(
 		"# beginning __up_read\n\t"
-LOCK_PREFIX	"  xadd      %%edx,(%%eax)\n\t" /* subtracts 1, returns the old value */
+LOCK	        "  xadd      %%edx,(%%eax)\n\t" /* subtracts 1, returns the old value */
 		"  js        2f\n\t" /* jump if the lock is being waited upon */
 		"1:\n\t"
 		LOCK_SECTION_START("")

@@ -214,7 +215,7 @@
 	__asm__ __volatile__(
 		"# beginning __up_write\n\t"
 		"  movl      %2,%%edx\n\t"
-LOCK_PREFIX	"  xaddl     %%edx,(%%eax)\n\t" /* tries to transition 0xffff0001 -> 0x00000000 */
+LOCK	        "  xaddl     %%edx,(%%eax)\n\t" /* tries to transition 0xffff0001 -> 0x00000000 */
 		"  jnz       2f\n\t" /* jump if the lock is being waited upon */
 		"1:\n\t"
 		LOCK_SECTION_START("")

@@ -239,7 +240,7 @@
 {
 	__asm__ __volatile__(
 		"# beginning __downgrade_write\n\t"
-LOCK_PREFIX	"  addl      %2,(%%eax)\n\t" /* transitions 0xZZZZ0001 -> 0xYYYY0001 */
+LOCK	        "  addl      %2,(%%eax)\n\t" /* transitions 0xZZZZ0001 -> 0xYYYY0001 */
 		"  js        2f\n\t" /* jump if the lock is being waited upon */
 		"1:\n\t"
 		LOCK_SECTION_START("")

@@ -263,7 +264,7 @@
 static inline void rwsem_atomic_add(int delta, struct rw_semaphore *sem)
 {
 	__asm__ __volatile__(
-LOCK_PREFIX	"addl %1,%0"
+LOCK	          "addl %1,%0"
 		: "=m"(sem->count)
 		: "ir"(delta), "m"(sem->count));
 }

@@ -276,7 +277,7 @@
 	int tmp = delta;
 
 	__asm__ __volatile__(
-LOCK_PREFIX	"xadd %0,(%2)"
+LOCK  	          "xadd %0,(%2)"
 		: "+r"(tmp), "=m"(sem->count)
 		: "r"(sem), "m"(sem->count)
 		: "memory");

diff -Naur linux-2.6.12/include/asm-i386/smp_alt.h linux-2.6.12.post/include/asm-i386/smp_alt.h
@@ -0,0 +1,32 @@
+#ifndef __ASM_SMP_ALT_H__
+#define __ASM_SMP_ALT_H__
+
+#include <linux/config.h>
+
+#ifdef CONFIG_SMP
+#if defined(CONFIG_SMP_ALTERNATIVES) && !defined(MODULE)
+#define LOCK \
+        "6677: nop\n" \
+	".section __smp_alternatives,\"a\"\n" \
+	".long 6677b\n" \
+	".long 6678f\n" \
+	".previous\n" \
+	".section __smp_replacements,\"a\"\n" \
+	"6678: .byte 1\n" \
+	".byte 1\n" \
+	".byte 0\n" \
+        ".byte 1\n" \
+	".byte -1\n" \
+	"lock\n" \
+	"nop\n" \
+	".previous\n"
+void prepare_for_smp(void);
+void unprepare_for_smp(void);
+#else
+#define LOCK "lock ; "
+#endif
+#else
+#define LOCK ""
+#endif
+
+#endif /* __ASM_SMP_ALT_H__ */

Ouch, this one is really ugly.

If you want to go this way, then you should instead add an X86_FEATURE_SMP that gets cleared on UP and on SMP with just one core (and detect when CPU hotplug ain't gonna happen ;), and then do

	#ifdef CONFIG_SMP
	#define smp_alternative(x,y) alternative(x,y,X86_FEATURE_SMP)
	#else
	#define smp_alternative(x,y) asm(x)
	#endif

or something similar, instead of creating a totally new infrastructure to do the thing that "alternative()" already does.

(Yeah, the above doesn't really work, since usually the SMP form is the longer one, and "alternative()" wants the long complex one first. So maybe the x86 feature needs to be "X86_FEATURE_UP" instead, since it's now a "feature" to only have one core ;)

Linus

It seems that SMP vs. UP lock / spinlock overhead is relevant even for future, multi-core CPUs in a virtualization context, as the notion of hotplug here is based on scheduling constraints of the virtualization engine, and the kernel can quite readily end up with only one VCPU.

But it also seems that there are separate, competing mechanisms for implementing this dynamic code change, which is undesirable. The notion of boot-time dynamic code change for SMP is useful for native hardware. Run-time dynamic code change is useful for virtual hardware, and minimally useful for hardware CPU hotplug. Run-time dynamic code change is also useful on virtual hardware if you consider live kernel migrations across CPUs from different vendors, or with different features. Again, this is minimally useful for hardware CPU hotplug.

But in essence, there should be one nice way to encapsulate this code modification that lives for both run-time and boot-time code. The boot-time modifiers can jettison the alternative tables, and the run-time guys (which might include CPU hotplug) can keep those alternatives around so they can be unapplied later. One can even imagine more complex alternative features (if I have SSE2, use code X, but if SSE3 is available use code Y, else fall back to code Z) being useful at some point.

Both points combined are a basic argument for providing an alternative choice function in apply_alternatives, which takes as input the alternative specification, and returns a pointer to the chosen code. This function can be driven by dynamic data (number of plugged CPUs), or by static specifications (feature spec in the alternative section).

Zach

this assumes that you don't just always want to assume and use SMP primitives in a virtualized context. I sort of question that assumption; sure these things have overhead, especially "lock", but if the solution is more complexity and weird things to hide that half-percent or less of performance difference... then do remember that such complexity is not free either. Runtime tricks cost.

Runtime tricks that increase complexity cost, yes. It's all a question of measured gain vs. complexity. But a couple of percent gained on an overall basis can be magnified enormously if you are looking at a workload that stresses a particular path. I would expect some of those gains to be non-trivial, especially if considering the optimizations you could do on page table updates knowing you needn't worry about SMP issues anymore. Even UP has (still?) some places where additional locks are present here, and could benefit from having SMP alternatives.

Zach

a couple of percents sounds really really high to me. If it's really that then I think Andi's conclusion is wrong with respect to that locking cliff; if we spend a few percent of our performance on locks in the uncontended case we're way over the edge in my opinion.

page table updates happen in the hypervisor in a xen like paravirtualized setup right? so that happens outside the kernel..

I somehow expected that answer, it took me quite some time to figure what the patch does. It certainly needs at least a number of cleanups before I'd consider it mergable. The alternative() macro is much easier to read.

Well, the "no hotplug" probably is exactly the reason why the patch doesn't use the existing alternatives mechanism, it's a boot-time one-way ticket. The xenified linux kernel actually switches both ways at runtime if you plug in/out a second virtual CPU.

I don't like the idea very much. That covers only 50% of what the patch does, you can patch SMP => UP but not the other way around. Doesn't matter much on real hardware, but for virtual it is quite useful.

Yep, extending alternatives is probably better than duplicating the code. Maybe having some alternative_smp() macro which places both code versions into the .altinstr_replacement table? If that sounds ok I'll try to come up with a experimental patch. If not: other ideas are welcome.

cheers,

Gerd

i.e. something like this (as basic idea, patch is far away from doing anything useful ...)?

Gerd

diff -pu -urp linux-2.6.14/arch/i386/kernel/setup.c work-2.6.14/arch/i386/kernel/setup.c
@@ -1435,9 +1435,9 @@ void apply_alternatives(void *start, voi
 		}
 	} 
 	for (a = start; (void *)a < end; a++) { 
+		BUG_ON(a->replacementlen > a->instrlen); 
 		if (!boot_cpu_has(a->cpuid))
 			continue;
-		BUG_ON(a->replacementlen > a->instrlen); 
 		memcpy(a->instr, a->replacement, a->replacementlen); 
 		diff = a->instrlen - a->replacementlen; 
 		/* Pad the rest with nops */

@@ -1450,6 +1450,19 @@ void apply_alternatives(void *start, voi
 	}
 } 
 
+void apply_alternatives_smp(void *start, void *end) 
+{ 
+	struct alt_instr *a; 
+
+	for (a = start; (void *)a < end; a++) { 
+		if (X86_FEATURE_UP != a->cpuid)
+			continue;
+		memcpy(a->instr,
+		       a->replacement + a->replacementlen,
+		       a->instrlen);
+	}
+} 
+
 void __init alternative_instructions(void)
 {
 	extern struct alt_instr __alt_instructions[], __alt_instructions_end[];

diff -pu -urp linux-2.6.14/include/asm-i386/cpufeature.h work-2.6.14/include/asm-i386/cpufeature.h
@@ -70,6 +70,8 @@
 #define X86_FEATURE_P3		(3*32+ 6) /* P3 */
 #define X86_FEATURE_P4		(3*32+ 7) /* P4 */
 
+#define X86_FEATURE_UP		(3*32+ 8) /* smp kernel running on up */
+
 /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
 #define X86_FEATURE_XMM3	(4*32+ 0) /* Streaming SIMD Extensions-3 */
 #define X86_FEATURE_MWAIT	(4*32+ 3) /* Monitor/Mwait support */

diff -pu -urp linux-2.6.14/include/asm-i386/system.h work-2.6.14/include/asm-i386/system.h
@@ -329,6 +329,25 @@ struct alt_instr { 
 		      "663:\n\t" newinstr "\n664:\n"   /* replacement */    \
 		      ".previous" :: "i" (feature) : "memory")  
 
+#ifdef CONFIG_SMP
+#define alternative_smp(smpinstr, upinstr, input) 	\
+	asm volatile ("661:\n\t" smpinstr "\n662:\n" 		     \
+		      ".section .altinstructions,\"a\"\n"     	     \
+		      "  .align 4\n"				       \
+		      "  .long 661b\n"            /* label */          \
+		      "  .long 663f\n"		  /* new instruction */ 	\
+		      "  .byte %c0\n"             /* feature bit */    \
+		      "  .byte 662b-661b\n"       /* sourcelen */      \
+		      "  .byte 664f-663f\n"       /* replacementlen */ \
+		      ".previous\n"						\
+		      ".section .altinstr_replacement,\"ax\"\n"			\
+		      "663:\n\t" upinstr "\n"     /* replacement */    \
+		      "664:\n\t" smpinstr "\n"    /* original again */ \
+		      ".previous" :: "i" (X86_FEATURE_UP), ##input)
+#else
+#define alternative_smp(smpinstr, upinstr) asm(upinstr, ##input)
+#endif
+
 /*
  * Alternative inline assembly with input.
  * 

You still need to preserve the originals so that you can patch in both directions. In the dynamic scenario, you need a multi-way set of alternatives, with the most conservative of those compiled in inline.

Zach

why do you insist on both directions? That still sounds like real overkill to me.

cpu hotplug going from UP to SMP ? :)

Dave

If you have CPU hotplug enabled, you can run SMP code!

-hpa

Sure, but if you boot with 1 CPU, spinlocks get nop'd to emulate UP, and on a 'installed a new cpu' hotplug event, they all come back.

Dave

the good news is that all hotplugable x86 cpus will have HT or dual core support.. so you always work in pairs of 2

While there are good arguments for a pure SMP hardware world, there are good arguments in the virtualization world for virtual uniprocessors. No one can be sure which combination of HT / polycore / package isolation we will end up with, but we can be sure that legacy systems will be around for longer than anyone wants to think about. As this CR4 valid on 486s patch proves :)

The point that you don't nop if you have hotplug enabled (which is not the norm.)

-hpa

It's not overkill in the virtualization context, and there are (struggling, but infinite possibilities) opportunities for native here as well. Run-time SMP->UP->SMP can benefit hotplug (albeit slightly). But once you have a basic, generic mechanism for run-time code modularization, there is very little cost to adding other features. Run-time PAE / non-PAE conversion is far more radical, but not outside the realm of possibility - and useful (in both directions) for memory hotplug. Run-time CPU vendor migration is possible, if you, say hotplug an AMD chip into a previously Intel socket.

Sure, most of this is science fiction. But the possibilities are great - it's another tool you can use towards modularizing functionality - specifically, scattered functionality like CPU instructions, spinlocks, and MMU operations that really do deserve to be inlined, and really can benefit from taking advantage of faster hardware instruction sequences. That the tool already exists in a limited form means that with natural extensions, it could easily be refined to allow bi-directional or multidirectional run-time choices.

Basically, it removes a lot of the barriers that force configuration time choices on the running kernel, and you can start to look at even deeply entrenched parts of the kernel as modular.

Zach

No, there are almost no opportunities for native.

Especially with SMP, doing on-line code switching is really really nasty. You basically have to shut down all CPU's to make sure there are no races with other CPU's executing the code while it's being rewritten.

I'd be very very nervous about it. It would have to be some major performance feature for it to make sense over a simple "switch function pointers around" approach.

Linus

Sure, alternatives_smp() puts both versions into the .altinstr_replacement section because of that ;)

The idea is to have SMP compiled in and let the normal apply_alternatives() handle the SMP->UP patching case using the new feature bit. apply_alternatives_smp() handles UP->SMP patching when you plug in a new virtual CPU.

cheers,

Gerd

this wouldn't build with CONFIG_SMP=n -- you forgot the input param here.

also, given this:

Yep, and I've noticed meanwhile that it becomes quite messy if you try to do that with asm instructions which have both input and output parameters. One way around that would be to use named parameters in the inline assembler. Problem with that is that only gcc >= 3.1 understands those and at the moment the minimun requited compiler for the kernel still is gcc 2.95.3 according to Documentation/Changes ...

Is it an option to raise the required gcc version to 3.x, given that even Debian/stable ships with gcc 3.3 these days?

cheers,

Gerd

Can't work. The altinstructions are in init-code/data, and will be free'd after boot. Which is as it should be. But it means that any setup that expects to use them to switch back and forth is broken (not that your patch does so now, but if that's what you are moving toward..)

Linus

Hmmm, what about modules ?

Dave

Good point, so better place the ones we need at runtime into a separate table ...

We'll need some new fields in struct module ...

Is already on the list in my head, but I didn't bother yet for that proof-of-concept discussion patch ;)

Gerd

Adapting $subject to the actual topic, so other lkml readers can catch up ;)

Ok, here new version of the SMP alternatives patch. It features:

* it actually compiles and boots, so you can start playing with it ;)
* reuses the alternatives bits we have already as far as possible.
* separate table for the SMP alternatives, so we can keep them and switch at runtime between SMP and UP (for virtual CPU hotplug).
* two new alternatives macros, one generic which can handle quite comples stuff such as spinlocks, one for the "lock prefix" case.

TODO list:

* convert more code (bitops, ...).
* module support (using modules is fine, they run the safe SMP version of the code, they just don't benefit from the optimizations yet).
* integrate with xen bits and CPU hotplug, at the moment it's a boot-time only thing.
* benchmark it.
* x86_64 version.
* drop the printk's placed into the code for debugging.
* probably more ...

How it works right now:

* The patch switches to UP unconditionally when doing the usual alternatives stuff at boot time
* Just before booting the second CPU it switches to SMP.

How to test:

* boot with "maxcpus=1" to run the UP code.

Comments are welcome.

cheers,

Gerd

diff -pu -urp linux-2.6.14/arch/i386/kernel/setup.c work-2.6.14/arch/i386/kernel/setup.c
@@ -1423,10 +1423,10 @@ static struct nop { 
    self modifying code. This implies that assymetric systems where
    APs have less capabilities than the boot processor are not handled. 
    Tough. Make sure you disable such features by hand. */ 
-void apply_alternatives(void *start, void *end) 
+int apply_alternatives(void *start, void *end) 
 { 
 	struct alt_instr *a; 
-	int diff, i, k;
+	int diff, i, k, count=0;
         unsigned char **noptable = intel_nops; 
 	for (i = 0; noptypes[i].cpuid >= 0; i++) { 
 		if (boot_cpu_has(noptypes[i].cpuid)) { 

@@ -1435,9 +1435,9 @@ void apply_alternatives(void *start, voi
 		}
 	} 
 	for (a = start; (void *)a < end; a++) { 
+		BUG_ON(a->replacementlen > a->instrlen); 
 		if (!boot_cpu_has(a->cpuid))
 			continue;
-		BUG_ON(a->replacementlen > a->instrlen); 
 		memcpy(a->instr, a->replacement, a->replacementlen); 
 		diff = a->instrlen - a->replacementlen; 
 		/* Pad the rest with nops */

@@ -1446,14 +1446,49 @@ void apply_alternatives(void *start, voi
 			if (k > ASM_NOP_MAX)
 				k = ASM_NOP_MAX;
 			memcpy(a->instr + i, noptable[k], k); 
-		} 
+		}
+		count++;
 	}
+	return count;
+} 
+
+extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
+extern struct alt_instr __smp_alt_instructions[], __smp_alt_instructions_end[];
+
+void switch_alternatives_up(void) 
+{ 
+	int n;
+
+	set_bit(X86_FEATURE_UP, boot_cpu_data.x86_capability);
+	n = apply_alternatives(__smp_alt_instructions, __smp_alt_instructions_end);
+	/* TODO: handle modules */
+	printk("%s: %d replacements\n", __FUNCTION__, n);
+} 
+
+void switch_alternatives_smp(void) 
+{ 
+	struct alt_instr *a   = __smp_alt_instructions;
+	struct alt_instr *end = __smp_alt_instructions_end;
+	int count = 0;
+
+	clear_bit(X86_FEATURE_UP, boot_cpu_data.x86_capability);
+	for (; a < end; a++) { 
+		memcpy(a->instr,
+		       a->replacement + a->replacementlen,
+		       a->instrlen);
+		count++;
+	}
+	/* TODO: handle modules */
+	printk("%s: %d replacements\n", __FUNCTION__, count);
 } 
 
 void __init alternative_instructions(void)
 {
-	extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
-	apply_alternatives(__alt_instructions, __alt_instructions_end);
+	int n;
+
+	n = apply_alternatives(__alt_instructions, __alt_instructions_end);
+	printk("%s: %d replacements\n", __FUNCTION__, n);
+	switch_alternatives_up();
 }
 
 static char * __init machine_specific_memory_setup(void);

diff -pu -urp linux-2.6.14/arch/i386/kernel/smpboot.c work-2.6.14/arch/i386/kernel/smpboot.c
@@ -1074,6 +1074,8 @@ void *xquad_portio;
 EXPORT_SYMBOL(xquad_portio);
 #endif
 
+extern void switch_alternatives_smp(void);
+
 static void __init smp_boot_cpus(unsigned int max_cpus)
 {
 	int apicid, cpu, bit, kicked;

@@ -1184,6 +1186,8 @@ static void __init smp_boot_cpus(unsigne
 			continue;
 		if (max_cpus <= cpucount+1)
 			continue;
+		if (1 == kicked)
+			switch_alternatives_smp();
 
 		if (((cpu = alloc_cpu_id()) <= 0) || do_boot_cpu(apicid, cpu))
 			printk("CPU #%d not responding - cannot use it.\n",

diff -pu -urp linux-2.6.14/arch/i386/kernel/vmlinux.lds.S work-2.6.14/arch/i386/kernel/vmlinux.lds.S
@@ -68,6 +68,16 @@ SECTIONS
 	*(.data.init_task)
   }
 
+  . = ALIGN(4);
+  __smp_alt_instructions = .;
+  .smp_altinstructions : AT(ADDR(.smp_altinstructions) - LOAD_OFFSET) {
+	*(.smp_altinstructions)
+  }
+  __smp_alt_instructions_end = .; 
+  .smp_altinstr_replacement : AT(ADDR(.smp_altinstr_replacement) - LOAD_OFFSET) {
+	*(.smp_altinstr_replacement)
+  }
+
   /* will be freed after init */
   . = ALIGN(4096);		/* Init code and data */
   __init_begin = .;

diff -pu -urp linux-2.6.14/include/asm-i386/cpufeature.h work-2.6.14/include/asm-i386/cpufeature.h
@@ -70,6 +70,8 @@
 #define X86_FEATURE_P3		(3*32+ 6) /* P3 */
 #define X86_FEATURE_P4		(3*32+ 7) /* P4 */
 
+#define X86_FEATURE_UP		(3*32+ 8) /* smp kernel running on up */
+
 /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
 #define X86_FEATURE_XMM3	(4*32+ 0) /* Streaming SIMD Extensions-3 */
 #define X86_FEATURE_MWAIT	(4*32+ 3) /* Monitor/Mwait support */

diff -pu -urp linux-2.6.14/include/asm-i386/spinlock.h work-2.6.14/include/asm-i386/spinlock.h
@@ -48,18 +48,23 @@
 	"jmp 1b\n" \
 	"4:\n\t"
 
+#define __raw_spin_lock_string_up \
+	"\n\tdecb %0"
+
 static inline void __raw_spin_lock(raw_spinlock_t *lock)
 {
-	__asm__ __volatile__(
-		__raw_spin_lock_string
-		:"=m" (lock->slock) : : "memory");
+	alternative_smp(
+		__raw_spin_lock_string,
+		__raw_spin_lock_string_up,
+		"=m" (lock->slock) : : "memory");
 }
 
 static inline void __raw_spin_lock_flags(raw_spinlock_t *lock, unsigned long flags)
 {
-	__asm__ __volatile__(
-		__raw_spin_lock_string_flags
-		:"=m" (lock->slock) : "r" (flags) : "memory");
+	alternative_smp(
+		__raw_spin_lock_string_flags,
+		__raw_spin_lock_string_up,
+		"=m" (lock->slock) : "r" (flags) : "memory");
 }
 
 static inline int __raw_spin_trylock(raw_spinlock_t *lock)

@@ -178,13 +183,16 @@ static inline int __raw_write_trylock(ra
 
 static inline void __raw_read_unlock(raw_rwlock_t *rw)
 {
-	asm volatile("lock ; incl %0" :"=m" (rw->lock) : : "memory");
+	alternative_smp_lock(
+		"incl %0",
+		"=m" (rw->lock) : : "memory");
 }
 
 static inline void __raw_write_unlock(raw_rwlock_t *rw)
 {
-	asm volatile("lock ; addl $" RW_LOCK_BIAS_STR ", %0"
-				 : "=m" (rw->lock) : : "memory");
+	alternative_smp_lock(
+		"addl $" RW_LOCK_BIAS_STR ", %0",
+		"=m" (rw->lock) : : "memory");
 }
 
 #endif /* __ASM_SPINLOCK_H */

diff -pu -urp linux-2.6.14/include/asm-i386/system.h work-2.6.14/include/asm-i386/system.h
@@ -329,6 +329,42 @@ struct alt_instr { 
 		      "663:\n\t" newinstr "\n664:\n"   /* replacement */    \
 		      ".previous" :: "i" (feature) : "memory")  
 
+#ifdef CONFIG_SMP
+#define alternative_smp(smpinstr, upinstr, args...) 	\
+	asm volatile ("661:\n\t" smpinstr "\n662:\n" 		     \
+		      ".section .smp_altinstructions,\"a\"\n"          \
+		      "  .align 4\n"				       \
+		      "  .long 661b\n"            /* label */          \
+		      "  .long 663f\n"		  /* new instruction */ 	\
+		      "  .byte 0x68\n"            /* X86_FEATURE_UP */    \
+		      "  .byte 662b-661b\n"       /* sourcelen */      \
+		      "  .byte 664f-663f\n"       /* replacementlen */ \
+		      ".previous\n"						\
+		      ".section .smp_altinstr_replacement,\"ax\"\n"    		\
+		      "663:\n\t" upinstr "\n"     /* replacement */    \
+		      "664:\n\t" smpinstr "\n"    /* original again */ \
+		      ".previous" : args)
+#define alternative_smp_lock(lockinstr, args...) 	\
+	asm volatile ("661:\n\tlock; " lockinstr "\n" 		     \
+		      ".section .smp_altinstructions,\"a\"\n"          \
+		      "  .align 4\n"				       \
+		      "  .long 661b\n"            /* label */          \
+		      "  .long 663f\n"		  /* new instruction */ 	\
+		      "  .byte 0x68\n"            /* X86_FEATURE_UP */    \
+		      "  .byte 1\n"               /* sourcelen */      \
+		      "  .byte 0\n"               /* replacementlen */ \
+		      ".previous\n"						\
+		      ".section .smp_altinstr_replacement,\"ax\"\n"    		\
+		      "663:\n"                    /* replacement */    \
+		      "664:\n\tlock\n"            /* original again */ \
+		      ".previous" : args)
+#else
+#define alternative_smp(smpinstr, upinstr, args...) \
+	asm volatile (upinstr : args)
+#define alternative_smp_lock(lockinstr, args...) \
+	asm volatile (lockinstr : args)
+#endif
+
 /*
  * Alternative inline assembly with input.
  * 

Now, some days hacking & debugging and kernel crashing later I have something more than just proof-of-concept ;)

Modules are supported now, fully modularized distro kernel works fine with it. If you have a kernel with HOTPLUG_CPU compiled you can shutdown the second CPU of your dual-processor system via sysfs (echo 0 > /sys/devices/system/cpu/cpu1/online) and watch the kernel switch over to UP code without lock-prefixed instructions and simplified spinlocks, then power up the second CPU again (echo 1 > /sys/...) and watch it patching back in the SMP locking.

For testing & benchmarking purposes I've put also in two (temporary) sysrq's to switch between UP and SMP bits without booting/shutting down the second CPU. That one breaks non-i386 builds which are trivially fixable by just dropping the drivers/char/sysrq.c changes ;)

enjoy,

Gerd

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/arch/i386/kernel/Makefile work-2.6.14/arch/i386/kernel/Makefile
@@ -7,7 +7,7 @@
 obj-y	:= process.o semaphore.o signal.o entry.o traps.o irq.o vm86.o \
 		ptrace.o time.o ioport.o ldt.o setup.o i8259.o sys_i386.o \
 		pci-dma.o i386_ksyms.o i387.o dmi_scan.o bootflag.o \
-		doublefault.o quirks.o i8237.o
+		doublefault.o quirks.o i8237.o alternative.o
 
 obj-y				+= cpu/
 obj-y				+= timers/

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/arch/i386/kernel/module.c work-2.6.14/arch/i386/kernel/module.c
@@ -104,26 +104,39 @@
 	return -ENOEXEC;
 }
 
-extern void apply_alternatives(void *start, void *end); 
-
 int module_finalize(const Elf_Ehdr *hdr,
 		    const Elf_Shdr *sechdrs,
 		    struct module *me)
 {
-	const Elf_Shdr *s;
+	const Elf_Shdr *s, *text = NULL, *alt = NULL, *smpalt = NULL;
 	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
 
-	/* look for .altinstructions to patch */ 
 	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) { 
-		void *seg; 		
-		if (strcmp(".altinstructions", secstrings + s->sh_name))
-			continue;
-		seg = (void *)s->sh_addr; 
-		apply_alternatives(seg, seg + s->sh_size); 
-	} 	
+		if (0 == strcmp(".text", secstrings + s->sh_name))
+			text = s;
+		if (0 == strcmp(".altinstructions", secstrings + s->sh_name))
+			alt = s;
+		if (0 == strcmp(".smp_altinstructions", secstrings + s->sh_name))
+			smpalt = s;
+	}
+
+	if (alt) {
+		/* patch .altinstructions */ 
+		void *aseg = (void *)alt->sh_addr;
+		apply_alternatives(aseg, aseg + alt->sh_size, NULL, NULL);
+	}
+	if (smpalt && text) {
+		void *aseg = (void *)smpalt->sh_addr;
+		void *tseg = (void *)text->sh_addr;
+		alternatives_smp_module_add(me, me->name,
+					    aseg, aseg + smpalt->sh_size,
+					    tseg, tseg + text->sh_size);
+	}
+
 	return 0;
 }
 
 void module_arch_cleanup(struct module *mod)
 {
+	alternatives_smp_module_del(mod);
 }

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/arch/i386/kernel/semaphore.c work-2.6.14/arch/i386/kernel/semaphore.c
@@ -110,11 +110,11 @@
 ".align	4\n"
 ".globl	__write_lock_failed\n"
 "__write_lock_failed:\n\t"
-	LOCK "addl	$" RW_LOCK_BIAS_STR ",(%eax)\n"
+	LOCK_PRE "addl	$" RW_LOCK_BIAS_STR ",(%eax)" LOCK_POST "\n"
 "1:	rep; nop\n\t"
 	"cmpl	$" RW_LOCK_BIAS_STR ",(%eax)\n\t"
 	"jne	1b\n\t"
-	LOCK "subl	$" RW_LOCK_BIAS_STR ",(%eax)\n\t"
+	LOCK_PRE "subl	$" RW_LOCK_BIAS_STR ",(%eax)" LOCK_POST "\n\t"
 	"jnz	__write_lock_failed\n\t"
 	"ret"
 );

@@ -124,11 +124,11 @@
 ".align	4\n"
 ".globl	__read_lock_failed\n"
 "__read_lock_failed:\n\t"
-	LOCK "incl	(%eax)\n"
+	LOCK_PRE "incl	(%eax)" LOCK_POST "\n"
 "1:	rep; nop\n\t"
 	"cmpl	$1,(%eax)\n\t"
 	"js	1b\n\t"
-	LOCK "decl	(%eax)\n\t"
+	LOCK_PRE "decl	(%eax)" LOCK_POST "\n\t"
 	"js	__read_lock_failed\n\t"
 	"ret"
 );

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/arch/i386/kernel/smpboot.c work-2.6.14/arch/i386/kernel/smpboot.c
@@ -874,6 +874,7 @@
 	unsigned short nmi_high = 0, nmi_low = 0;
 
 	++cpucount;
+	switch_alternatives_smp();
 
 	/*
 	 * We can't use kernel_thread since we must avoid to

@@ -1315,6 +1316,9 @@
 	fixup_irqs(map);
 	/* It's now safe to remove this processor from the online map */
 	cpu_clear(cpu, cpu_online_map);
+
+	if (1 == num_online_cpus())
+		switch_alternatives_up();
 	return 0;
 }
 

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/arch/i386/kernel/vmlinux.lds.S work-2.6.14/arch/i386/kernel/vmlinux.lds.S
@@ -68,6 +68,16 @@
 	*(.data.init_task)
   }
 
+  . = ALIGN(4);
+  __smp_alt_instructions = .;
+  .smp_altinstructions : AT(ADDR(.smp_altinstructions) - LOAD_OFFSET) {
+	*(.smp_altinstructions)
+  }
+  __smp_alt_instructions_end = .; 
+  .smp_altinstr_replacement : AT(ADDR(.smp_altinstr_replacement) - LOAD_OFFSET) {
+	*(.smp_altinstr_replacement)
+  }
+
   /* will be freed after init */
   . = ALIGN(4096);		/* Init code and data */
   __init_begin = .;

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/drivers/char/sysrq.c work-2.6.14/drivers/char/sysrq.c
@@ -271,6 +271,34 @@
 	.enable_mask	= SYSRQ_ENABLE_RTNICE,
 };
 
+#ifdef CONFIG_SMP
+/* handy for testing & benchmarking, probably temporary though.
+ *                                       -- kraxel */
+static void sysrq_handle_up(int key, struct pt_regs *pt_regs,
+			    struct tty_struct *tty)
+{
+	switch_alternatives_up();
+}
+static struct sysrq_key_op sysrq_up_op = {
+	.handler	= sysrq_handle_up,
+	.help_msg	= "UP(x)",
+	.action_msg	= "switch smp alternatives to UP",
+	.enable_mask	= SYSRQ_ENABLE_LOG,
+};
+
+static void sysrq_handle_smp(int key, struct pt_regs *pt_regs,
+			    struct tty_struct *tty)
+{
+	switch_alternatives_smp();
+}
+static struct sysrq_key_op sysrq_smp_op = {
+	.handler	= sysrq_handle_smp,
+	.help_msg	= "SMP(y)",
+	.action_msg	= "switch smp alternatives to SMP",
+	.enable_mask	= SYSRQ_ENABLE_LOG,
+};
+#endif
+
 /* Key Operations table and lock */
 static DEFINE_SPINLOCK(sysrq_key_table_lock);
 #define SYSRQ_KEY_TABLE_LENGTH 36

@@ -323,8 +351,13 @@
 /* u */	&sysrq_mountro_op,
 /* v */	NULL, /* May be assigned at init time by SMP VOYAGER */
 /* w */	NULL,
+#ifdef CONFIG_SMP
+/* x */	&sysrq_up_op,
+/* y */	&sysrq_smp_op,
+#else
 /* x */	NULL,
 /* y */	NULL,
+#endif
 /* z */	NULL
 };
 

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/include/asm-i386/cpufeature.h work-2.6.14/include/asm-i386/cpufeature.h
@@ -70,6 +70,8 @@
 #define X86_FEATURE_P3		(3*32+ 6) /* P3 */
 #define X86_FEATURE_P4		(3*32+ 7) /* P4 */
 
+#define X86_FEATURE_UP		(3*32+ 8) /* smp kernel running on up */
+
 /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
 #define X86_FEATURE_XMM3	(4*32+ 0) /* Streaming SIMD Extensions-3 */
 #define X86_FEATURE_MWAIT	(4*32+ 3) /* Monitor/Mwait support */

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/include/asm-i386/futex.h work-2.6.14/include/asm-i386/futex.h
@@ -28,7 +28,7 @@
 "1:	movl	%2, %0\n\
 	movl	%0, %3\n"					\
 	insn "\n"						\
-"2:	" LOCK_PREFIX "cmpxchgl %3, %2\n\
+"2:	" LOCK_PRE "cmpxchgl %3, %2" LOCK_POST "\n\
 	jnz	1b\n\
 3:	.section .fixup,\"ax\"\n\
 4:	mov	%5, %1\n\

@@ -68,7 +68,7 @@
 #endif
 		switch (op) {
 		case FUTEX_OP_ADD:
-			__futex_atomic_op1(LOCK_PREFIX "xaddl %0, %2", ret,
+			__futex_atomic_op1(LOCK_PRE "xaddl %0, %2" LOCK_POST, ret,
 					   oldval, uaddr, oparg);
 			break;
 		case FUTEX_OP_OR:

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/include/asm-i386/rwsem.h work-2.6.14/include/asm-i386/rwsem.h
@@ -99,7 +99,7 @@
 {
 	__asm__ __volatile__(
 		"# beginning down_read\n\t"
-LOCK_PREFIX	"  incl      (%%eax)\n\t" /* adds 0x00000001, returns the old value */
+LOCK_PRE       	"  incl      (%%eax)" LOCK_POST "\n\t" /* adds 0x00000001, returns the old value */
 		"  js        2f\n\t" /* jump if we weren't granted the lock */
 		"1:\n\t"
 		LOCK_SECTION_START("")

@@ -130,7 +130,7 @@
 		"  movl	     %1,%2\n\t"
 		"  addl      %3,%2\n\t"
 		"  jle	     2f\n\t"
-LOCK_PREFIX	"  cmpxchgl  %2,%0\n\t"
+LOCK_PRE       	"  cmpxchgl  %2,%0" LOCK_POST "\n\t"
 		"  jnz	     1b\n\t"
 		"2:\n\t"
 		"# ending __down_read_trylock\n\t"

@@ -150,7 +150,7 @@
 	tmp = RWSEM_ACTIVE_WRITE_BIAS;
 	__asm__ __volatile__(
 		"# beginning down_write\n\t"
-LOCK_PREFIX	"  xadd      %%edx,(%%eax)\n\t" /* subtract 0x0000ffff, returns the old value */
+LOCK_PRE       	"  xadd      %%edx,(%%eax)" LOCK_POST "\n\t" /* subtract 0x0000ffff, returns the old value */
 		"  testl     %%edx,%%edx\n\t" /* was the count 0 before? */
 		"  jnz       2f\n\t" /* jump if we weren't granted the lock */
 		"1:\n\t"

@@ -188,7 +188,7 @@
 	__s32 tmp = -RWSEM_ACTIVE_READ_BIAS;
 	__asm__ __volatile__(
 		"# beginning __up_read\n\t"
-LOCK_PREFIX	"  xadd      %%edx,(%%eax)\n\t" /* subtracts 1, returns the old value */
+LOCK_PRE	"  xadd      %%edx,(%%eax)" LOCK_POST "\n\t" /* subtracts 1, returns the old value */
 		"  js        2f\n\t" /* jump if the lock is being waited upon */
 		"1:\n\t"
 		LOCK_SECTION_START("")

@@ -214,7 +214,7 @@
 	__asm__ __volatile__(
 		"# beginning __up_write\n\t"
 		"  movl      %2,%%edx\n\t"
-LOCK_PREFIX	"  xaddl     %%edx,(%%eax)\n\t" /* tries to transition 0xffff0001 -> 0x00000000 */
+LOCK_PRE       	"  xaddl     %%edx,(%%eax)" LOCK_POST "\n\t" /* tries to transition 0xffff0001 -> 0x00000000 */
 		"  jnz       2f\n\t" /* jump if the lock is being waited upon */
 		"1:\n\t"
 		LOCK_SECTION_START("")

@@ -239,7 +239,7 @@
 {
 	__asm__ __volatile__(
 		"# beginning __downgrade_write\n\t"
-LOCK_PREFIX	"  addl      %2,(%%eax)\n\t" /* transitions 0xZZZZ0001 -> 0xYYYY0001 */
+LOCK_PRE	"  addl      %2,(%%eax)" LOCK_POST "\n\t" /* transitions 0xZZZZ0001 -> 0xYYYY0001 */
 		"  js        2f\n\t" /* jump if the lock is being waited upon */
 		"1:\n\t"
 		LOCK_SECTION_START("")

@@ -262,9 +262,9 @@
  */
 static inline void rwsem_atomic_add(int delta, struct rw_semaphore *sem)
 {
-	__asm__ __volatile__(
-LOCK_PREFIX	"addl %1,%0"
-		: "=m"(sem->count)
+	alternative_smp_lock(
+		"addl %1,%0",
+		"=m"(sem->count)
 		: "ir"(delta), "m"(sem->count));
 }
 

@@ -275,9 +275,9 @@
 {
 	int tmp = delta;
 
-	__asm__ __volatile__(
-LOCK_PREFIX	"xadd %0,(%2)"
-		: "+r"(tmp), "=m"(sem->count)
+	alternative_smp_lock(
+		"xadd %0,(%2)",
+		"+r"(tmp), "=m"(sem->count)
 		: "r"(sem), "m"(sem->count)
 		: "memory");
 

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/include/asm-i386/semaphore.h work-2.6.14/include/asm-i386/semaphore.h
@@ -102,7 +102,7 @@
 	might_sleep();
 	__asm__ __volatile__(
 		"# atomic down operation\n\t"
-		LOCK "decl %0\n\t"     /* --sem->count */
+		LOCK_PRE "decl %0" LOCK_POST "\n\t"     /* --sem->count */
 		"js 2f\n"
 		"1:\n"
 		LOCK_SECTION_START("")

@@ -126,7 +126,7 @@
 	might_sleep();
 	__asm__ __volatile__(
 		"# atomic interruptible down operation\n\t"
-		LOCK "decl %1\n\t"     /* --sem->count */
+		LOCK_PRE "decl %1" LOCK_POST "\n\t"     /* --sem->count */
 		"js 2f\n\t"
 		"xorl %0,%0\n"
 		"1:\n"

@@ -151,7 +151,7 @@
 
 	__asm__ __volatile__(
 		"# atomic interruptible down operation\n\t"
-		LOCK "decl %1\n\t"     /* --sem->count */
+		LOCK_PRE "decl %1" LOCK_POST "\n\t"     /* --sem->count */
 		"js 2f\n\t"
 		"xorl %0,%0\n"
 		"1:\n"

@@ -176,7 +176,7 @@
 {
 	__asm__ __volatile__(
 		"# atomic up operation\n\t"
-		LOCK "incl %0\n\t"     /* ++sem->count */
+		LOCK_PRE "incl %0" LOCK_POST "\n\t"     /* ++sem->count */
 		"jle 2f\n"
 		"1:\n"
 		LOCK_SECTION_START("")

diff -urN -x 'build-*' -x '*~' -x Make -x scripts linux-2.6.14/include/asm-i386/spinlock.h work-2.6.14/include/asm-i386/spinlock.h
@@ -48,18 +48,23 @@
 	"jmp 1b\n" \
 	"4:\n\t"
 
+#define __raw_spin_lock_string_up \
+	"\n\tdecb %0"
+
 static inline void __raw_spin_lock(raw_spinlock_t *lock)
 {
-	__asm__ __volatile__(
-		__raw_spin_lock_string
-		:"=m" (lock->slock) : : "memory");
+	alternative_smp(
+		__raw_spin_lock_string,
+		__raw_spin_lock_string_up,
+		"=m" (lock->slock) : : "memory");
 }
 
 static inline void __raw_spin_lock_flags(raw_spinlock_t *lock, unsigned long flags)
 {
-	__asm__ __volatile__(
-		__raw_spin_lock_string_flags
-		:"=m" (lock->slock) : "r" (flags) : "memory");
+	alternative_smp(
+		__raw_spin_lock_string_flags,
+		__raw_spin_lock_string_up,
+		"=m" (lock->slock) : "r" (flags) : "memory");
 }
 
 static inline int __raw_spin_trylock(raw_spinlock_t *lock)

@@ -178,13 +183,16 @@
 
 static inline void __raw_read_unlock(raw_rwlock_t *rw)
 {
-	asm volatile("lock ; incl %0" :"=m" (rw->lock) : : "memory");
+	alternative_smp_lock(
+		"incl %0",
+		"=m" (rw->lock) : : "memory");
 }
 
 static inline void __raw_write_unlock(raw_rwlock_t *rw)
 {
-	asm volatile("lock ; addl $" RW_LOCK_BIAS_STR ", %0"
-				 : "=m" (rw->lock) : : "memory");
+	alternative_smp_lock(
+		"addl $" RW_LOCK_BIAS_STR ", %0",
+		"=m" (rw->lock) : : "memory");
 }
 
 #endif /* __ASM_SPINLOCK_H */

Hi!

Thanks, Sharp!

This looks like total overkill to me. Who needs to optimize CPU hotplug this way? If you really need this just do it at boot time with the existing mechanisms. This would keep it much simpler and simplicity is very important with such code because otherwise the testing of all the corner cases will kill you.

BTW the existing mechanism already works fine for modules too.

Sure, for real hardware doing that at boot time is be perfectly fine. In a virtual environment it's very useful to be able to plug in one more virtual CPU on demand without rebooting though. The patch isn't very useful alone, it's more one step on the road of getting the xen bits merged mainline.

Ok, dropped. I've just copyed that from the original, pretty ugly xen patch.

cheers,

Gerd

If you are patching code another x86 CPU is running you must halt the other processors and ensure it executes a serialzing instruction before it enters any patched code.

How many kilobytes of tables do you add to the kernel to do this pointless stunt btw ?

Alan "CPU errata are fun" Cox

Yes that is why the original alternative() mechanism always only runs before the code is ever executed.

I much prefer the MSR bit too. Unfortunately it doesn't exist (or rather I bet it exists somewhere, just undocumented) on Intel systems.

-Andi

The MSR bits will break things like ECC scrubbing however. That can be addressed although the test patch I have just refuses to load EDAC if the BIOS writers didn't follow the BIOS guidelines.

Certainly it would be cleaner and easier to save the MSR, scrub and put it back than do the fixup magic. Some drivers would need auditing as they seem to use locked ops or xchg (implicit lock) to lock with a PCI DMA master.

You mean it might break an insane hack in someone's ECC scrubbing implementation. But last time I talked to people about this they suggested just using an uncacheable mapping instead of this horrible thing. Uncached is actually what you want there, not relying on some undocumented lock bus cycle behaviour.

IMHO that would be much better and actually have a chance of working over multiple generation.

Which drivers? I don't think there is anything in tree. I went over all the drivers early in the x86-64 port.

I'm sure I would have noticed because they very likely needed inline assembly for this and this generally broke when moving to x86-64.

DRM did some tricks, but generally not with the hardware I believe, only between user/kernel space.

-Andi

I reviewed your suggestions before:

1. The lock behaviour *is* defined for main memory access by all bus masters.
2. Uncached mappings are unworkable for this because we must never have a page mapped with conflicting cache types - thats ugly, and plain horrific on SMP.
3. Uncached has undefined semantics when racing a PCI master. Lock has defined semantics. An uncached add #0 is permitted to read the memory and then write it back as two different cycles and I suspect does.
4. The AMD BIOS guide requires both that LOCK is enabled by default and that the "lock affects the external bus" bit is clear to enable locking on the external bus.

The problems we would have would be if some smartarse chip designed optimised lock addl #0 to a no-op and the fact we possibly ought to wbinvd the page and read it again to check the ECC recovery worked.

eepro100 used to from memory but it now carefully does a 16bit I/O.

Or use xchg() which is implied lock on x86 so isn't magically made non-atomic by the LOCK macros. I did a sweep for xchg and didn't see any problem.

It does it extensively for user/kernel. Whether it does it with the GPU in user space I can't say. The cards I am familar with do not.

Alan

For uncached memory, right?

For kernel mapping change_page_attr() takes care of it, and for user space memory following all mappings is the only reliable way to find out which process needs to be killed anyways - and when you do that you can as well unmap or just kill.

Consider what happens with such a race: either the PCI master gets an bus abort because it still sees the corrupted data. Or it already accesses the repaired data. Both is ok.

The "Linux guidelines" might be different.

-Andi

I think I see the source of the confusion. Scrubbing is the process of taking data that is correctable and writing it back to memory so that if a second correctable error occurs the net is still corrected.

Directed killing of processes is something that must be done inside a synchronous exception (like a machine check) because otherwise it is so racy you don't know who has seen the bad data.

Eric

That's supposed to be done by hardware, no?
At least the K8 has a hardware scrubber (although it's not always enabled)

If you try to do it this way then the code will become such
a mess if not impossible to write that your changes to merge them and get it right are very slim. The only sane way to do all the locking etc. is to hand over the handling to a thread. While that make the window of misusing the data wider it's the only sane alternative vs not doing it at all.

Also due to the way hardware works with machine checks usually being async and not precise works you have that window anyways, so it's not even worse. Also consider multiple CPUs.

-Andi

Recent good implementations like the Opteron will do it for you. Older or cheaper memory controllers will not.

Having an architecturally sane software scrubber as backup for the hardware implementations is nice, and except in the cases where someone disables the lock prefix it is takes very little code on x86.

Even on the Opteron you could theoretically have the case of a brain-dead external memory controller, although that is not likely.

First I don't have any code to do this, but I have though about it. The races are the primary reason I have never pushed for something like this. With memory errors coming in as machine checks it is now possible to do a correct version.

Essentially we are talking something with the complexity of a page fault. All that must happen synchronously is the task must be stopped, and flagged.

As for races every cpu that accesses that data should take a synchronous exception. DMA should do something similar but I'm not as familiar with that side of the problem. And because everything takes an exception multiple cpu races are not a problem.

Of course there are still the memory errors that are so bad that they don't even cause a machine check. Those are a real pain to debug, and fix.

In this latter case assuming the memory error is transient and not hard using a write-combine memory attribute when you write to re-initialize the ECC state is the way to go. But remember you most do it on the cpu that is part of the memory controller. Otherwise something in the whole read/modify process will fail to get the ECC state initialized properly on an Opteron.

Eric

Beware of errata - there's at leats one errata on Opteron which forces you to choose between x4 (chipkill) ECC and scrubber. One or the other, but not both. There are plenty of errata on the scrubber alone. Worse, if my (brain)memory is correct, without the scrubber, correctable errors are corrected on the fly, but never written back to DRAM.

All the scrub errata were fixed with E stepping AFAIK.

You have a point that using a sw scrubber might make sense on earlier steppings though in case someone really wants chipkill.

-Andi

Varies immensely by system. Where there is a hardware scrubber and it is enabled it will be used. Once nice thing about K8 is the mem controller is in the CPU so they all use the same driver (not yet merged)

Its utterly hideous because the usual 'ECC error' reporting technique for an uncorrectable error is an NMI. Locks could be in any state at this point and even the registers needing to be accessed are across PCI and we could be half way through a PCI configuration cycle.

The -mm EDAC code works on the basic assumption that unrecovered ECC is a system halter although that is configurable.

What do you need a special driver for if the northbridge just can do the scrubbing by itself?

On the modern systems I'm familiar with it's an machine check (although not necessarily a recoverable one and there might be other bad side effects)

I don't know what you could do over the default code for K8 at least. And on modern Intel server chipsets I would expect it also to not be needed.

-Andi

You need a driver to collect and report all the ECC single bit errors to the user so that they can decide if they have problem hardware.

EDAC is more than one thing
- Control response to a fatal error
- Report non-fatal events for analysis/user decision making

Hardware scrubbing is good, but knowing the rate of non-fatal errors and the trend in rate of errors is essential to planning and management of systems.

The Intel I have looked at generates MCE if the L2/L1/bus parity errors but not on a RAM ECC error as that is memory controller not CPU level. That usually asserts NMI. Same for most older chips PIII/AMD Athlon etc

Varies a lot again. Hopefully that'll simplify as/when/if Intel put the memory controller on CPU.

Alan

Assuming the errors are logged to the standard machine check architecture that's already done by mce.c. K8 does that definitely.

Take a look at mcelog at some point.
Your distro probably already sets it up by default to log to /var/log/mcelog

x86-64 mce.c does all that There was even a port to i386 around at some point.

-Andi

Right on the k8 memory controller there is a lot of overlap, with what has already been implemented. For all other x86 memory controllers the code is filling a large void. The current k8 code has been delayed for this reason.

Where the EDAC code goes beyond the current k8 facilities is the decode to the dimm level so that the bad memory stick can be easily identified.

One of the goals of the EDAC code is to work towards a unified kernel architecture for this kind error reporting. Currently every architecture (if the error are reported at all) handles this differently which makes it very hard to do something sane is user space.

Eric

And also in finding/recording PCI parity errors (which will link nicely into the IBM work for code to handle reported PCI errors).

At least the Lindenhurst (E7205) datasheet says the chipset can trigger MCEs in the CPU (using a MCEERR# pin). I don't know if it's always enabled, but the hardware seems to have the capability. That's the oldest Intel server chipset supported with EM64T CPUs.

The threshold counters are not supported directly only.

That would be nice to have agreed. But I don't really know how to do this without mainboard specific knowledge. If you have something usable it's best to port it to mce.c or perhaps mcelog

There is a clear case for being architecture specific here. Some architectures - like PPC64 or IA64 - have good firmware support for it, so it's best to use these facilities. On others like i386 and
x86-64 the x86-64 log architecture is good. I might be a bit biased but I think it's very good and should be used on i386
at some point too. I don't see any need for more.

-Andi

I don't think that triggers on correctable errors, and I'm not certain how useful the information reported it. But it should be at least as good as an NMI :)

Truthfully it really isn't just server chipsets that are interesting either. Anything that supports ECC or parity on memory is interesting.

We do this for every memory controller EDAC supports, so yes we know how to implement this. Merging the non-controversial bits is coming. But it is certainly a goal to take the best of the mce code and the EDAC code to generate a good k8 driver.

Motherboard specific knowledge is really not required. All that is really required is memory controller specific knowledge. With that you can decode to the chip select level on most memory controllers.

Then you need just a little extra code (probably in user space) to map the chip select to which dimm socket they go to on the motherboard.

The memory controller knowledge pretty much needs to be a kernel driver because reading the memory controller registers can be a non-trivial exercise at times. At least one piece of Intel seems to like recommending that BIOS developers turn off the PCI device that has the registers.

The implementation clearly should be architecture specific, and it will take more feeling out before any work can be done to even think about unify the interfaces.

Right now the goal is to simply get what has proven useful in the real world merged.

Currently I do not see implementation problems but I do see facilities not being as useful as they could be. Getting little things like decoding to the dimm sorted out removes hours of work figuring out which dimm has problems.

I also see a large number of errors that the hardware can detect that are going unreported because the code just isn't there. There are all kinds of bus errors that chipsets can report that are just being ignored.

So now you know some of the hopes and dreams behind some of the EDAC code and some more of the model. Hopefully this will lead to productive conversations as the code is merged.

Eric

I'm curious about that too. Even with k8 you can get down to a chip-select, but that doesn't necessarily map to a DIMM in any useful way, unless you have some mobo knowledge. Are we going to need a new BIOS table to map chip-selects onto DIMMs? :)

Yeah that's my problem.

It's not theoretical. We had cases where someone had to go through 10+ DIMMs on a big machine in try and error to find out which one is wrong. Very bad situation.

[Double plus bad if it wasn't actually any of the DIMMs that were bad, but one of the VRMs on a big Opteron - it causes all the same symptoms as a bad DIMM :/]

I proposed something like that - best with an ASCII string ("First DIMM on the top left corner") But getting such stuff into BIOS is difficult and long winded.

-Andi

I have the exact same problem right now. Part of our early bootup we run a simplish memory test. Basically it's a "can the memory hold state" test. If anything fails, we have to identify as exactly as possible WHICH DIMM needs to be replaced, so the hardware ops people can do it at assembly/test time.

We implemented AMD's reference algorithm, and made it work in the presence of a hardware IO hole. It seems to work beautifully, but the last step is turning a (node:chip-select) into a (node:dimm). Simple boards will use simple mappings, but we can't know that without board specific info. Especially with quad-rank DIMMs. :)

It would be easy enough to get into LinuxBIOS. :)

Seriously, this is work that is *long* overdue. I have been wanting to look at this for over a year, but I have not had time.

Doing proper architecture and chipset-specific ECC/error handling which ties into a bigger abstracted error system is going to be really nice.

If you get something working it would be good if you could share the code (even if it still needs to be tweaked)

IMNSHO the x86-64 mce.c with its error log is a reasonable start. All the smarts can be in user space and in mcelog.c. DIMM decoding is a special case though because the information is really useful to be printed onto the screen for fatal MCEs. So that one is better in kernel space.

-Andi

The below code works for us. Note that I did not implement the node-interleaving parts of the AMD algorithm. If that matters, it should be simple enough to do. The BKDG has good pseudo-code. The only thing it gets absolutely wrong is the IO hole.

Let me know if you see any problems here. This is a userspace tool, but should be trivial to adapt.

#include <stdio.h>
#include <stdlib.h>
#include "pci.h"

#define MAX_NODES 8
#define MAX_CS 8
#define NODE_DEV(n) (24+(n))
#define FOUR_GIG 0x01000000 // shifted to hold address[39..8]

static char *progname;

static void
usage(void)

{
	fprintf(stderr, "usage: %s <address>\n", progname);
}

/*
* Map a CS to a pair of DIMM slots (for 128 bit operation). This mapping
* is board-specific, and has the potential to be very ugly. */

static int cs_to_pair[MAX_CS][2] = {
	{ 0, 1 }, { 0, 1 },
	{ 2, 3 }, { 2, 3 },
	{ 4, 5 }, { 4, 5 },
	{ 6, 7 }, { 6, 7 },
};

int
main(int argc, char *argv[])

{
	unsigned long long raw_addr;
	uint32_t address;
	char *endp;
	int node;

	progname = argv[0];

	if (argc != 2) {
		usage();
		exit(1);
	}
	raw_addr = strtoull(argv[1], &endp, 0);
	if (endp == argv[1]) {
		usage();
		exit(2);
	}

	/*
	 * The address space is 40 bits (for now).  We want to use 32 bit
	 * values, so we convert the input address to hold address[39..8].
	 * We'll use this format everywhere.  This loses the
	 * low-order bits, so we keep a raw copy around.
	 */
	address = (raw_addr & 0xffffffffff) >> 8;

	/* find the node that holds this address */
	for (node = 0; node < MAX_NODES; node++) {
		int pci_dev;
		uint32_t tmp;
		int dram_en;
		uint32_t dram_base;
		uint32_t dram_limit;
		int hole_en;
		uint32_t hole_base;
		uint32_t hole_size;
		int cs;

		/*
		 * The DRAM map and IO hole info are in function 1 of each
		 * node.
		 */
		pci_dev = pci_open(0, NODE_DEV(node), 1);
		if (pci_dev < 0) {
			/* node does not exist */
			break;
		}

		/*
		 * DRAM_BASE and DRAM_LIMIT already hold address[39..8].
		 */
		tmp = pci_read32(pci_dev, 0x40+(node*8));
		dram_en = tmp & 0x3;
		dram_base = tmp & 0xffff0000;

		tmp = pci_read32(pci_dev, 0x44+(node*8));
		dram_limit = tmp | 0x0000ffff;

		/*
		 * HOLE_BASE holds address[35..4], so we convert it to hold
		 * address[39..8].
		 */
		tmp = pci_read32(pci_dev, 0xf0);
		hole_en = tmp & 0x1;
		hole_base = (tmp & 0xff000000) >> 8;
		hole_size = FOUR_GIG - hole_base;

		pci_close(pci_dev);

		if (!dram_en) {
			/* no DRAM here */
			continue;
		}

		if (address > dram_limit) {
			/* keep looking */
			continue;
		}

		/*
		 * The address must be on this node.
		 */

		/* is the address in the IO hole? */
		if (hole_en && address >= hole_base && address < FOUR_GIG) {
			/* no DRAM in the IO hole */
			break;
		}

		/* is the address >= 4GB on a node with a hole? */
		if (hole_en && address >= FOUR_GIG) {
			/* adjust address for the IO hole */
			address -= hole_size;
		}

		/* adjust address to be node-relative */
		address -= dram_base;

		/* store addr[35..4] */
		address <<= 4;

		/* The chip-select map is in function 2 of each node. */
		pci_dev = pci_open(0, NODE_DEV(node), 2);

		/* find the chip-select that has this address */
		for (cs = 0; cs < MAX_CS; cs++) {
			uint32_t cs_base;
			uint32_t cs_mask;

			/*
			 * CS_BASE and CS_MASK hold address[35..0], so we
			 * convert them to hold address[39..8].
			 */
			tmp = pci_read32(pci_dev, 0x40+(cs*4));
			if ((tmp & 0x1) == 0) {
				/* this CS is not enabled */
				continue;
			}
			cs_base = tmp & 0xffe0fe00;

			tmp = pci_read32(pci_dev, 0x60+(cs*4));
			cs_mask = (tmp | 0x001f01ff) & 0x3fffffff;

			/* this should handle interleaving, too */
			if ((address & ~cs_mask) == (cs_base & ~cs_mask)) {
				int *pair = cs_to_pair[cs];
				int dimm;

				/* which DIMM is this address on? */
				if ((raw_addr & (1<<3)) == 0) {
					dimm = pair[0];
				} else {
					dimm = pair[1];
				}

				printf("node %d, CS %d, DIMM %d\n",
				       node, cs, dimm);
				break;
			}
		}
		pci_close(pci_dev);
		break;
	}
	return 0;
}

Thanks. But without a per board DIMM mapping it's pretty useless, isn't it?

One could detect the IO hole by reading the IORR MSRs or alternatively parsing the e820 map in /var/log/boot.msg

-Andi

Exactly.

Why bother? In this process - turning a physical address into a DIMM, you're poking at all the data anyway, just get the IO hole straight from the chipset.

Nope.

Getting a per board chip select to DIMM mapping is fairly easy, you just need a lookup table of:
(memory_controller, chip_select, channel, dimm_label)

Which if the motherboard vendor does not give it to you is pretty straight forward to discover just by plugging a minimal memory configuration into various slots. We can already query in software what the motherboard is so keeping a table like this in user space is
not a problem.

The problem is not detection but compensating for how it changes the address.

I do agree that it would be nice if there was a standard for BIOS's reporting this information. In LinuxBIOS it is one of those TODO list items we never quite get to. But so far a user space table has proved quite useful in practice.

Eric

Propose it the desktop management people and get it into the DMI standard. They already have entries for each memory slot, they already have entries for descriptive strings for connectors. In fact you may well be able to 'bend' the spec enough to do it as is.

There are enough fields that maybe one of them is loose enough to mean this. It doesn't help us convince mobo vendors to support it, though.

With arbitary desktop/laptop/etc. vendors it's pretty hopeless I agree. But I suspect there is a chance at least on the server side. There is only a limited number of companies working on server BIOSes for their boards and they tend to be more receptive to Linux's need because it's now a significant part of their market.

And it's clearly an obviously useful "RAS feature" which is fully buzzword compatible and everything.

IMHO it's time that Linux gets more proactive regarding talking to BIOS vendors. Perhaps a generic "BIOS writers guide for Linux" would be a good thing. I have at least one other extension I would like BIOS vendors to support. Just would need to come up with a writeup for a clearly defined specification.

-Andi

I wrote one for 2.2 era but it never got updated. I've probably still got it around somewhere.

BIOS, and hardware. I think Alan wrote something up way long ago, but it hasn't really been updated.

-hpa

It would seem that the OEMs buying the board would like this feature, since it could be incorporated into POST, diagnostic CDs, etc. And since server owners are more likely to have a service contract, anything to make service calls faster is a benefit to the system vendor.

If someone handed them some good specs except for the table, I suspect they would see the benefit. Independent BIOS writers compete for board contracts, in-house writers want features with one time cost and every time benefit, I think you're right that this would be a benefit to everyone.

Given that it seems so simple, is there a reason why this hasn't been around for ages?

   -bill davidsen ([email protected])
"The secret to procrastination is to put things off until the
 last possible moment - but no longer"  -me

Some BIOSes route that into SMI. The BIOS can then log the error and tell the OS via the nmi_now bit. Uggh.

For all memory. Same as processor to processor.

You are working from a kernel view address of a page that may be user space. You don't need or want to kill anything because you are scrubbing a corrected error.

This is a correctable error so there would be no abort. And there is a race if you think for a microsecond or two

Scrubber reads (add #0 load of input)
Bus master writes #FFFFFFFF
Scrubber writes back #0

The lock prefix ensures that doesn't occur.

Then the EDAC code will reconfigure the registers back as expected by the PC architecture. I've no problem with that and if EDAC is the only person requiring a semantic that is more expensive it can flip the bits back and forth. No need for anyone else to pay that cost.

Alan

What I suggested to Intel at the Developer Days is to have a MSR (or, better yet, a bit in the page table pointer %cr0) that disables "lock" in _user_ space. Ie a lock would be a no-op when in CPL3, and only with certain processes.

The kernel really isn't that critical. We always need the locks in SMP (unlike user space, which never needs them if the process isn't threaded), and in the kernel space we occasionally need it even with UP to protect against devices. And we _can_ do these instruction rewrites, and they are even pretty trivial for the non-hotplug case.

User space is actually a lot more important. People spend more time in user space, and there the lock prefix is much more often totally useless and cannot just be edited away once per boot.

Linus

You mean %cr3, right?

-hpa

Yes.

It _should_ be fairly easy to do something like that - just a simple global flag that gets set and makes CPL3 ignore lock prefixes. Even timing doesn't matter - it it takes a hundred cycles for the setting to take effect, we don't care, since you can't write %cr3 from user space anyway, and it will certainly take a hundred cycles (and a few serializing instructions) until we get to CPL3.

I'd personally prefer it to be in %cr3, since we'd have to reload it on task switching, and that's one of the registers we load anyway. And it would make sense. But it could be in an MSR too.

Of course, if it's in one of the low 12 bits of %cr3, there would have to be a "enable this bit" in %cr4 or something. Historically, you could write any crap in the low bits, I think.

Linus

Another bit for ring 0 would be actually useful too. Then the patching patch here wouldn't be needed.

-Andi

The lock prefix '0F' is used for a lot of opcodes other than "lock". Go check the instruction set reference. It's not trivial what you are proposing. Intel has a pretty hacked up opcode map with a lot of history. The bit should be in CR4 and not CR3.

J

No it's not.

0F is indeed the two-byte prefix. But lock is F0, and it's unique.

Sometimes Intel re-uses the prefixes for other things eg "rep nop", but I don't think that has ever happened for the lock prefix.

Besides, the instructions look very different internally in the CPU after decoding, and anyway you'd not want to ignore the lock prefix _early_ at decode time anyway (many instructions turn into illegal instructions with a lock prefix, as do reg-reg modrm bytes). So you'd dismiss the lock prefix not at a byte level, but at a minimum just after the decode stage.

Linus

I always get numbers and words transposed. Thanks for the correction.

J

No, most of them are RAZ, but there are at least a couple of bits which have effect (e.g. caching of the page tables.)

However, with PAE there aren't really a whole lot of unused bits in CR3.

-hpa

Changing CR3 will break compatibility with Windows and interfere with Intel's Bread and Butter gravy Train with M$. CR4 was created to deal with some of the legacy issues with backward compatiblity with OS's that read CR3. Messing with CR3 will break Windows.

They won't do anything that will upset the apple cart with M$. I dealt with
Intel folks for years when Linux was unknown. They look and act like boy scouts-- don't be fooled -- they're totally an M$ shop, always have been, always will be. Linux was and is an intresting brain fart on their radar. Their interests in it are solely based on their internal "Rabbits and Dogs" Andy Grove mentality. They say there are rabbits and dogs. Rabbits run out front, dogs chase the rabbits. Intel does business with rabbits. Linux is a dog -- it chases after innovators and replicates their work. The fact it's free is the basis of their interest.

Those interests do not extend to anything that interferes with their M$ relationship. Push for CR4, they might agree, but be assured your request will pass Balmers desk before it gets approved.

J

There is a much much better way to do it than just user space and without hitting cr3/cr4 - put "lock works" in the PAT and while we'll have to add PAT support which we need to do anyway we would get a world where on uniprocessor lock prefix only works on addresse targets we want it to - ie pci_alloc_consistent() pages.

Alan

The idea was to turn LOCK on only if the process has any shared writable mapping and num_online_cpus() > 0.

Might be a bit costly to rewrite all the page tables for that case just to change the PAT index. A bit is nicer for that.

-Andi

That makes a lot of sense, and if we hit hardware that does funky stuff then the driver can set a 'vma needs lock' bit for the same effect.

CPU insert/remove is performed how many times a second ? Or for that matter why not just reload the PAT register and keep the index the same ?

For user space the primary trigger event would be "has any shared writable mappings or multiple threads". Even on a real MP systems it's perfectly ok to run a program with no writable shared mappings with LOCK off. Depending on the workload this transistion could happen quite often. Especially there is a worst case of an application allocating a few GB of memory and then starting a new thread.

-Andi

you also want this for single threaded apps, so that the glibc locking stuff can not do lock for single-threaded apps and non-shared memory

It's not about CPU insert/remove.

It's about a single-threaded process becoming multi-threaded, ie a simple "clone()" operation (or doing a shared mmap).

So it needs to be _fast_.

I would strongly argue that it's not a TLB/PAT operation at all. It has nothing to do with the address of the operation. It's a global bit, and it's in the cr3 just because that's what gets reloaded on task switching. But it could be in the CS register too, for all I care..

Linus

Yep. Though I presume you mean "> 1".

One hopes that num_online_cpus() never reaches zero during runtime ;-)

Jeff

Yeah, > 1 of course.
-Andi

Hi!

Actually num_online_cpus() is very usefull -- suspend to RAM ;-))))).

Pavel

Thanks, Sharp!

No. That would be wrong.

The thing is, "lock" is useless EVEN ON SMP in user space 99% of the time.

Think of all the thread locking in libc - where 99% of all processes are single-threaded, and it does nothing but slow things down.

Actual UP machines are going to go away - even ARM is going SMP, and in the PC space, we'll have multi-core laptops probably being the rule rather than the exception in a couple of years. So the kernel will need "lock" in the forseeable future, and optimizing for UP is a lost game.

But optimizing for a single _thread_ is not a lost game. I don't believe that threaded applications are necessarily going to take over all that much in a lot of areas. Sure, we'll have more threaded apps too, but we'll continue to have tons more of performance-critical non-threaded things like compilers etc.

And _that_ is worth optimizing for. General libraries that have to be able to handle the threaded case dynamically, but that are often run with no shared memory anywhere.

THAT is what I'd like to have CPU support for. Not for UP (it's going away), and not for the kernel (it's never single-threaded).

Linus

There is one reasonly interesting special case that will probably stay
around: single CPU guest in a virtualized environment.

-Andi

.. and then the _virtualizer_ should just set the bit.

However, quite frankly, virtualization is overhyped, in my opinion. And if it forces people to run UP because of performance issues, it's simply not acceptable for a lot of loads.

It's cool technology and all, but realistically..

Linus

That wouldn't work if it's limited limited to ring 3.

Also currently at least the Xen the driver interfaces seem to rely on lock, but perhaps that can be changed.

I don't think it'll force them to that, it will just be a common use case. e.g. you start a separate VM to run your firewall in. Do you really need it multithreaded?

-Andi

Well, with VTX or Pacifica virtualization is in ring 3. The fact that Xen isn't is a workaround for current hardware, so when we're talking about new hardware it's pointless.

What you really want is one bit for kernel mode (cpl 0-2) and one for user mode (cpl 3).

-hpa

Not it's not. The whole point is that there is no "ring compression" The guest has all its normal rings, just the hypervisor has additional "negative" rings.

In the current Xen x86-64 para virtualization setup the guest kernel is in ring 3, but I hope VT/P. will do away with that because it causes lots of issues.

Yes.

-Andi

Uhm... maybe we think of it differently, but typically I consider the host rings (which is what I talked about above) as orthogonal to the guest ring. To the host, the guest is just a process in ring 3.

-hpa

I don't think your thoughts match the terminology as used by Intel/AMD/Xen at least.

-Andi

Perhaps not. The Intel terminology seems really confused, especially.

-hpa

The question is: do you need a virtualized environment for it.

And the answer is: no.

I realize that you can make up an infinite number of things you _can_ use virtualization for. That doesn't mean that people _will_.

Linus

There will continue to be tons of embedded uniprocessor Linux use...

It will take a while for Linux phones and watches to become multi-core.

Jeff

I don't think I see the point. This would let you optimize for the "multi-threaded, but hasn't created any threads yet" or even "multi-threaded, but not right now" cases. But those really aren't the interesting case to optimize for - that's the equivalent of supporting CPU hotplug.

The interesting case is when you know at static link time that the library is single-threaded, or even at dynamic link time. And it's easy enough at both of those times to handle this. In many cases glibc doesn't, because it's valid to dlopen libpthread.so, but that could be accomodated - a simple matter of software.

Daniel Jacobowitz
CodeSourcery, LLC

No, you can never know that unless you can't call mmap().

-hpa

Please explain what problem you see. If you use mmap to manually load libpthread.so, and patch up its relocations without going to ld.so, obviously you get to keep both pieces. Or are you talking about synchronizing access to shared mmaped buffers?

This is different from what Linus was talking about precisely because we can do it imperatively ("I know this program is single-threaded and I'm telling you so" instead of "Hmm, this program hasn't called clone yet").

It's not as technologically slick but I'd need a lot of convincing to believe it wasn't just as useful; and it has the benefit of not requiring new silicon.

Daniel Jacobowitz
CodeSourcery, LLC

Yes. Any shared mmaps may require working lock.

-hpa

Not "any". Only writable shared mmap. Which is actually the rare case.

Even then, we might want to have such processes have a way to say "I don't do futexes in this mmap" or similar. Quite often, writable shared mmaps aren't interested in locked cycles - they are there to just write things to disk, and all the serialization is done in the kernel when the user does a "munmap()" or a "msync()".

Linus

In fact for being explict we already have PROT_SEM on some architectures to report if we are going to use atomic operations, in the mmap. For x86 we would probably need to introduce a PROT_NOSEM but it is sounds fairly straight forward to implement.

Eric

PROT_SEM was a mistake, I feel. It's way too easy to get it wrong. You have most architectures and environments that don't need it, and as a result, applications simply won't have it in their sources.

I suspect that with MAP_SHARED + PROT_WRITE being pretty uncommon anyway, we can probably find trivial patterns in the kernel. Like only one process holding that file open - which is what you get with things that use mmap() to write a new file (I think "ld" used to have a config option to write files that way, for example).

And if we end up sometimes giving "lock" meaning even when it's not needed, tough. The point of the simple hack is very much a "get 90% of the advantage for very little effort".

Regardless, even if we get a flag like that (and the Intel people didn't seem to dismiss the idea), it's likely a more than a few years down the line. So it's not like this is a pressing concern ;)

Linus

NO.

There is not a _single_ compiler that is multi-threaded, and I'd argue that there probably never will be. It's pointless.

There's a _lot_ of really performance-sensitive stuff that will NEVER EVER be threaded. You may run a hundred copies of them at the same time, but every single copy will be single-threaded.

And this will optimize that case in a BIG way.

This is _not_ about "CPU hotplug". This is _not_ about "threaded apps before they are threaded". This is all about the fact that serious computation is done single-threaded, and anybody who thinks that single-threading is going away is so totally out to lunch that it's not even fun.

And yes, Sun will die. Single-thread performance matters a hell of a lot, and any company that bets that it doesn't, is a failure.

Linus

I get the feeling you didn't read my message at all. Let me try it again.

Why should we use a silicon based solution for this, when I posit that there are simpler and equally effective userspace solutions?

Daniel Jacobowitz
CodeSourcery, LLC

Name them.

In user space, doing things like clever run-time linking things is actually horribly bad. It causes COW faults at startup, and/or makes the compiler have to do indirections unnecessarily. Both of which actually make caches less effective, because now processes that really effectively do have exactly the same contents have them in different pages.

The other alternative (which apparently glibc actually does use) is to dynamically branch over the lock prefixes, which actually works better:
it's more work dynamically, but it's much cheaper from a startup standpoint and there's no memory duplication, so while it is the "stupid" approach, it's actually better than the clever one.

The third alternative is to know at link-time that the process never does anything threaded, but that needs more developer attention and non-standard setups, and you _will_ get it wrong (some library will create some thread without the developer even realizing). It also has the duplicated library overhead (but at least now the duplication is just twice, not "each process duplicates its own private pointer")

In short, there simply isn't any good alternatives. The end result is that thread-safe libraries are always in practice thread-safe even on UP, even though that serializes the CPU altogether unnecessarily.

I'm sure you can make up alternatives every time you hit one _particular_ library, but that just doesn't scale in the real world.

In contrast, the simple silicon support scales wonderfully well. Suddenly libraries can be thread-safe _and_ efficient on UP too. You get to eat your cake and have it too.

Linus

Those are the wrong ways of doing this in userspace. There are right ways. For instance, tag the binary at link time "single-threaded". Use dynamic linking and the existing hwcap mechanism to select single-threaded libraries instead of the default ones. Your single-threaded applications will no longer mmap the same copy of glibc as your multi-threaded applications; this does make caching mildly less effective but only if you have a single-threaded app and a multi-threaded one fighting for CPU time.

Glibc does not do this to the best of my knowledge. It does select different code paths in various places based on the presence of multiple threads, but that's for cancellation, not for locking.

This is also a trivially solvable problem in userspace; you make the dynamic linker enforce consistency of the tags.

The number of userspace libraries that use atomic operations is, in practice, quite small.

By buying new hardware and only caring about people using the magic architecture. No thanks.

Maybe I'll implement this some weekend.

Daniel Jacobowitz
CodeSourcery, LLC

And I mentioned exactly this. It's my third alternative.

And it doesn't work well, exactly because developers don't know if the libraries they use are always single-threaded etc. More importantly, it just doesn't happen that much. People do "make ; make install". Hopefully from pretty standard sources. Having to tweak things so that a project compiles with a magic flag on a particular distribution is simply not done very much.

Linus

But distributors (Debian included) do this all the time :-)

I'd even volunteer to get it done and pushed out and in use, if I was as convinced of the benefits. For most applications, though, I'm still sceptical.

Daniel Jacobowitz
CodeSourcery, LLC

This works and the system is designed this way. But it's unlikely that any distribution will ship code like this since the maintenance is to problematic.

Wrong. Linus is right, we jump over lock prefix. After a lot of benchmarking I found this to be the fastest was and the Intel people seemed to agree.

This would require that potentially every single DSO is duplicated as threaded and non-threaded. If you like this you might as well enter the horror world of BSD with their libc_r. This will never fly, the support costs are too high.

It really not and the number using them is growing.

➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

That is the problem, waiting for Intel to do hardware magic, or even to decide IF they do it. Like assuming that everyone has SMP because a few percent of the users have dual core chips. The majority of the markey will have SMP someday, but ignoring the current status isn't realistic.

Love to see it, I'm only semi-convinced it can be done in a way which actually produces significant benefits.

   -bill davidsen ([email protected])
"The secret to procrastination is to put things off until the
 last possible moment - but no longer"  -me

Using self modifying code stubs will work, and Intel's architecture will support it. This would be faster than waiting 2-3 years for Intel to spin a processor rev. NetWare did something similair with global branch tables for memory protection.

J

Small data point: In a lot of gcc-related build processes, the configure/makefile junk passes '-pthread' to the compiler/linker.

So a lot of programs in Linux distros are already built this way. The bigger problem is with libraries, which cannot know ahead of time whether the app is threaded or not, and therefore must assume threaded.

A few libs do things like glibc, others (like GLib) have an explicit mylib_thread_init() called at program startup.

Jeff

Just a note to say glibc is getting better wrt to locking. Compare the results and trival test program here:
http://lkml.org/lkml/2001/12/7/75
That showed that for glibc 2.2.4, getc & putc were 669% slower than the unlocked versions.

4 years later and with 2.3.5-1ubuntu1, getc & putc are only 230% slower than the unlocked versions:

$ dd bs=1MB count=100 if=/dev/zero | ./locked >/dev/null
100000000 bytes transferred in 3.709362 seconds (26958813 bytes/sec)
$ dd bs=1MB count=100 if=/dev/zero | ./unlocked >/dev/null
100000000 bytes transferred in 1.602427 seconds (62405339 bytes/sec)

Pádraig.

this could also mean that the unlocked version has gotten slower ;)

I believe that a hardware solution would also accomodate the case where a program runs unthreaded for most of the processing, and only starts threads to do the final stage "report generation" tasks, where that makes sense. I don't believe that it helps in the case where init uses threads and then reverts to a single thread for the balance of the task. I can't think of anything which does that, so it's probably a non-critical corner case, or something the thread library could correct.

   -bill davidsen ([email protected])
"The secret to procrastination is to put things off until the
 last possible moment - but no longer"  -me

Startup routine of a scientific app calls a multithreaded "fetch work" routine, then crunches the data using a single thread. This could even happen somewhere inside a library, so the application itself is unaware that threads were ever invoked. This is not a far-fetched case.

You really need per-address object notions of "threadedness" when talking about shared memory, since you may need shared memory to be atomic, but operate on the heap in single threaded fashion.

Zach

In 2-3 years we might actually see the hardware solution, maybee .... I am skeptical Intel will move quickly on it. A software solution will get out faster.

Jeff

I'm not sure a hardware solution is even the right thing - consider a shared memory database process with a private heap. You really want locks on the shared memory, and you really don't on the heap.

You need a way to type the lock semantics by memory region, and a working hardware solution can not perform as well as a careful software solution. As was pointed out earlier, you can't use memory type attributes to infer lock semantics, you must assume them in the decoder or implement complex deadlock detection and recovery in silicon.

I would be willing to bet that library users know best. Most cleanly written libraries already have wrapper functions that can be used to plug in needed libc functions like malloc, even file I/O. Even if they don't, you can rewrap all of the imported functions. Using this, you can isolate threaded libraries from single threaded applications, and make sure the performance critical libraries use non-threaded operations. You can even afford to use a medium heavy hammer and switch from non-threaded to threaded dependent libraries every time you call a thread-using library function, because by assumption, the majority of performance critical code is going to be running single threaded.

Zach

Sure you can. You just have to be prepared to take a microop exception if you speculate incorrectly.

-hpa

I spoke silicon too heavy handedly. The complexity of the issue disappears if you take an exception, but rewinding state prior to the exception and reissuing is going to be less efficient than getting it right the first time, which is something software can always guarantee. You need to add more hardware for prediction to get it right all the time, and it is not clear the cost of that hardware is justified when software can always do the right thing.

Zach

Taking exceptions is fine as long as you don't do it too often. I'm starting to suspect that the only way to do this right all the time is to have this be part of the page attributes, since it's region-specific.

-hpa

The problem is that nobody will change all the software. Your careful software solution will only benefit a small minority of performance conscious and well tuned programs.

The hardware solution might not be perfect, but has a good chance to apply to 90% of the "don't care" programs and help them all a bit. And every bit counts in the quest for more single thread performance.

And if someone wants to fine tune their programs they can still change the software as much as they want.

-Andi

Now I see what you are aiming at, yes that makes vast amounts of sense and since AMD have the "no lock effect" bit for general case maybe they can

What it really comes down to (virtualization or not!) is whether or not the OS can guarantee that nothing else is messing with memory at the same time.

This is potentially different from process to process (because of page table differences) and from kernel to user space (because of the User bit in the page tables.)

-hpa

CPUs in embedded the space could outnumber desktops & servers greatly (cell phones, access pointers, routers, media players, etc). Most of these will be UP for some time.

That's not entirely clear either.

There are definite advantages to SMP even in the embedded space - or, to put it more strongly: _especially_ in the embedded space.

Not only does power usage go up cubically with frequency (which means that two cores are a lot more efficient than one at double the frequency), but embedded space also often has some clear separation between tasks that -can- be threaded (and often part of is has real-time characteristics, so getting a core of its own can be a good thing). Often more so than in the desktop space.

Now, obviously, in the "4- or 8-bit microcontroller" kind of embedded space, SMP isn't going to be a big issue. But anything that already uses an ARM, MIPS or a PowerPC-like chip, going SMP is not at all ridiculous. That includes things like cellphones, where one core might be for communication functions, and one for smartphones.

None of the cellphone manufacturers seem to be in the least interested in doing a "phone only" solution. They can already do that cheaply, they can't make much money off it, and they are all interested in features. And it really _is_ more power-efficient to have, say, a dual-core 200MHz chip than it is to have a single-core 300MHz one.

Now, sometimes those SMP systems will actually be used as "tightly coupled UP", where one of the CPU's is just basically a DSP. And from a power efficiency standpoint, having specialized hardware (and thus _A_MP rather than SMP) is obviously better, but in complex tasks - and communication tends to be that - general-purpose is often desirable enough that people will take the inefficiencies of a GP CPU over a fixed-function specialized DSP-kind of environment.

But SMP is absolutely _not_ unusual in embedded. It's been there for years already, and it's clearly moving downwards there too.

Linus

I would argue that there is no "the embedded space," but rather a set of embedded spaces with various needs. Having worked doing industrial control for three years and lunched with IC folks another decade, I'm fairly sure that consumer goods are very different from real industrial control, a realtime items (multimedia) are different than phones and PDAs. Until the phone gets "swear at it" slow, features like voice recognition are more important than doing voice to number lookup in 20ms instead of 400ms. Cost and battery life matter a lot too, while the media and IC markets are already attached to expensive stuff, so the computer is is smaller fraction of the cost.

Absolutely true, but that dual core 200 MHz chip probably draws more power than a 200 MHz uni, etc. So there will probably be uni applications for the forseeable future, any benefit in uni performance will be useful.

   -bill davidsen ([email protected])
"The secret to procrastination is to put things off until the
 last possible moment - but no longer"  -me

It's unlikely, though, that you'd have a need to run an SMP-compiled kernel on these devices.

-hpa

Given the CPU architecture it is unimplementable. Intructions are split into microinstuctions and they are executed out of order. PAT is looked up when LOAD microinstruction is executed. Imagine this

MOV EDX, [address1]
LOCK ADD [address2], EDX

that is translated to

LOAD EDX, [address1]
LOAD TMP1, [address2]
ADD TMP1, EDX
STORE [address2], TMP1

... now LOAD finds LOCK attribute in PAT --- so it locks the bus, however EDX is still not loaded. Now LOAD EDX can't execute because the bus is locked and ADD and STORE can't execute because they're waiting for LOAD EDX. Deadlock.

Locks are so slow not because they are locks (if the target is in L1 cache, they operate only on cache and don't go to bus at all), but because they need to flush completely microinstruction pool to avoid problems like this. Of course Intel won't waste silicon in the execution engine for instructions that execute so rarely, so they microcode them instead. So lock detection is done at the decoder.

Mikulas

Why should they waste their (already complex) decoding logic with that? Why can't an application instead set a bit somewhere if it's running on SMP and if it's threaded and branch to variants with and without lock prefix?

(correctly predicted branch is even faster than some microcode to determine the value of your bit)

Mikulas

Why? Because it would make the same code run faster on their CPU than on their competitor's. That's the kind of things that make x86 vendors smile.

-hpa

This is a goof rist step. But the effectiveness will descrease significantly in the near future. It can only work for single-threaded processes without writable shared memory.

With the growing number of cores/threads the need to use parallelism rises. With techniques like OpenMP the threshold to do this is lowered significantly. The process-model, so much preferred on this list over the thread model, requires shared memory, therefore also eliminating the effectiveness of this functionality.

A real solution needs to be more fine grained. It is often known in the userland code whether the specific word which is accessed using atomic ops really can be shared. The POSIX interfaces, for instance, require that all mutexes etc which are placed in shared memory are attributed as such. Combine this with the knowledge about the number of threads in use and the result is that even with writable shared memory segments the lock prefix can be avoided. There are a whole bunch of cases where we already do conditional locking. It's just plain ugly and not as efficient as we would like i t.

So, implementing control with per-userlevel context will se rapidly diminishing success and I'm wondering whether it is better to go for something with a bit finer level of control.

➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

Patching in/out SMP-locking with more than one active CPU would be a pretty silly idea in the first place ;)

16 .smp_altinstructions 0000ae0b c03b4000 003b4000 002b4000 2**2
CONTENTS, ALLOC, LOAD, READONLY, DATA
17 .smp_altinstr_replacement 00000f6e c03bee0b 003bee0b 002bee0b 2**0
CONTENTS, ALLOC, LOAD, CODE

cheers,

Gerd

No, doing this crap with CPU hotplug is a silly idea. Patching on a real UP system, and then throwing out the tables, makes sense. Keeping two sets of tables for a minimal performance improvement in a very rare configuration (CPU hotplug is the exception, not the rule) is just plain stupid. You probably lose as much performance from the memory hogged up in the tables as you gain from it, and on every system where you have the tables at all you take the hit.

-hpa