Re: [PATCH printk 06/18] printk: Protect [un]register_console() with a mutex

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

On 2022-09-29, Petr Mladek <pmladek@suse.com> wrote:
> It is the legacy mode that tries to print to the consoles immediately.
> I am not sure if we could _ever_ remove this mode.

We are not trying to remove this mode. We are trying to introduce a new
mode. Once all the console drivers have moved over to the new mode, the
old mode can disappear. If some console drivers never move over, the old
mode can hang around.

It is important to understand that we are not trying to change the old
mode. This was our big mistake leading to the 5.19-revert.

> And it is most likely the main reason why semaphore is used instead
> of a mutex:
>
>     + printk() can be called in atomic context
>
>     + also there is the console_trylock_spinning() trick that allows
>       to transfer the semaphore to another owner without locking.
>
> Do you see any RT-friendly solution for the legacy mode, please?

No. Legacy mode will never work for RT because the console drivers are
using spinlocks, which for RT requires that preemption is enabled.

> Anyway, some lock will still be needed to synchronize the list.
> But could it be mutex? What about the legacy mode of printk_emit()?

For list updates a mutex is fine. All list updates already require
may_sleep contexts. For just iterating the list, SRCU is fine.

But we really need an atomic variable (or separate data-race bools) for
the properties that are not immutable. AFAIK this is only CON_ENABLED
and CON_CONSDEV (and I seriously question the usefulness/correctness of
CON_CONSDEV). If console_is_enabled() could be safely called without a
lock, neither console_lock nor console_list_lock would be needed to
safely iterate and act on the console list.

The NOBKL consoles (not included in this series) use a separate atomic
state variable to handle this. Perhaps the legacy consoles could
(mis)use that variable so that CON_ENABLED is atomic for them as well.

>> Yes! One of the main points of this final phase of the rework is to
>> remove console_sem usage (for NOBKL consoles). If a system is running
>> with only NOBKL consoles registered, ideally that system should never
>> call console_lock()/console_trylock(). Once all drivers have
>> converted over to the NOBKL interface, console_sem will serve no
>> purpose for the printk and console frameworks, so it can be removed.
>
> And even if we convert all console drivers then people still might
> want the legacy mode.

For converted drivers there is no use for the pseudo-synchronous legacy
mode. Converted drivers can run in true synchronous mode if the user
wants.

> My understanding is that some atomic consoles would be real hacks.

Well, it is up to the maintainers to make sure they are not real
hacks. We are not mandating that all drivers are converted. But I think
when devs start seeing the benefits of the converted drivers (and will
have many working examples to be inspired by) there will be honest
efforts to correctly convert the driver.

> They might be good enough for panic(). But what about running system.
> It seems that people might want the legacy more even on running
> system. Will it be doable with mutex?

I'm not sure what you mean here, but I think you are referencing
situations that are not valid. Either drivers are legacy (and continue
using the BKL) or they are correctly converted to the new atomic/thread
model (and have nothing to do with the BKL).

There will be some exceptions (such as fbdev), which is why we are also
considering special alternatives for this class of drivers (such as BSoD
splash on panic, rather than an atomic console).

> I would really like to avoid state where we have two locks (semaphore
> and mutex) serializing the same thing (console list).

I understand. I will look into this more closely. But it may just mean
adding comments above each console_lock() to say:

1. that it is being using to stop all console printing

2. why all console printing needs to stop

Notice that the above list does not include "provide synchronization for
the console list".

John