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

[Rene Herman <rene.herman@keyaccess.nl>]

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.

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
 }