Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk v2 1/8] printk: Add non-BKL (nbcon) console basic infrastructure | Date | Mon, 31 Jul 2023 22:45:00 +0206 |
| |
On 2023-07-31, Petr Mladek <pmladek@suse.com> wrote: >>> #define have_serialized_console (have_legacy_console || have_boot_console) >> >> This macro is not about having a serialized console. The first >> sentence in the comment describes it best. It is just to signal if we >> need to do the console lock/unlock dance to generate console output. >> >> Something like "need_bkl_dance" would be a better name, but it >> doesn't sound very technical. > > I see. > > Question: Will console_lock() serialize the early-boot handling > of non-BKL conosles? I mean the direct flush in vprintk_emit().
Initially there will be no nbcon consoles that support CON_BOOT. This means that there are no nbcon consoles in "early boot". The only reason that nbcon consoles and legacy boot consoles would co-exist (aside from the brief moment before boot consoles are unregistered) is if keep_bootcon is used.
As long as a boot console is registered, nbcon consoles are also bound to console_lock() serialization. We have no choice until we can safely link boot consoles to regular consoles.
I think this will be ok for the first release. The 8250 will not become less reliable in early boot. And once the boot console is unregistered, the 8250 nbcon console will be able to fly.
> At lest, the v1 patch set called cons_atomic_flush() in vprintk_emit() > without taking outside console_lock().
Yes. But in the v1 patch set, console_is_usable() returns false for nbcon consoles if there is a boot console registered. So the cons_atomic_flush() in vprintk_emit() would not end up printing anything.
As per your v1 feedback, that check will no longer be in console_is_usable(), but instead will be further out in higher level code.
> If console_lock never serializes non-BKL consoles then I rather would define: > > #define serialized_nbcon (have_nbcon && have_boot_console) > and use: > > + "have_legacy_console" when console lock/unlock dance is neded. > > + "serialize_nbcon" the non-BKL consoles need to be serialized > > IMHO, it will be more clear what is going on.
We have 3 scenarios that I would like to easily identify using global variable(s).
1. There are only nbcon consoles. The console lock never needs to be taken.
2. There are nbcon consoles and regular legacy consoles. The console lock must be taken to serialize only the regular legacy consoles. There are separate code paths (without the console lock) that will take care of nbcon atomic printing and nbcon threaded printing.
3. There are nbcon consoles and boot consoles. The console lock must be taken to serialize all consoles.
Perhaps rather than using 2 booleans and a macro, we just use a single atomic_t that describes the console serialization mode? The effect is the same, but maybe it makes the intention of the code a bit easier to understand?
SERMOD_BOOTCON = 0, SERMOD_WITH_LEGACY = 1, SERMOD_ONLY_NBCON = 2,
Or maybe describe the modes based on their behavior rather than their condition:
SERMOD_ONLY_CONSOLE_LOCK = 0, SERMOD_ALSO_CONSOLE_LOCK = 1, SERMOD_NO_CONSOLE_LOCK = 2,
>>> + Update console_flush_all() description. Mention that it flushes >>> only serialized consoles >> >> Agreed. It is only responsible for bkl dance flushing. > > Will it flush only legacy consoles? Or will it flush also non-BKL > consoles during the early boot?
It will also flush nbcon consoles if a boot console is registered.
> I think that it is wrong even after adding the nbcon check. The code > looks like this at the end of this patchset: > > /* > * If consoles are not usable, it cannot be expected > * that they make forward progress, so only increment > * @diff for usable consoles. > */ > if (!console_is_usable(c)) > continue; > > if (flags & CON_NBCON) > printk_seq = nbcon_seq_read(c); > else if (locked) > printk_seq = c->seq; > else > continue; > > I guess that the "else-continue" path will never happen. But when > it happens then pr_flush() would ignore a usable console and it looks > wrong.
My reason for keeping the "if locked" was to remind the developer that the console lock must be held in order to safely read @console->seq. But you are right that it makes things look awkward. I will just change the code to:
if (flags & CON_NBCON) printk_seq = nbcon_seq_read(c); else printk_seq = c->seq;
There is already a comment at the console_lock() explaining why it is taken. That is enough.
John
| |