Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Sep 2022 20:04:44 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH printk 06/18] printk: Protect [un]register_console() with
 a mutex
Message-ID: <YzcvvPargLYDHhgq@alley>
References: <20220924000454.3319186-1-john.ogness@linutronix.de>
 <20220924000454.3319186-7-john.ogness@linutronix.de>
 <YzMT27FVllY3u05k@alley>
 <87mtajkqvu.fsf@jogness.linutronix.de>
 <YzW9ExRVjv6PzvWR@alley>
 <Yza1wxVcH2bsITcs@alley>
 <87leq1uev5.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87leq1uev5.fsf@jogness.linutronix.de>
Precedence: bulk

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