Re: [PATCH printk 06/18] printk: Protect [un]register_console() with a mutex

[Petr Mladek]

On Fri 2022-09-30 16:22:30, John Ogness wrote:
> On 2022-09-30, Petr Mladek <pmladek@xxxxxxxx> wrote:
> > You know, the more locks we have, the bigger is the risk of
> > deadlocks, and the more hacks would be needed in
> > console_flush_on_panic(). And I am afraid
> > that console_lock() will be with us for many years and
> > maybe forever.
> 
> Sure. Removing console_lock() will be a long battle involving many
> drivers. I am not trying to fight that battle right now. I just want
> console_lock() out of the way of NOBKL consoles.

There is some misunderstanding. I am going to think more about your
arguments over the weekend.

But maybe, the above is important. You want to get cosole_lock() out
of the way of NOBLK consoles. What does it exactly mean, please?
What code paths are important to achieve this?

>From my POV, the most important code path is the kthread. But it
should use SRCU. I mean that the kthread will take neither
cosnole_lock() nor console_list_lock().

Is there any other code path where console_list_lock() will help
you to get console_lock() out of the way?


>From my POV, the proposed code does:

register_console()
{
	console_list_lock();
	console_lock();

	/* manipulate struct console and the console_list */

	console_unlock();
	console_list_unlock();
}

register_console()
{
	console_list_lock();
	console_lock();

	/* manipulate struct console and the console_list */

	console_unlock();
	console_list_unlock();
}

printk_kthread()
{
	while() {
		srcu_read_lock();

		if (read_flags_srcu())
		     /* print line */

		srcu_read_unlock();
	}
}

vprintk_emit()
{
	/* store message */

	if (do_not_allow_sync_mode)
		return;

	if (console_trylock()) {
		console_flush_all();
		__console_unlock();
	}
}

some_other_func()
{
	console_list_lock();
	/* do something with all registered consoles */
	console_list_unlock();
}

console_flush_all()
{
	do_something_with_all_consoles();
	do_something_else_with_all_consoles();
}

What if?

do_something_with_all_consoles()
{
	console_list_lock();
	/* do something */
	console_list_unlock();
}

Wait, there is a possible ABBA deadlock because
do_something_with_all_consoles() takes console_list_lock()
under console_lock(). And register_console() does it
the other way around.

But it is less obvious because these are different locks.

>From my POV, both locks serialize the same things
(console_list manipulation). SRCU walk should be
enough for most iterations over the list.

And I do not see which code path would really benefit from
having the new console_list_lock() instead of console_lock().

Best Regards,
Petr