Re: [PATCH printk v3 2/7] printk: nbcon: Add acquire/release logic

[Petr Mladek]

On Sun 2023-09-03 17:11:34, John Ogness wrote:
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 
> Add per console acquire/release functionality. The console 'locked'
> state is a combination of multiple state fields:
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -47,6 +80,431 @@ static inline bool nbcon_state_try_cmpxchg(struct console *con, struct nbcon_sta
>  	return atomic_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_state), &cur->atom, new->atom);
>  }
>  
> +/**
> + * nbcon_context_try_acquire_direct - Try to acquire directly
> + * @ctxt:	The context of the caller
> + * @cur:	The current console state
> + *
> + * Return:	0 on success and @cur is updated to the new console state.
> + *		Otherwise an error code on failure.
> + *
> + * Errors:
> + *
> + *	-EPERM:		A panic is in progress and this is not the panic CPU
> + *			or this context does not have a priority above the
> + *			current owner or waiter. No acquire method can be
> + *			successful for this context.
> + *
> + *	-EBUSY:		The console is in an unsafe state. The caller should
> + *			try using the handover acquire method.
> + *
> + * The general procedure is to change @nbcon_state::prio from unowned to
> + * owned. Or, if the console is not in the unsafe state, to change
> + * @nbcon_state::prio to a higher priority owner.
> + */
> +static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
> +					    struct nbcon_state *cur)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct console *con = ctxt->console;
> +	struct nbcon_state new;
> +
> +	do {
> +		if (other_cpu_in_panic())
> +			return -EPERM;
> +
> +		if (ctxt->prio <= cur->prio || ctxt->prio <= cur->req_prio)
> +			return -EPERM;
> +
> +		if (cur->unsafe)
> +			return -EBUSY;
> +
> +		/*
> +		 * Direct acquires should never be attempted if
> +		 * an unsafe hostile takeover has ever happened.
> +		 */
> +		WARN_ON_ONCE(cur->unsafe_takeover);

I was a bit confused by this. My first thought was that
this function should never be called after hostile takeover.
And it did not make sense.

But it means that we should never be here if an unsafe hostile
takeover happened. I would slight change the comment:

		/*
		 * The console should never be safe for a direct acquire
		 * if an unsafe hostile takeover has ever happened.
		 */
		WARN_ON_ONCE(cur->unsafe_takeover);

> +		new.atom = cur->atom;
> +		new.prio	= ctxt->prio;
> +		new.req_prio	= NBCON_PRIO_NONE;
> +		new.unsafe	= cur->unsafe_takeover;
> +		new.cpu		= cpu;
> +
> +	} while (!nbcon_state_try_cmpxchg(con, cur, &new));
> +
> +	cur->atom = new.atom;
> +
> +	return 0;
> +}
> +
> +static bool nbcon_waiter_matches(struct nbcon_state *cur, int expected_tag,
> +				int expected_prio)
> +{
> +	/*
> +	 * Since consoles can only be acquired by higher priorities,
> +	 * waiting contexts are uniquely identified by @prio. However,
> +	 * since owners and waiters can unexpectedly change, it is
> +	 * possible that later another waiter appears with the same
> +	 * priority. For this reason @req_tag is also needed.
> +	 *
> +	 * Using the waiting CPU would be better, but there are not enough
> +	 * bits in the state variable for this. Since unexpected waiter
> +	 * changes are rare and them going unnoticed does not cause fatal
> +	 * problems, the tagged bits should be sufficient.
> +	 */
> +
> +	if (cur->req_prio != expected_prio)
> +		return false;
> +
> +	if (cur->req_tag != expected_tag)
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * nbcon_context_try_acquire_requested - Try to acquire after having
> + *					 requested a handover
> + * @ctxt:	The context of the caller
> + * @cur:	The current console state
> + * @req_tag:	The tagged bits used to identify this waiting context

I would suggest to rename @req_tag to @req_cnt or @req_wait_cnt
and describe it:

 * @req_wait_cnt:	Counter to distinguish waiting contexts of
 *			the same priority.

Later update: I would actually remove the tag completely, see below.

> + * Return:	0 on success and @cur is updated to the new console state.
> + *		Otherwise an error code on failure.
> + *
> + * Errors:
> + *
> + *	-EPERM:		A panic is in progress and this is not the panic CPU
> + *			or this context is no longer the waiter. For the
> + *			former case, the caller must carefully remove the
> + *			request before aborting the acquire.
> + *
> + *	-EBUSY:		The console is still locked. The caller should
> + *			continue waiting.
> + *
> + * This is a helper function for nbcon_context_try_acquire_handover().
> + *
> + * The use of tagged bits is to partially deal with the situation that while
> + * this waiting context is in udelay(1):
> + *
> + *   1. another context with higher priority directly takes ownership
> + *   2. the higher priority context releases ownership
> + *   3. a lower priority context takes ownership
> + *   4. a context with the same priority as this context requests ownership
> + *   5. this waiting context finishes udelay(1) and thinks it is the waiter

I was a bit confused by the above description. First, I though that 1, 2,
3, 4 would match the req_tag number.

> + *
> + * To address this rare situation, tagged bits are used so that the waiter
> + * can better identify if it is really the waiter. In the above  example, the
> + * original waiter would use a @req_tag value of 1, whereas the follow-up
> + * waiter would use a @req_tag value of 2. This allows the original waiter to
> + * identify in step 5 that it has been replaced by another waiter.
> + */

I thought about how to avoid the confusion and update the description
of the @req_tag handling. And I am pretty sure that the race could
not happen in the current implementation.

I suggest to remove @req_tag completely and describe the problem
above nbcon_waiter_matches() implementation. Something like:


/**
 * The request owner is well defined by the @req_prio because
 *
 *   1. Only a context with a higher priority could take over the request.
 *   2. There are only three priorities.
 *   3. Only one CPU is allowed to request PANIC priority.
 *   4. Lower priorities are ignored during panic() until reboot.
 *
 * As a result, the following scenario is not possible:
 *
 *   1. Another context with a higher priority directly takes ownership.
 *   2. The higher priority context releases the ownership.
 *   3. A lower priority context takes the ownership.
 *   4. Another context with the same priority as this context
 *	creates a request and starts waiting.
 */
static bool nbcon_waiter_matches(struct nbcon_state *cur,
				 int expected_prio)
{
	return cur->req_prio == expected_prio;
}

> +
> +	if (cur->req_tag != expected_tag)
> +		return false;
> +
> +	return true;
> +}
> +
> +static int nbcon_context_try_acquire_requested(struct nbcon_context *ctxt,
> +					       struct nbcon_state *cur,
> +					       char req_tag)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct console *con = ctxt->console;
> +	struct nbcon_state new;
> +
> +	do {
> +		/*
> +		 * Note: If the acquire is aborted due to a panic CPU,
> +		 * the caller must still remove the request!
> +		 */
> +		if (other_cpu_in_panic())
> +			return -EPERM;
> +
> +		/*
> +		 * If an unsafe hostile takeover has occurred, a handover
> +		 * is no longer possible.
> +		 */
> +		if (cur->unsafe_takeover)
> +			return -EPERM;
> +
> +		/* Is this context still the requester? */
> +		if (!nbcon_waiter_matches(cur, req_tag, ctxt->prio))
> +			return -EPERM;
> +
> +		/* If still locked, caller should continue waiting. */
> +		if (cur->prio != NBCON_PRIO_NONE)
> +			return -EBUSY;
> +
> +		/* Handover acquires should never be attempted if unsafe. */
> +		WARN_ON_ONCE(cur->unsafe);

IMHO, it is not completely clear how it is guranteed that cur->unsafe
could never be set at this point. It works because:

  1. The previous owner would never release the lock in unsafe region.

  2. cur->unsafe might be set also because of cur->unsafe_takeover
     but this is checked above.

I would personally use the following comment:

		/*
		 * The lock should have never been released in an unsafe
		 * region.
		 */
		WARN_ON_ONCE(cur->unsafe);

> +
> +		new.atom = cur->atom;
> +		new.prio	= ctxt->prio;
> +		new.req_prio	= NBCON_PRIO_NONE;
> +		new.unsafe	= cur->unsafe_takeover;
> +		new.cpu		= cpu;
> +
> +	} while (!nbcon_state_try_cmpxchg(con, cur, &new));
> +
> +	/* Handover success. This context now owns the console. */
> +
> +	cur->atom = new.atom;
> +
> +	return 0;
> +}

Otherwise, it looks good to me.

Best Regards,
Petr