[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YzcvvPargLYDHhgq@alley>
Date: Fri, 30 Sep 2022 20:04:44 +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,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk 06/18] printk: Protect [un]register_console() with
a mutex
On Fri 2022-09-30 16:22:30, John Ogness wrote:
> On 2022-09-30, Petr Mladek <pmladek@...e.com> 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
Powered by blists - more mailing lists