[PATCH] setup_per_cpu_areas in 2.5.8pre3

[PATCH] setup_per_cpu_areas in 2.5.8pre3

[Andries.Brouwer]

Now that I am writing anyway, one of the changes I needed
to compile 2.5.8pre3 is the following.

--- main.c~	Fri Apr 12 12:10:48 2002
+++ main.c	Sun Apr 14 16:11:33 2002
@@ -270,7 +270,9 @@
 #else
 #define smp_init()	do { } while (0)
 #endif
-
+static inline void setup_per_cpu_areas(void)
+{
+}
 #else
 
 #ifdef __GENERIC_PER_CPU

There is a nest of #ifdef's there, and in some branches
setup_per_cpu_areas() is not defined.

Of course the real fix is to remove the #ifdef's,
maybe using a weak symbol instead, or some other construction
that defines an empty default that can be replaced by an actual
routine.

Andries

Re: [PATCH] setup_per_cpu_areas in 2.5.8pre3

[Rusty Russell]

On Sun, 14 Apr 2002 21:15:29 GMT
Andries.Brouwer@cwi.nl wrote:

> 
> Now that I am writing anyway, one of the changes I needed
> to compile 2.5.8pre3 is the following.

Yeah, better patch below (__GENERIC_PER_CPU implies SMP anyway).

> Of course the real fix is to remove the #ifdef's,
> maybe using a weak symbol instead, or some other construction
> that defines an empty default that can be replaced by an actual
> routine.

Not unless you make it as readable as the current code.  Having magic
appearing functions sounds cool, but beware that the cure might be
worse than the disease.

Yes, I know this patch looks like I'm moving smp_init(), but really
it's moving the #ifdef __GENERIC_PER_CPU bit past the SMP #endif.

Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy
diff -urN -I \$.*\$ --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.8/init/main.c working-2.5.8-percpu/init/main.c
--- linux-2.5.8/init/main.c	Mon Apr 15 11:47:50 2002
+++ working-2.5.8-percpu/init/main.c	Tue Apr 16 21:11:50 2002
@@ -274,6 +274,18 @@
 
 #else
 
+/* Called by boot processor to activate the rest. */
+static void __init smp_init(void)
+{
+	/* Get other processors into their bootup holding patterns. */
+	smp_boot_cpus();
+
+	smp_threads_ready=1;
+	smp_commence();
+}
+
+#endif
+
 #ifdef __GENERIC_PER_CPU
 unsigned long __per_cpu_offset[NR_CPUS];
 
@@ -301,18 +313,6 @@
 {
 }
 #endif /* !__GENERIC_PER_CPU */
-
-/* Called by boot processor to activate the rest. */
-static void __init smp_init(void)
-{
-	/* Get other processors into their bootup holding patterns. */
-	smp_boot_cpus();
-
-	smp_threads_ready=1;
-	smp_commence();
-}
-
-#endif
 
 /*
  * We need to finalize in a non-__init function or else race conditions

Re: [PATCH] setup_per_cpu_areas in 2.5.8pre3

[Andries.Brouwer]

    > Now that I am writing anyway, one of the changes I needed
    > to compile 2.5.8pre3 is the following.

    Yeah, better patch below

Good.

    > Of course the real fix is to remove the #ifdef's,
    > maybe using a weak symbol instead, or some other construction
    > that defines an empty default that can be replaced by an actual
    > routine.

    Not unless you make it as readable as the current code.  Having magic
    appearing functions sounds cool, but beware that the cure might be
    worse than the disease.

But maybe I do not think that the current code is very readable.
Probably because just before fixing this compilation problem I had
to fix a different one where atomic_dec_and_lock was undefined,
and one finds a forest of #ifdef's in spinlock.h.

#ifdef's are evil. You have one, and there are two possible sources.
Easy and readable. You have two, and there are four. Already a small
effort to check that indeed all four combinations are OK. That was
what went wrong in the setup_per_cpu_areas case. You have three and
it is almost certain that someone forgets to check all possible
eight cases.

#ifdef's hide source from the compiler, so that when stuff compiles
for the developer it need not compile for the next person who comes
along. We have a kernel compilation project because of that.

So, if we can replace a certain type of #ifdef use by a different
formal construction, where all source is seen by the compiler,
that might well be progress.

Andries

Re: [PATCH] setup_per_cpu_areas in 2.5.8pre3

[Tom Rini]

On Wed, Apr 17, 2002 at 10:24:29AM +0200, Andries.Brouwer@cwi.nl wrote:
 
> #ifdef's are evil. You have one, and there are two possible sources.
> Easy and readable. You have two, and there are four. Already a small
> effort to check that indeed all four combinations are OK. That was
> what went wrong in the setup_per_cpu_areas case. You have three and
> it is almost certain that someone forgets to check all possible
> eight cases.

How does the following patch look ?  It's alot longer than the rest but
imho it makes things readable as well.

-- 
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

===== init/main.c 1.40 vs edited =====
--- 1.40/init/main.c	Tue Apr  9 13:35:31 2002
+++ edited/init/main.c	Mon Apr 15 08:56:20 2002
@@ -261,20 +261,7 @@
 extern void setup_arch(char **);
 extern void cpu_idle(void);
 
-#ifndef CONFIG_SMP
-
-#ifdef CONFIG_X86_LOCAL_APIC
-static void __init smp_init(void)
-{
-	APIC_init_uniprocessor();
-}
-#else
-#define smp_init()	do { } while (0)
-#endif
-
-#else
-
-#ifdef __GENERIC_PER_CPU
+#if defined(CONFIG_SMP) && defined(__GENERIC_PER_CPU)
 unsigned long __per_cpu_offset[NR_CPUS];
 
 static void __init setup_per_cpu_areas(void)
@@ -300,8 +287,9 @@
 static inline void setup_per_cpu_areas(void)
 {
 }
-#endif /* !__GENERIC_PER_CPU */
+#endif
 
+#ifdef CONFIG_SMP
 /* Called by boot processor to activate the rest. */
 static void __init smp_init(void)
 {
@@ -311,7 +299,15 @@
 	smp_threads_ready=1;
 	smp_commence();
 }
-
+#else
+#ifdef CONFIG_X86_LOCAL_APIC
+static void __init smp_init(void)
+{
+	APIC_init_uniprocessor();
+}
+#else
+#define smp_init()	do { } while (0)
+#endif
 #endif
 
 /*