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
On Sun, 14 Apr 2002 21:15:29 GMT
[email protected] 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
> 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
On Wed, Apr 17, 2002 at 10:24:29AM +0200, [email protected] 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
/*