Re: [PATCH printk v1 11/13] printk: reimplement console_lock for proper kthread support

[John Ogness <john.ogness@linutronix.de>]

On 2022-02-22, Petr Mladek <pmladek@suse.com> wrote:
>> >> With non-threaded console printers preemption is disabled while
>> >> holding the console lock in order to avoid the situation where the
>> >> console printer is scheduled away and no other task can lock the
>> >> console (for printing or otherwise).
>> >
>> > I guess that you are talking about that vprintk_emit() disables
>> > the preemtion. console_lock() does not disable preemption.
>> 
>> Yes. I did not mean to imply that console_lock() is disabling
>> preemption. But preemption is disabled at some point _within_ the
>> console_lock()/console_unlock() window. I consider
>> console_lock()/console_unlock() to be the main API functions for direct
>> printing and I wanted to point out that calling that pair will cause
>> preemption disabling.
>
> But console_unlock() should not call consoles when kthreads are
> usable. It should just wake up them. The kthreads might still
> use console_lock() and __console_unlock().

I am only talking about when direct printing should be performed. Direct
printing depends on more than just the kthreads. See
allow_direct_printing().

> I do not understand why this should be a reason to introduce
> per-console locks.

I see your point and you are correct. The reason for per-console locks
is purely to allow the console printers to run in parallel.

>> >> Disabling preemption is necessary because the console lock is
>> >> implemented purely as a semaphore, which has no owner.
>> >
>> > I do not understand this explanation.
>> >
>> > The preemtion was added into vprintk_emit() by the commit
>> > fd5f7cde1b85d4c8e09c ("printk: Never set console_may_schedule
>> > in console_trylock()").
>> >
>> > It is not really necessary. It actually increases the risk of
>> > softlockups.  The motivation was to increase the chance that
>> > messages will get printed. Nobody prints the messages when the
>> > owner is sleeping.  We hoped that the console owner steeling will
>> > be efficient enough to prevent the softlockups.
>> 
>> Yes, I am referring to this case where the context taking the
>> semaphore goes to sleep. If another context tries to take the
>> semaphore, the scheduler does not know which task it can schedule to
>> resolve the situation because there is no true lock contention.
>> 
>> With a mutex there is an owner. When another task tries to lock a
>> mutex, the scheduler knows which task must be scheduled to resolve
>> this true lock contention. (There are also other benefits relating to
>> priority inheritance, but I chose not to mention this.)
>
> This sounds interesting. Does scheduler wake up or prioritize
> mutex owners?

Sorry, the only example of this is priority inheritance. But for
non-PREEMPT_RT there is no priority inheritance. The lock would need to
be an rtmutex for this ability, which probably doesn't make sense for
printk.

And besides, the priority inheritance only works in 1 direction. With
the use of the flags approach, there is no way for printing kthreads to
boost console_lock owners. So even mentioning priority inheritance is
misleading.

> I do not see this in the code. There is the optimistic spin but
> it is used only when the current owner is just running on another CPU.
>
> Again, I do not see how this could be a motivation to introduce
> per-console mutexes.

You are correct.

>> >> Like non-threaded console printers, kthread printers use the
>> >> console lock to synchronize during printing. However, since they
>> >> use console_lock() instead of a best-effort console_trylock(), it
>> >> is not possible to disable preemption upon locking.
>> >   ^^^^^^^^^^^^^^^
>> >
>> > IMHO, it is possible to disable preemtion. But we propably do
>> > not want to do it, especially on RT.
>> 
>> As stated in the commit message of fd5f7cde1b85 ("printk: Never set
>> console_may_schedule in console_trylock()"), enabling preemption is
>> "a mistake". That is why I said "not possible". Perhaps I should say
>> "bad idea".
>
> "bad idea" is better. I am sorry that I am too picky. But the disabled
> preemption was a big mystery when I started working on printk code
> ages ago. And it was hard to find any explanation if it was required
> or just nice to have. Clear commit message or comment would have
> helped a lot.

AFAICT the issue only applies to the printk() call chain, because the
caller may be holding other locks that are critical to the system. (In
Tetsuo's case [0], it was @oom_lock.)

[0] https://lore.kernel.org/linux-mm/201603022101.CAH73907.OVOOMFHFFtQJSL@I-love.SAKURA.ne.jp/

That means the reasoning in my commit message about kthreaded printing
and disabling preemption is moot. I will clean that up. Thanks for
pointing out my handwaving.

> I do not see how the per-console mutexes help with preemtion:
>
>    + console_lock()/console_unlock() is fast when kthreads are usable
>
>    + console_lock()/console_unlock() behaves the same as before
>      when kthreads are not usable, for example, during early boot,
>      or panic.
>
> BTW: The possibility to process printk kthreads in parallel is
>      a good enough reason from my POV. But I would still like
>      to understand your primary motivation. There must be some
>      important aspect that I do not understand. And it would be
>      great to document it.

You are right. The v2 commit message will focus on:

- the motivation for per-console locks is parallel printing

- explain about how disabling preemption is only necessary for direct
  printing via printk() because the caller may be holding
  system-critical and/or timing-sensitive locks (and also to allow the
  console owner/waiter logic to work correctly)

- correctly clarifying why the various types
  (semaphore/mutex/flag/atomic) were chosen to implement the printing
  sychronization between atomic-direct, non-atomic-direct, and kthreads
  (and I will explicitly remind the audience that mutex_trylock() cannot
  be used in atomic context)

Thanks, Petr!

John