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.
On Tuesday 09 May 2006 19:29, 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?
This is part of a recent patch by Gerd Hoffmann:
>> Implement SMP alternatives, i.e. switching at runtime between different
>> code versions for UP and SMP. The code can patch both SMP->UP and UP->SMP.
>> The UP->SMP case is useful for CPU hotplug.
An explicit test of the smp variable (not CONFIG_SMP macro) exists in this
code. I missed the discussion about this patch, but it appears from reading
the patch notes that the behavior is intentional. More information can be
found in the git and lkml archives.
> Rene.
Thanks,
Chase
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 <[email protected]>
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
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.
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");
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)
Mikael Pettersson <[email protected]> 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
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
}
On 11 May 2006 20:28:09 +0200, Andi Kleen wrote:
>Mikael Pettersson <[email protected]> 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
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
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.
