Messages in this thread | | | Date | Fri, 12 Apr 2024 11:07:05 +0200 | From | Petr Mladek <> | Subject | Re: [PATCH printk v4 17/27] printk: nbcon: Use nbcon consoles in console_flush_all() |
| |
On Thu 2024-04-11 16:14:58, Petr Mladek wrote: > On Wed 2024-04-03 00:17:19, John Ogness wrote: > > Allow nbcon consoles to print messages in the legacy printk() > > caller context (printing via unlock) by integrating them into > > console_flush_all(). The write_atomic() callback is used for > > printing. > > Hmm, this patch tries to flush nbcon console even in context > with NBCON_PRIO_NORMAL. Do we really want this, please? > > I would expect that it would do so only when the kthread > is not working. > > > Provide nbcon_legacy_emit_next_record(), which acts as the > > nbcon variant of console_emit_next_record(). Call this variant > > within console_flush_all() for nbcon consoles. Since nbcon > > consoles use their own @nbcon_seq variable to track the next > > record to print, this also must be appropriately handled. > > I have been a bit confused by all the boolean return values > and what _exactly_ they mean. IMHO, we should make it more > clear how it works when it can't acquire the context. > > IMHO, it is is importnat because console_flush_all() interprets > nbcon_legacy_emit_next_record() return value as @progress even when > there is no guaranteed progress. We just expect that > the other context is doing something. > > It feels like it might get stuck forewer in some situatuon. > It would be good to understand if it is OK or not. > > > Later update: > > Hmm, console_flush_all() is called from console_unlock(). > It might be called in atomic context. But the current > owner might be theoretically scheduled out. > > This is from documentation of nbcon_context_try_acquire() > > /** > * nbcon_context_try_acquire - Try to acquire nbcon console > * @ctxt: The context of the caller > * > * Context: Any context which could not be migrated to another CPU. > > > I can't find any situation where nbcon_context_try_acquire() is > currently called in normal (schedulable) context. This is probably > why you did not see any problems with testing.
> I see 3 possible solutions: > > 1. Enforce that nbcon context can be acquired only with preemtion > disabled.
We actually have to make sure that preemtion is disabled because nbcon_owner_matches() is not reliable after a wakeup.
The context might be taken by a higher priority context then released and then taken by another task on the same CPU as the original sleeping owner. I mean this:
CPU0 CPU1
[ task A ]
nbcon_context_try_acquire() # success with NORMAL prio # .unsafe == false; // safe for takeover
[ schedule: task A -> B ]
WARN_ON() nbcon_atomic_flush_pending() nbcon_context_try_acquire() # success with EMERGENCY prio # .unsafe == false; // safe for takeover
# flushing nbcon_context_release()
nbcon_context_try_acquire() # success with NORMAL prio [ task B ] # .unsafe == false; // safe for takeover
[ schedule: task B -> A ]
nbcon_enter_unsafe() nbcon_context_can_proceed()
BUG: nbcon_context_can_proceed() returns "true" because the console is owned by a context on CPU0 with NBCON_PRIO_NORMAL.
But it should return "false". The console is owned by a context from task B and we do the check in a context from task A.
I guess that most of the current code is safe because, for example:
+ __nbcon_atomic_flush_pending() disables interrupts before acquiring the context
+ nbcon_driver_acquire() is called under spin_lock in the uart_port_*lock() API.
+ Even the nbcon_kthread_func() in the current RT tree acquires the context under con->device_lock(). Where the device_lock() is a spin_lock in the only supported uart serial console.
To be done:
1. We should make this clear:
+ Add either preempt_disable() or cant_sleep() into nbcon_context_try_acquire().
+ Replace cant_migrate() with cant_sleep everywhere
+ Fix/update the documentation
2. We should make sure that the context is acquired for each emitted record separately at least when using the normal priority.
For example, __nbcon_atomic_flush_pending() is wrong from this POV. It is used also from console_unlock(). It should allow to schedule in between the records in this case.
Best Regards, Petr
PS: I am still shaking my head around this. Sigh, I haven't expected such a big "aha moment" at this stage.
| |