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

[Petr Mladek]

On Wed 2022-03-02 15:27:50, John Ogness wrote:
> I have taken some time to investigate the percpu implementation so that
> I could provide clear answers here.
> 
> On 2022-02-15, Petr Mladek <pmladek@xxxxxxxx> wrote:
> > I am not 100% sure. But it seems that static per-CPU variables might
> > actually be used since the boot.
> 
> You are correct, but until per-cpu areas are setup, they all point to
> CPU0. Normally that is not a problem since usually code is using
> this_cpu_ptr() (which will always be the CPU0 in early boot), rather
> than specifying foreign CPUs with per_cpu_ptr().
> 
> > Most likely, only dynamically allocated per-cpu variables have to wait
> > until the per-cpu areas are initialized.
> 
> It is also important to wait if data will be stored that is no longer
> valid after per-cpu areas are setup. setup_per_cpu_areas() copies the
> static CPU0 per-cpu value to all the newly setup per-cpu areas.
> 
> This is actually the cause for the mystery [0] of failing irq_work when
> printk_deferred was added with commit
> 15341b1dd409749fa5625e4b632013b6ba81609b ("char/random: silence a
> lockdep splat with printk()".

Just for record, the right commit ID in the mainline is
1b710b1b10eff9d466. It used printk_deferred() in _warn_unseeded_randomness():

--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1687,8 +1687,9 @@ static void _warn_unseeded_randomness(const char *func_name, void *caller,
 	print_once = true;
 #endif
 	if (__ratelimit(&unseeded_warning))
-		pr_notice("random: %s called from %pS with crng_init=%d\n",
-			  func_name, caller, crng_init);
+		printk_deferred(KERN_NOTICE "random: %s called from %pS "
+				"with crng_init=%d\n", func_name, caller,
+				crng_init);
 }
 
 /*



> By avoiding queueing irq_work before setup_per_cpu_areas(), we correctly
> avoided this problem. (I considered sending a patch so that
> irq_work_claim() will fail if a global @percpu_complete is not yet
> set.

That is a great TODO :-)

> But for now, our set_percpu_data_ready() solution is at least good
> enough for the printk subsystem.)

Yes but it is most likely not needed for CON_ANYTIME consoles,
see below.

> > We should probably revisit the code and remove the fallback to
> > normal static variables.
> 
> Definitely. Now it is clear that @printk_count and @printk_count_nmi do
> not need early variants.

Note the ordering:

asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
{
[...]
	early_security_init();
[...]
	setup_per_cpu_areas();
[...]
	parse_early_param();
	after_dashes = parse_args("Booting kernel",


My guess is that _warn_unseeded_randomness() was called from a code
before early_security_init(). It was bad because per-CPU variables
were not ready yet.

The early consoles are enabled by parse_early_param(). It happens
after setup_per_cpu_areas(). It means that all console drivers
should be on the safe side.

Best Regards,
Petr