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;
From:   John Ogness <john.ogness@linutronix.de>
To:     Petr Mladek <pmladek@suse.com>
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 v3 14/15] printk: extend console_lock for proper
 kthread support
In-Reply-To: <Yma71x2p10d6yOLU@alley>
References: <20220419234637.357112-1-john.ogness@linutronix.de>
 <20220419234637.357112-15-john.ogness@linutronix.de>
 <20220421124119.GB11747@pathway.suse.cz>
 <875yn2h5ku.fsf@jogness.linutronix.de> <YmKnp3Ccu7laW3E4@alley>
 <87o80tp5lv.fsf@jogness.linutronix.de> <YmLGfuSV5u7xp5BZ@alley>
 <877d7gu7yg.fsf@jogness.linutronix.de> <Yma71x2p10d6yOLU@alley>
Date:   Mon, 25 Apr 2022 21:16:37 +0206
Message-ID: <87czh5568i.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 2022-04-25, Petr Mladek <pmladek@suse.com> wrote:
>> Honestly, I would prefer this to what v4 is doing. The only reason
>> CON_THD_BLOCKED is a flag is to save space. But we are only talking
>> about a few bytes being saved. There aren't that many consoles.
>> 
>> It would be a very simple change. Literally just replacing the 3 lines
>> that set/clear CON_THD_BLOCKED and replacing/reordering the 2 lines that
>> check the flag. Then all the READ_ONCE/WRITE_ONCE to @flags could be
>> removed.
>
> I agree that it sounds like the easiest solution for now. If you
> prepare v5 with this change then I push it into linux-next instead
> of v4.

I will send a v5 only for that patch. I will make it a reply to the same
v4 patch.

> Well, I think that we need to make con->lock safe to use in the long
> term. The above workaround in printk_kthread_func() is good enough
> for now because this is the only location where con->lock is taken without
> console_sem. But I am sure that we/people will want to do more
> console-specific operations without console_sem in the future.
>
> IMHO, the only sane approach is to follow the proposed rules:
>
>     + console_lock() will synchronize both global and per-console
>       stuff.
>
>     + con->lock will synchronize per-console stuff.
>
>     + con->lock could not be taken alone when the big console_lock()
>       is taken.
>
>
> I currently know only about two solutions:
>
>     1. The nested locking. console_lock() will take console_sem
>        and all con->lock's and will keep them locked.
>
>        It is rather trivial in principle. The problem is lockdep
>        and possible ABBA deadlocks caused by unstable ordering.
>
>
>     2. Create the wrappers around con->lock that will check
>        whether console_sem is taken (con->locked flag).
>
>        It will require additional per-console waitqueue. But all
>        the magic will be hidden in the wrappers.
>
>
> I personally prefer 2nd approach for the long term solution. It might
> look more complicated but it will not break lockdep.

The 2nd approach doesn't break lockdep because it is hiding from it. We
are basically open coding our own blocking lock mechanism that avoids
looking like nested locking.

If using nested locking is the best technical solution, then we should
not let lockdep prevent us from using that solution.

You talk about ABBA deadlocks due to unstable ordering, but I still do
not see how that is possible with the @console_sem protection. Unless
you are talking about some code that is trying to lock multiple consoles
at once without taking the console_lock. That would need to be forbidden
by the API.

My main concern with nested locking (aside from lockdep complexities) is
the console suspension. That is a bizarre state where @console_sem is
released even though the console is in a type of frozen state. It is
tricky/messy, especially since the kthreads need to remain silent in
this state. I suppose the kthreads would also need to be unlocked in
suspend_console() and the kthreads would respect @console_suspended and
go back to sleep (similar to how CON_THD_BLOCKED is now).

John