Date: Wed, 19 Jun 2024 17:13:04 +0200
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH printk v2 09/18] printk: nbcon: Start printing threads

On Tue 2024-06-18 17:34:22, Petr Mladek wrote:
> On Tue 2024-06-04 01:30:44, John Ogness wrote:
> > If there are no boot consoles, the printing threads are started
> > in early_initcall.
> > 
> > If there are boot consoles, the printing threads are started
> > after the last boot console has unregistered. The printing
> > threads do not need to be concerned about boot consoles because
> > boot consoles cannot register once a non-boot console has
> > registered.
> > 
> > Until a printing thread of a console has started, that console
> > will print using atomic_write() in the printk() caller context.
>
> > --- a/kernel/printk/nbcon.c
> > +++ b/kernel/printk/nbcon.c
> > @@ -1568,6 +1570,19 @@ void nbcon_kthread_create(struct console *con)
> >  	sched_set_normal(con->kthread, -20);
> >  }
> >  
> > +static int __init printk_setup_threads(void)
> > +{
> > +	struct console *con;
> > +
> > +	console_list_lock();
> > +	printk_threads_enabled = true;
> 
> What is the actual meaning of the variable?
> 
> Does it mean that kthreads can be created? (can be forked?)
> 
> It does not guarantee that the kthreads will be running.
> They might still get blocked by a boot console.

I have investigated it more. And it looks like the variable
means that the system is initialized enough to support
the kthreads.

It is similar to the variable __printk_percpu_data_ready.

I am not sure if it would have helped to prevent the confusion
but I would suggest to rename it and add a comment:

/*
 * The graphics console drivers are going to use workqueues.
 * This variable is set to true when they are safe to use.
 */
static bool __printk_threads_ready __ro_after_init;

> 
> > +	for_each_console(con)
> > +		nbcon_kthread_create(con);

Also we should make it more clear when this function is supposed
to succeed. I mean:

	if (have_nbcon_console && !have_boot_console)
		for_each_console(con)
			nbcon_kthread_create(con);

And similar in unregister_console()

	/*
	 * When the last boot console unregisters, start up the
	 * printing threads.
	 */
	if (is_boot_con && have_nbcon_console && !have_boot_console) {
		for_each_console(c)
			nbcon_kthread_create(c);
	}

The same condition (have_nbcon_console && !have_boot_console)
is used on many other locations.

In another mail, I suggested to define a macro for this condition:

    #define printing_via_context_safe (have_nbcon_console && !have_boot_console)

It might be even better because it would connect the related
code via cscope.

Hmm, the "_safe" suffix might be confusing in this context.
A better name might be "printing_via_nbcon_context" so that
we have:

/* Printing serialized by console_lock dance is needed. */
#define printing_via_unlock (have_legacy_console || have_boot_console)

/* Printing serialized by nbcon console context is needed and safe. */
#define printing_via_nbcon_context (have_nbcon_console && !have_boot_console)

> > +	console_list_unlock();
> > +	return 0;
> > +}
> > +early_initcall(printk_setup_threads);
> > +
> >  /**
> >   * nbcon_alloc - Allocate buffers needed by the nbcon console
> >   * @con:	Console to allocate buffers for
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2397,6 +2400,7 @@ asmlinkage int vprintk_emit(int facility, int level,
> >  		 * consoles cannot print simultaneously with boot consoles.
> >  		 */
> >  		if (is_panic_context ||
> > +		    !printk_threads_enabled ||
> >  		    (system_state > SYSTEM_RUNNING)) {
> >  			nbcon_atomic_flush_pending();
> 
> IMHO, this is not safe. nbcon_atomic_flush_pending()
> is synchronized only via the nbcon console context.
> It means that there might be races with boot consoles.
> 
> Another problem is the meaning of @printk_threads_enabled variable.
> It does not guarantee that the kthreads are running. They might
> still be blocked by boot consoles.
> 
> 
> BTW: The same problem might have the system_state > SYSTEM_RUNNING.
>      The boot consoles are removed only when the preferred console
>      is registered and "keep_bootcon" is not set.
> 
>      Note that printk_late_init() unregisters the boot consoles
>      only when they are using a code in the init sections.
> 
> >  		}

I take it back. We are on the safe side because the above code is
called only when

	if (have_nbcon_console && !have_boot_console)

This is why I suggest to use the same condition around the code
starting the kthreads. It might help to create the mental
model.

Best Regards,
Petr

Powered by blists - more mailing lists