Re: two locations: was: Re: [PATCH printk v1 03/13] printk: use percpu flag instead of cpu_online()

[Petr Mladek <pmladek@suse.com>]

On Wed 2022-03-02 15:55:23, John Ogness wrote:
> On 2022-02-16, Petr Mladek <pmladek@suse.com> wrote:
> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index d1b773823d63..b346e60e9e51 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -2577,11 +2577,11 @@ static int have_callable_console(void)
> >>   *
> >>   * Console drivers may assume that per-cpu resources have been allocated. So
> >>   * unless they're explicitly marked as being able to cope (CON_ANYTIME) don't
> >> - * call them until this CPU is officially up.
> >> + * call them until per-cpu resources have been allocated.
> >>   */
> >>  static inline int can_use_console(void)
> >>  {
> >> -	return cpu_online(raw_smp_processor_id()) || have_callable_console();
> >> +	return (printk_percpu_data_ready() || have_callable_console());
> >>  }
> >
> > cpu_online(raw_smp_processor_id()) check is used also in
> > call_console_drivers(). The same logic should be used in both
> > locations.
> >
> > I found this when reviewing 6th patch that replaced both checks
> > with a single one.
> >
> > Note that I am still not sure if this change is correct at all.
> > It will allow to always call the console during CPU hotplug
> > and I am not sure if it is safe. IMHO, it might cause problems when
> > a console driver uses, for example, CPU-bound workqueues.
> 
> You are correct. We must take hotplug into account for !CON_ANYTIME
> consoles. There may be some hotplug callbacks that make memory
> unavailable for the console.
>
> However, I will add the use of printk_percpu_data_ready() in the
> check. !CON_ANYTIME consoles also should not be called until the per-cpu
> areas are ready. For example, it would be bad if a console queued
> irq_work before per-cpu areas are setup (cpu_online() is true during
> this time).
> 
> One of my main concerns was that raw_smp_processor_id() was used for the
> check. It is conceptually wrong to exclude certain consoles based on a
> current CPU when migration is still enabled. I understand that the use
> of can_use_console() is an optimization to avoid doing extra work where
> there are no consoles available. But the task could be preemptible there
> and _conceptually_, could get moved to another CPU before its write()
> callback is called. The cpu_online() check belongs in code where
> preemption is disabled.
> 
> If the context is preemptible, I do not think it will ever see
> !cpu_online(). So I think if the cpu_online() check is limited to
> unlocking when console_trylock() was used, it will be correct.

I investigated the cpu hotplug code and found the following:

1. In the cpu_down() code path, @cpu_online mask is cleared
   by this call chain:

   + take_cpu_down()
     + __cpu_disable()
       + smp_ops.cpu_disable()
	 + native_cpu_disable()   # x86
	   + cpu_disable_common()
	     + remove_cpu_from_maps()
	       + set_cpu_online(cpu, false)

   , where take_cpu_down() is called via:

   + .teardown.single() calback for CPUHP_TEARDOWN_CPU state
     + takedown_cpu()
       + stop_machine_cpuslocked()
         + stop_cpus()
           + __stop_cpus()
             + queue_stop_cpus_work()
	       + cpu_stop_queue_work()

  , which queues the work in cpu_stopper thread that is bound
    to the CPU:

   + cpu_stop_init()
     + smpboot_register_percpu_thread()
       + __smpboot_create_thread()
         + kthread_create_on_cpu()


   Summary: @cpu_online mask is cleared on the affected CPU in
            cpu_stopper thread that is bound to the same CPU.
	    It happens when handling CPUHP_TEARDOWN_CPU.


2. The CPU hotplug states are split into three groups:

     + code running on control CPU (another CPU)
     + low level code running on the hotplugged CPU
     + code running in the hotplug thread on the hotplugged CPU

   It is described in include/linux/cpuhotplug.h:

	/* PREPARE section invoked on a control CPU */
	CPUHP_OFFLINE = 0,
	[...]

	/*
	 * STARTING section invoked on the hotplugged CPU in low level
	 * bringup and teardown code.
	 */
	CPUHP_AP_IDLE_DEAD,
	[...]
	CPUHP_AP_ONLINE,
	CPUHP_TEARDOWN_CPU,

	/* Online section invoked on the hotplugged CPU from the hotplug thread */
	CPUHP_AP_ONLINE_IDLE,
	CPUHP_AP_SCHED_WAIT_EMPTY,
	[...]
	CPUHP_ONLINE,

   , where sched_cpu_wait_empty() is the .teardown.single callback for
     CPUHP_AP_SCHED_WAIT_EMPTY. After this callback, another tasks
     should not be scheduled on this CPU. Any attempt should be
     catched and handled by sched_cpu_dying().

   Note that CPUHP_AP_SCHED_WAIT_EMPTY is called before
   CPUHP_TEARDOWN_CPU when the CPU goes down.

   Summary: @cpu_only mask is cleared for the CPU when other tasks
	    could not longer be sheduled there.


Result: cpu_online(raw_smp_processor_id()) should be safe for our
	purpose. It will return false only the task could not longer
	migrate from the CPU.

	I have to admit that it is far from obvious and tricky like
	hell.


OK, cpu_online(raw_smp_processor_id()) check is not racy. Another
question is whether it is a good check whether the consoles are usable
or not.

I found the following:

1. My understanding is that affinity of IRQs is disabled right after
   clearing @cpu_online mask:

	void cpu_disable_common(void)
	{
		[...]
		remove_cpu_from_maps(cpu);
		[...]
		fixup_irqs();
		[...]
	}

2. Timers must not be used close after clearing @cpu_online mask, see
   see include/linux/cpuhotplug.h:

	/* Must be the last timer callback */
	CPUHP_AP_DUMMY_TIMER_STARTING,
	[...]
	CPUHP_AP_ONLINE,
	CPUHP_TEARDOWN_CPU,


Result: From my POV, cpu_online(raw_smp_processor_id()) marks
	reasonable place in the CPU hotplug code when the conosles
	start/stop being usable.

	But again, it is far from obvious and tricky like hell.


Summary: We need to keep cpu_online(raw_smp_processor_id()) check
	to make the consoles safe during CPU hotplug.

	IMHO, it is not about per-CPU variables. It is more
	about timers, interrupts. The hotplugged CPU is not
	ready call console code at these early hotplug stages.


> Regardless, my v2 will keep cpu_online() checks since they are necessary
> for hotplug support.

Yes, please. We should also somehow document this. But it can be done
separately. It is not necessarily in the scope of your patchset.

Best Regards,
Petr