Messages in this thread | | | Date | Tue, 15 Feb 2022 11:38:48 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH printk v1 03/13] printk: use percpu flag instead of cpu_online() |
| |
On Mon 2022-02-14 16:35:18, Sergey Senozhatsky wrote: > On (22/02/11 17:05), Petr Mladek wrote: > > On Mon 2022-02-07 20:49:13, John Ogness wrote: > [..] > > The problem is the commit ac25575203c11145066ea ("[PATCH] CPU hotplug > > printk fix"). It suggests that per-CPU data of slab are freed during > > hotplug. > > > > There are many other things that are manipulated during cpu hotplug. > > And there are the two notifiers "printk:dead" and "printk:online", > > see printk_late_init(). Maybe, we should use them to decide whether > > the non-trivial consoles are callable during CPU hotplug. > > Great findings. Looks like we only set __printk_percpu_data_ready to > true and never set it back to false, relying on cpu_online() in such > cases. But here's the thing: we have printk_percpu_data_ready() in > __printk_recursion_counter() and in wake_up_klogd() and in > defer_console_output(), but why we never check __printk_percpu_data_ready > in __down_trylock_console_sem()/__up_console_sem() and more importantly > in console_trylock_spinning() and those do access this_cpu() in printk safe > enter/exit. Am I missing something?
Great point!
I am not 100% sure. But it seems that static per-CPU variables might actually be used since the boot.
This is from mm/percpu.c
* There is special consideration for the first chunk which must handle * the static percpu variables in the kernel image as allocation services * are not online yet. In short, the first chunk is structured like so: * * <Static | [Reserved] | Dynamic> * * The static data is copied from the original section managed by the * linker. The reserved section, if non-zero, primarily manages static * percpu variables from kernel modules. Finally, the dynamic section * takes care of normal allocations.
I thought that it might work only for CPU0. But it seems that it probably works for each possible cpu, see:
bool __is_kernel_percpu_address(unsigned long addr, unsigned long *can_addr) { #ifdef CONFIG_SMP const size_t static_size = __per_cpu_end - __per_cpu_start; void __percpu *base = __addr_to_pcpu_ptr(pcpu_base_addr); unsigned int cpu;
for_each_possible_cpu(cpu) { void *start = per_cpu_ptr(base, cpu); void *va = (void *)addr;
if (va >= start && va < start + static_size) { [...] }
and
/** * is_kernel_percpu_address - test whether address is from static percpu area * @addr: address to test * * Test whether @addr belongs to in-kernel static percpu area. Module * static percpu areas are not considered. For those, use * is_module_percpu_address(). * * RETURNS: * %true if @addr is from in-kernel static percpu area, %false otherwise. */ bool is_kernel_percpu_address(unsigned long addr) { return __is_kernel_percpu_address(addr, NULL); }
Most likely, only dynamically allocated per-cpu variables have to wait until the per-cpu areas are initialized.
This might explain why there is no generic are_per_cpu_variables_ready() callback.
We should probably revisit the code and remove the fallback to normal static variables.
Best Regards, Petr
| |