Messages in this thread | | | From | John Ogness <> | Subject | Re: two locations: was: Re: [PATCH printk v1 03/13] printk: use percpu flag instead of cpu_online() | Date | Wed, 02 Mar 2022 15:55:23 +0106 |
| |
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.
In the current implementation of printk(), it would be odd to do this conditional check (perhaps by passing @do_cond_resched to can_use_console()). But my series does significant refactoring and actually does need to distinguish between console_lock() and console_trylock() due to the kthreads and supporting the handover. So it should work well that the cpu_online() check for !CON_ANYTIME is only performed when !preemptible.
Regardless, my v2 will keep cpu_online() checks since they are necessary for hotplug support.
John
| |