lkml.org 
[lkml]   [2022]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: two locations: was: Re: [PATCH printk v1 03/13] printk: use percpu flag instead of cpu_online()
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

\
 
 \ /
  Last update: 2022-03-08 17:11    [W:0.195 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site