Messages in this thread | | | Date | Fri, 30 Sep 2022 20:04:44 +0200 | From | Petr Mladek <> | 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@suse.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
| |