Messages in this thread | | | From | John Ogness <> | Subject | Re: hostile takeover: Re: [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic | Date | Tue, 29 Aug 2023 12:28:10 +0206 |
| |
On 2023-08-09, Petr Mladek <pmladek@suse.com> wrote: >> Add per console acquire/release functionality. The console 'locked' >> state is a combination of multiple state fields: >> >> - Hostile takeover >> >> The new owner takes the console over without 'req_prio' >> handshake. >> >> This is required when friendly handovers are not possible, >> i.e. the higher priority context interrupted the owning >> context on the same CPU or the owning context is not able >> to make progress on a remote CPU. > > I always expected that there would be only one hostile takeover. > It would allow to take the lock a harsh way when the friendly > way fails.
You are referring to the hostile takeover when unsafe. A hostile takeover when safe is still considered a hostile takeover. (At least, until now that is how it was considered. More below...)
>> All other policy decisions have to be made at the call sites: >> >> - What is marked as an unsafe section. >> - Whether to spinwait if there is already an owner. >> - Whether to attempt a hostile takeover when safe. >> - Whether to attempt a hostile takeover when unsafe. > > But there seems to be actually two variants. How they are > supposed to be used, please? > > I would expect that a higher priority context would always > be able to takeover the lock when it is in a safe context. > > By other words, "hostile takeover when safe" would be > the standard behavior for context with a higher priority.
The difference is that with "hostile takeover when safe" there is a foreign CPU that still thinks it has the lock, even though it does not.
> By other words, the difference between normal takeover and > "hostile takeover when safe" is that the 1st one has to > wait until the current owner calls nbcon_enter_unsafe(). > But the result is the same. The current owner might > prematurely end after calling nbcon_enter_unsafe(). > > Maybe, this another relic from the initial more generic approach?
I suppose so. But then why not try the "hostile takeover when safe" first and only use the handover request as a fallback? Like this:
1. try direct 2. try hostile takeover when safe 3. try handover 4. try hostile takeover when unsafe
Then we should remove the "hostile" label for #2 and just call it:
1. try direct 2. try safe takeover 3. try handover 4. try hostile takeover
(I guess this is how you imagined things should be.)
>> +/** >> + * struct nbcon_context - Context for console acquire/release >> + * @console: The associated console >> + * @spinwait_max_us: Limit for spinwait acquire >> + * @prio: Priority of the context >> + * @unsafe: This context is in an unsafe section > > This seems to be an input value for try_acquire(). It is > controversial. > > I guess that it removes the need to call nbcon_enter_unsafe() > right after try_acquire_console().
Yes. It removes the "safe window" between try_acquire_console() and nbcon_enter_unsafe() by allowing to acquire and mark unsafe atomically. For example, this would be useful for the port_lock() wrapper we discussed. But it is not strictly necessary. I can remove it.
Below I answer your comments anyway.
> Hmm, this semantic is problematic: > > 1. The result would be non-paired enter_unsafe()/exit_unsafe() > calls.
The paired calls are either:
try_acquire(unsafe=1) -> release()
or
try_acquire(unsafe=0) + enter_unsafe() -> exit_unsafe() + release()
> 2. I would personally expect that this is an output value > set by try_acquire() so that the context might know > how it got the lock.
The state structure is used to communicate this information.
> 3. Strictly speaking, as an input value, it would mean that > try_acquire() is called when the console is in "unsafe" state. > But the caller does not know in which state the console > is until it acquires the lock.
This is not about the current state. The calling _context_ wants to set a certain state. The caller wants to acquire the lock and atomically mark this as the beginning of an unsafe section. But as I wrote, this is not strictly necessary. Only an efficient convenience.
> For me it is not easy to remember which permutation is used where ;-) > It would be easier if we remove the "hostile when safe" variant. > Then 3 variables might be enough. I suggest something like: > > state.unsafe Console is busy and takeover is not safe. > > state.unsafe_takeover A hostile takeover in an unsafe state happened > in the past. The console can't be safe until > re-initialized. > > ctxt.allow_unsafe_takeover Allow hostile takeover even if unsafe. > Can be used only with PANIC prio. Might cause > a system freeze when the console is used later.
Since "safe takeovers" should be handled equivalent to "handovers" then I agree with your suggestion.
John
| |