Timer list init is done AFTER use

On SMP systems the timer list init is done by way of a cpu_notifier call. This has two problems:

1.) Timers are started WAY before the cpu_notifier call chain is executed. In particular the console blanking timer is deleted and inserted every time printk() is called. That this does not fail is only because the kernel has yet to protect location zero.

2.) This notifier is called when a cpu comes up. I suspect that initializing the timer list when a hot swap of a cpu is done is NOT the right thing to do. In any case, if this is a desired action, the list still needs to be initialized prior to its use.

The attached patch initializes all the timer lists at init_timers time and does not put code in the notify list.

Quoted 9 lines · george anzinger

On SMP systems the timer list init is done by way of a cpu_notifier call. This has two problems:

1.) Timers are started WAY before the cpu_notifier call chain is executed. In particular the console blanking timer is deleted and inserted every time printk() is called. That this does not fail is only because the kernel has yet to protect location zero.

But init_timers() directly calls timer_cpu_notify(), which directly calls init_timers_cpu().

So your patch appears to be a no-op for the boot CPU.

Quoted 6 lines

2.) This notifier is called when a cpu comes up. I suspect that initializing the timer list when a hot swap of a cpu is done is NOT the right thing to do. In any case, if this is a desired action, the list still needs to be initialized prior to its use.

It should be OK as-is? The CPU_UP_PREPARE callout is performed before the secondary starts doing things. Its timers are initialised.

Quoted 3 lines

The attached patch initializes all the timer lists at init_timers time and does not put code in the notify list.

But the patch assumes that the per-cpu data exists for all CPUs - even the !cpu_possible() ones.

This is true at present. But the intent here is that the per-cpu storage be allocated as the CPUs come up, and in their node-local memory. That saves memory and presumably having the cpu-local timers in the cpu-local memory is a good thing.

I have working code which did all that, but it sort-of got lost because there was a lot going on at the time.

Have you actually observed any problem?

Quoted 12 lines · Andrew Morton

Quoted 9 lines · george anzinger

On SMP systems the timer list init is done by way of a cpu_notifier call. This has two problems:

1.) Timers are started WAY before the cpu_notifier call chain is executed. In particular the console blanking timer is deleted and inserted every time printk() is called. That this does not fail is only because the kernel has yet to protect location zero.

Quoted 4 lines

But init_timers() directly calls timer_cpu_notify(), which directly calls init_timers_cpu().

So your patch appears to be a no-op for the boot CPU.

That is correct. The problem is when cpu_init is called for the secondary cpus. It almost immediately calls printk.

...
My comments here are a wonderment if
this is the right thing to do when doing a hot swap of the cpu. I sort of doubt that this is correct.

Quoted 14 lines · george anzinger

Quoted 12 lines · Andrew Morton

Quoted 9 lines · george anzinger

On SMP systems the timer list init is done by way of a cpu_notifier call. This has two problems:

1.) Timers are started WAY before the cpu_notifier call chain is executed. In particular the console blanking timer is deleted and inserted every time printk() is called. That this does not fail is only because the kernel has yet to protect location zero.

Quoted 7 lines

Quoted 4 lines

But init_timers() directly calls timer_cpu_notify(), which directly calls init_timers_cpu().

So your patch appears to be a no-op for the boot CPU.

That is correct. The problem is when cpu_init is called for the secondary cpus. It almost immediately calls printk.

OK. So until that CPU sees it bit come on in smp_commenced_mask() it's not allowed to assume that it is running yet.

Quoted 4 lines

...
My comments here are a wonderment if
this is the right thing to do when doing a hot swap of the cpu. I sort of doubt that this is correct.

I agree. And from a quick read it does seem that ia32 is doing the right thing apart from calling printk.

I don't think we should make changes to the timer code because who knows what assumptions other console drivers could be making?

I don't think we should carefully remove all printk() calls because printk() is supposed to be robust, and always callable.

The logical thing is to implement arch_consoles_callable(). Does this look workable?

kernel/printk.c Fri Dec 20 11:33:14 2002 renamed from kernel/printk.c~ga Fri Dec 20 11:32:05 2002 +5 -1

@@ -43,7 +43,11 @@
 #define LOG_BUF_MASK   (LOG_BUF_LEN-1)
 
 #ifndef arch_consoles_callable
-#define arch_consoles_callable() (1)
+/*
+ * Some console drivers may assume that per-cpu resources have been allocated.
+ * So don't allow them to be called by this CPU until it is officially up.
+ */
+#define arch_consoles_callable() cpu_online(smp_processor_id())
 #endif
 
 /* printk's without a loglevel use this.. */

...

Quoted 2 lines

The logical thing is to implement arch_consoles_callable(). Does this look workable?

I am not sure. The first question is when does the online bit get set for cpu 0.

The next is that it does inhibit a
rather large block of printks. Is this ok?

Mind you, I have not tried it yet...

...
I am not sure. The first question is when does the online bit get set for cpu 0. The next is that it does inhibit a rather large block of printks. Is this ok?

Quoted 6 lines · george anzinger

...
I am not sure. The first question is when does the online bit get set for cpu 0. The next is that it does inhibit a rather large block of printks. Is this ok?

The boot cpu is set online extremely late. Strangely late. Why is this?

How about something like the below? We mark the boot cpu online in generic code as soon as it has initialised its per-cpu storage (seems appropriate?)

This will then allow that cpu to actually start calling into console drivers, if they have been registered. If those drivers do a mod_timer() (as the vga console does) then that will work OK.

Secondary CPUs also are not marked online until their per-cpu storage is initialised, and their notifiers have been called.

If a non-online cpu calls printk, its output will be buffered. It will be displayed by the next call to printk by an online CPU.

(The patch needs set_cpu_online()/set_cpu_possible() implementations done for the other architectures)

 arch/i386/kernel/smpboot.c |    5 -----
 include/asm-i386/smp.h     |   29 +++++++++++++++++++++++------
 include/linux/smp.h        |    7 ++++++-
 init/main.c                |    8 ++++++++
 kernel/printk.c            |    6 +++++-
 5 files changed, 42 insertions(+), 13 deletions(-)

kernel/printk.c Sat Dec 21 23:27:08 2002 renamed from kernel/printk.c~ga Sat Dec 21 23:27:08 2002 +5 -1

@@ -43,7 +43,11 @@
 #define LOG_BUF_MASK   (LOG_BUF_LEN-1)
 
 #ifndef arch_consoles_callable
-#define arch_consoles_callable() (1)
+/*
+ * Some console drivers may assume that per-cpu resources have been allocated.
+ * So don't allow them to be called by this CPU until it is officially up.
+ */
+#define arch_consoles_callable() cpu_online(smp_processor_id())
 #endif
 
 /* printk's without a loglevel use this.. */

init/main.c Sat Dec 21 23:55:00 2002 renamed from init/main.c~ga Sat Dec 21 23:48:03 2002 +8 -0

@@ -361,6 +361,14 @@ asmlinkage void __init start_kernel(void
        printk(linux_banner);
        setup_arch(&command_line);
        setup_per_cpu_areas();
+
+       /*
+        * Once the boot CPU's per-cpu memory is set up it may be considered
+        * online.  This is mainly to turn on printk output.
+        */
+       set_cpu_online(smp_processor_id());
+       set_cpu_possible(smp_processor_id());
+
        build_all_zonelists();
        page_alloc_init();
        printk("Kernel command line: %s\n", saved_command_line);

include/asm-i386/smp.h Sat Dec 21 23:54:16 2002 renamed from include/asm-i386/smp.h~ga Sat Dec 21 23:49:43 2002 +23 -6

@@ -80,15 +80,32 @@ extern volatile int logical_apicid_to_cp
 
 extern volatile unsigned long cpu_callout_map;
 
-#define cpu_possible(cpu) (cpu_callout_map & (1<<(cpu)))
-#define cpu_online(cpu) (cpu_online_map & (1<<(cpu)))
+static inline int cpu_possible(int cpu)
+{
+       return cpu_callout_map & (1 << cpu);
+}
+
+static inline void set_cpu_possible(int cpu)
+{
+       cpu_callout_map |= (1 << cpu);
+}
+
+static inline int cpu_online(int cpu)
+{
+       return cpu_online_map & (1 << cpu);
+}
+
+static inline void set_cpu_online(int cpu)
+{
+       cpu_online_map |= (1 << cpu);
+}
 
-extern inline unsigned int num_online_cpus(void)
+static inline unsigned int num_online_cpus(void)
 {
        return hweight32(cpu_online_map);
 }
 
-extern inline int any_online_cpu(unsigned int mask)
+static inline int any_online_cpu(unsigned int mask)
 {
        if (mask & cpu_online_map)
                return __ffs(mask & cpu_online_map);

@@ -96,13 +113,13 @@ extern inline int any_online_cpu(unsigne
        return -1;
 }
 
-static __inline int hard_smp_processor_id(void)
+static inline int hard_smp_processor_id(void)
 {
        /* we don't want to mark this access volatile - bad code generation */
        return GET_APIC_ID(*(unsigned long *)(APIC_BASE+APIC_ID));
 }
 
-static __inline int logical_smp_processor_id(void)
+static inline int logical_smp_processor_id(void)
 {
        /* we don't want to mark this access volatile - bad code generation */
        return GET_APIC_LOGICAL_ID(*(unsigned long *)(APIC_BASE+APIC_LDR));

arch/i386/kernel/smpboot.c Sat Dec 21 23:54:43 2002 renamed from arch/i386/kernel/smpboot.c~ga Sat Dec 21 23:52:51 2002 +0 -5

@@ -992,11 +992,6 @@ static void __init smp_boot_cpus(unsigne
        printk("CPU%d: ", 0);
        print_cpu_info(&cpu_data[0]);
 
-       /*
-        * We have the boot CPU online for sure.
-        */
-       set_bit(0, &cpu_online_map);
-       set_bit(0, &cpu_callout_map);
        boot_cpu_logical_apicid = logical_smp_processor_id();
        map_cpu_to_boot_apicid(0, boot_cpu_apicid);
 

include/linux/smp.h Sun Dec 22 00:30:15 2002 renamed from include/linux/smp.h~ga Sun Dec 22 00:29:05 2002 +6 -1

@@ -94,7 +94,12 @@ static inline void smp_send_reschedule_a
 #define cpu_online(cpu)                                ({ BUG_ON((cpu) != 0); 1; })
 #define num_online_cpus()                      1
 #define num_booting_cpus()                     1
-#define cpu_possible(cpu)                              ({ BUG_ON((cpu) != 0); 1; })
+#define cpu_possible(cpu)                      ({ BUG_ON((cpu) != 0); 1; })
+static inline void set_cpu_online(int cpu)
+{}
+static inline void set_cpu_possible(int cpu)
+{}
+
 
 struct notifier_block;
 

_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

The boot cpu is set online extremely late. Strangely late. Why is this?

How about something like the below? We mark the boot cpu online in generic code as soon as it has initialised its per-cpu storage (seems appropriate?)

This will then allow that cpu to actually start calling into console drivers, if they have been registered. If those drivers do a mod_timer() (as the vga console does) then that will work OK.