Re: [patch] SMP alternatives: skip with UP kernels.

Re: [patch] SMP alternatives: skip with UP kernels.

[Mikael Pettersson]

On Thu, 11 May 2006 14:13:22 +0200, Gerd Hoffmann wrote:
>+	if (0 == (__smp_alt_end - __smp_alt_begin))
>+		return; /* no tables, nothing to patch (UP kernel) */

That's an unnecessarily obscure way of stating the obvious:

	if (__smp_alt_end == __smp_alt_begin)

Re: [patch] SMP alternatives: skip with UP kernels.

[Andi Kleen]

Mikael Pettersson <mikpe@it.uu.se> writes:

> On Thu, 11 May 2006 14:13:22 +0200, Gerd Hoffmann wrote:
> >+	if (0 == (__smp_alt_end - __smp_alt_begin))
> >+		return; /* no tables, nothing to patch (UP kernel) */
> 
> That's an unnecessarily obscure way of stating the obvious:
> 
> 	if (__smp_alt_end == __smp_alt_begin)

iirc ISO-C guarantees that symbols have different values and the
optimizer could possibly make use of that fact. So you might actually
need some RELOC_HIDE()s to make this safe.

-Andi

Re: [patch] SMP alternatives: skip with UP kernels.

[Mikael Pettersson]

On 11 May 2006 20:28:09 +0200, Andi Kleen wrote:
>Mikael Pettersson <mikpe@it.uu.se> writes:
>
>> On Thu, 11 May 2006 14:13:22 +0200, Gerd Hoffmann wrote:
>> >+	if (0 == (__smp_alt_end - __smp_alt_begin))
>> >+		return; /* no tables, nothing to patch (UP kernel) */
>> 
>> That's an unnecessarily obscure way of stating the obvious:
>> 
>> 	if (__smp_alt_end == __smp_alt_begin)
>
>iirc ISO-C guarantees that symbols have different values and the
>optimizer could possibly make use of that fact. So you might actually
>need some RELOC_HIDE()s to make this safe.

OK, I didn't consider that (with "extern $type a[], b[];", "a == b"
is false by definition). However, the (0 == (_ - _)) version is no
safer, since a compiler can legally turn it back to a single "==".

Since you brought up the "rules of C" argument: the
(__smp_alt_end - __smp_alt_begin) expression is undefined because
pointer subtraction is only valid if both pointers point into
the same object (or just after it), which isn't the case here.

A plain "==" with RELOC_HIDE() on the operands would be best IMO.

/Mikael

2.6.17-rc3 -- SMP alternatives: switching to UP code

[Rene Herman]

Hi list.

I just noticed this in the 2.6.17-rc3 dmesg:

===
Checking 'hlt' instruction... OK.
SMP alternatives: switching to UP code
Freeing SMP alternatives: 0k freed
ACPI: setting ELCR to 0400 (from 1608)
===

Should I be seeing this "SMP alternatives" thing on a !CONFIG_SMP 
kernel? It does say 0k, but something is apparently being done at 
runtime still. Why?

Rene.

Re: 2.6.17-rc3 -- SMP alternatives: switching to UP code

[Gerd Hoffmann]

Rene Herman wrote:
> Hi list.
> 
> I just noticed this in the 2.6.17-rc3 dmesg:
> 
> ===
> Checking 'hlt' instruction... OK.
> SMP alternatives: switching to UP code
> Freeing SMP alternatives: 0k freed
> ACPI: setting ELCR to 0400 (from 1608)
> ===
> 
> Should I be seeing this "SMP alternatives" thing on a !CONFIG_SMP
> kernel? It does say 0k, but something is apparently being done at
> runtime still. Why?

The UP kernel has empty alternatives tables (as you've noticed), thus
the code doesn't do anything.  Nevertheless it probably makes sense to
add a few #ifdef CONFIG_SMP lines to avoid confusing people and safe a
few bytes ...

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
Erst mal heiraten, ein, zwei Kinder, und wenn alles läuft
geh' ich nach drei Jahren mit der Familie an die Börse.
http://www.suse.de/~kraxel/julika-dora.jpeg

Re: 2.6.17-rc3 -- SMP alternatives: switching to UP code

[Rene Herman]

Gerd Hoffmann wrote:

> Rene Herman wrote:

>> Should I be seeing this "SMP alternatives" thing on a !CONFIG_SMP
>> kernel? It does say 0k, but something is apparently being done at
>> runtime still. Why?
> 
> The UP kernel has empty alternatives tables (as you've noticed), thus
> the code doesn't do anything.  Nevertheless it probably makes sense to
> add a few #ifdef CONFIG_SMP lines to avoid confusing people and safe a
> few bytes ...

Okay, thanks. Yes, I agree such would make sense.

Rene.

[patch] SMP alternatives: skip with UP kernels.

[Gerd Hoffmann]

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

> Okay, thanks. Yes, I agree such would make sense.

Patch below.  It simply returns in case the tables are empty and nothing
is do to, thus avoids printing the confusing message.

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
Erst mal heiraten, ein, zwei Kinder, und wenn alles läuft
geh' ich nach drei Jahren mit der Familie an die Börse.
http://www.suse.de/~kraxel/julika-dora.jpeg

[-- Attachment #2: ifdef-smp-alts --]
[-- Type: text/plain, Size: 641 bytes --]

Index: vanilla-2.6.17-rc3-mm1/arch/i386/kernel/alternative.c
===================================================================
--- vanilla-2.6.17-rc3-mm1.orig/arch/i386/kernel/alternative.c	2006-05-10 12:25:39.000000000 +0200
+++ vanilla-2.6.17-rc3-mm1/arch/i386/kernel/alternative.c	2006-05-10 15:00:10.000000000 +0200
@@ -349,6 +349,9 @@ void __init alternative_instructions(voi
 	smp_alt_once = 1;
 #endif
 
+	if (0 == (__smp_alt_end - __smp_alt_begin))
+		return; /* no tables, nothing to patch (UP kernel) */
+
 	if (smp_alt_once) {
 		if (1 == num_possible_cpus()) {
 			printk(KERN_INFO "SMP alternatives: switching to UP code\n");

Re: [patch] SMP alternatives: skip with UP kernels.

[Rene Herman]

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

Gerd Hoffmann wrote:

[ depending on CONFIG_SMP ]

>> Okay, thanks. Yes, I agree such would make sense.
> 
> Patch below.  It simply returns in case the tables are empty and nothing
> is do to, thus avoids printing the confusing message.

It does avoid the message but well, that's also _all_ it avoids. Why do 
you want to do it this way? If I apply the attached, things compile and 
work fine which seems to confirm that, yes, all this smp_alternatives 
code is simply dead baggage on !CONFIG_SMP.

Yes, the #ifdef in arch/i386/kernel/module.c is a bit clumsy. Just proof 
of concept, might be abstracted out a bit better. It does lose most of 
alternatives.c which would seem to be good thing.

Unless I'm missing some points, I believe this code should not be 
compiled in for !CONFIG_SMP.

One other point, although probably not a relevant one. Your changes in 
module_finalize() potentially change behaviour. It used to be:

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);
}

which means that any .altinstructions section would be patched. It's now:

for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) {
	...
	if (!strcmp(".altinstructions", secstrings + s->sh_name))
		alt = s;
	...
}
if (alt) {
	/* patch .altinstructions */
	void *aseg = (void *)alt->sh_addr;
	apply_alternatives(aseg, aseg + alt->sh_size);
}

which means that only the last such section would. I suppose there's 
only one such section anyway, but not seeing a break in the original 
does make me wonder if this was originally done on purpose.

Rene.


[-- Attachment #2: alternatives_no_smp.diff --]
[-- Type: text/plain, Size: 2282 bytes --]

Index: local/arch/i386/kernel/alternative.c
===================================================================
--- local.orig/arch/i386/kernel/alternative.c	2006-05-11 20:28:56.000000000 +0200
+++ local/arch/i386/kernel/alternative.c	2006-05-11 20:32:24.000000000 +0200
@@ -118,6 +118,8 @@ void apply_alternatives(struct alt_instr
 	}
 }
 
+#ifdef CONFIG_SMP
+
 static void alternatives_smp_save(struct alt_instr *start, struct alt_instr *end)
 {
 	struct alt_instr *a;
@@ -283,10 +285,13 @@ void alternatives_smp_switch(int smp)
 	spin_unlock_irqrestore(&smp_alt, flags);
 }
 
+#endif /* CONFIG_SMP */
+
 void __init alternative_instructions(void)
 {
 	apply_alternatives(__alt_instructions, __alt_instructions_end);
 
+#ifdef CONFIG_SMP
 	/* switch to patch-once-at-boottime-only mode and free the
 	 * tables in case we know the number of CPUs will never ever
 	 * change */
@@ -318,4 +323,5 @@ void __init alternative_instructions(voi
 					    _text, _etext);
 		alternatives_smp_switch(0);
 	}
+#endif
 }
Index: local/arch/i386/kernel/module.c
===================================================================
--- local.orig/arch/i386/kernel/module.c	2006-05-11 20:28:56.000000000 +0200
+++ local/arch/i386/kernel/module.c	2006-05-11 20:29:00.000000000 +0200
@@ -112,12 +112,14 @@ int module_finalize(const Elf_Ehdr *hdr,
 	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
 
 	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) { 
-		if (!strcmp(".text", secstrings + s->sh_name))
-			text = s;
 		if (!strcmp(".altinstructions", secstrings + s->sh_name))
 			alt = s;
+#ifdef CONFIG_SMP
 		if (!strcmp(".smp_locks", secstrings + s->sh_name))
 			locks= s;
+		if (!strcmp(".text", secstrings + s->sh_name))
+			text = s;
+#endif
 	}
 
 	if (alt) {
@@ -126,16 +128,20 @@ int module_finalize(const Elf_Ehdr *hdr,
 		apply_alternatives(aseg, aseg + alt->sh_size);
 	}
 	if (locks && text) {
+#ifdef CONFIG_SMP
 		void *lseg = (void *)locks->sh_addr;
 		void *tseg = (void *)text->sh_addr;
 		alternatives_smp_module_add(me, me->name,
 					    lseg, lseg + locks->sh_size,
 					    tseg, tseg + text->sh_size);
+#endif
 	}
 	return 0;
 }
 
 void module_arch_cleanup(struct module *mod)
 {
+#ifdef CONFIG_SMP
 	alternatives_smp_module_del(mod);
+#endif
 }

Re: [patch] SMP alternatives: skip with UP kernels.

[Gerd Hoffmann]

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

>> Patch below.  It simply returns in case the tables are empty and nothing
>> is do to, thus avoids printing the confusing message.
> 
> It does avoid the message but well, that's also _all_ it avoids. Why do
> you want to do it this way?  [ ... patch ... ]
> 
> Yes, the #ifdef in arch/i386/kernel/module.c is a bit clumsy.

Yep, thats why.  I wanted avoid exactly that.  Having some code need to
know that function foobar() is only available with CONFIG_BAZ is set is
really ugly ...

The attached patch hides the magic in alternative.h and provides some
dummy inline functions for the UP case (gcc should manage to optimize
away these calls).  No changes in module.c.

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
Erst mal heiraten, ein, zwei Kinder, und wenn alles läuft
geh' ich nach drei Jahren mit der Familie an die Börse.
http://www.suse.de/~kraxel/julika-dora.jpeg

[-- Attachment #2: ifdef-smp-alts --]
[-- Type: text/plain, Size: 3161 bytes --]

Index: vanilla-2.6.17-rc3-mm1/arch/i386/kernel/alternative.c
===================================================================
--- vanilla-2.6.17-rc3-mm1.orig/arch/i386/kernel/alternative.c	2006-05-10 12:25:39.000000000 +0200
+++ vanilla-2.6.17-rc3-mm1/arch/i386/kernel/alternative.c	2006-05-12 10:03:40.000000000 +0200
@@ -168,6 +168,8 @@ void apply_alternatives(struct alt_instr
 	}
 }
 
+#ifdef CONFIG_SMP
+
 static void alternatives_smp_save(struct alt_instr *start, struct alt_instr *end)
 {
 	struct alt_instr *a;
@@ -328,6 +330,8 @@ void alternatives_smp_switch(int smp)
 	spin_unlock_irqrestore(&smp_alt, flags);
 }
 
+#endif
+
 void __init alternative_instructions(void)
 {
 	if (no_replacement) {
@@ -349,6 +353,7 @@ void __init alternative_instructions(voi
 	smp_alt_once = 1;
 #endif
 
+#ifdef CONFIG_SMP
 	if (smp_alt_once) {
 		if (1 == num_possible_cpus()) {
 			printk(KERN_INFO "SMP alternatives: switching to UP code\n");
@@ -370,4 +375,5 @@ void __init alternative_instructions(voi
 					    _text, _etext);
 		alternatives_smp_switch(0);
 	}
+#endif
 }
Index: vanilla-2.6.17-rc3-mm1/include/asm-i386/alternative.h
===================================================================
--- vanilla-2.6.17-rc3-mm1.orig/include/asm-i386/alternative.h	2006-05-09 18:56:14.000000000 +0200
+++ vanilla-2.6.17-rc3-mm1/include/asm-i386/alternative.h	2006-05-12 10:06:28.000000000 +0200
@@ -17,11 +17,19 @@ struct alt_instr {
 extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
 
 struct module;
+#ifdef CONFIG_SMP
 extern void alternatives_smp_module_add(struct module *mod, char *name,
 					void *locks, void *locks_end,
 					void *text, void *text_end);
 extern void alternatives_smp_module_del(struct module *mod);
 extern void alternatives_smp_switch(int smp);
+#else
+static inline void alternatives_smp_module_add(struct module *mod, char *name,
+					void *locks, void *locks_end,
+					void *text, void *text_end) {}
+static inline void alternatives_smp_module_del(struct module *mod) {}
+static inline void alternatives_smp_switch(int smp) {}
+#endif
 
 #endif
 
Index: vanilla-2.6.17-rc3-mm1/include/asm-x86_64/alternative.h
===================================================================
--- vanilla-2.6.17-rc3-mm1.orig/include/asm-x86_64/alternative.h	2006-05-09 18:56:14.000000000 +0200
+++ vanilla-2.6.17-rc3-mm1/include/asm-x86_64/alternative.h	2006-05-12 10:05:26.000000000 +0200
@@ -17,11 +17,20 @@ struct alt_instr {
 extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
 
 struct module;
+
+#ifdef CONFIG_SMP
 extern void alternatives_smp_module_add(struct module *mod, char *name,
 					void *locks, void *locks_end,
 					void *text, void *text_end);
 extern void alternatives_smp_module_del(struct module *mod);
 extern void alternatives_smp_switch(int smp);
+#else
+static inline void alternatives_smp_module_add(struct module *mod, char *name,
+					void *locks, void *locks_end,
+					void *text, void *text_end) {}
+static inline void alternatives_smp_module_del(struct module *mod) {}
+static inline void alternatives_smp_switch(int smp) {}
+#endif
 
 #endif
 

Re: [patch] SMP alternatives: skip with UP kernels.

[Rene Herman]

Gerd Hoffmann wrote:

>> Yes, the #ifdef in arch/i386/kernel/module.c is a bit clumsy.
> 
> Yep, thats why.  I wanted avoid exactly that.  Having some code need to
> know that function foobar() is only available with CONFIG_BAZ is set is
> really ugly ...
> 
> The attached patch hides the magic in alternative.h and provides some
> dummy inline functions for the UP case (gcc should manage to optimize
> away these calls).  No changes in module.c.

Works for me; it doesn't easily get non-clumsy in module.c I see. Sure 
you want to keep smp_alt_once outside the #ifdef? Seems to not be doing 
anything other than being set to 1 for !SMP.

Thanks,
Rene.