Re: [PATCH next v2 1/2] dump_stack: move cpu lock to printk.c

[Petr Mladek]

On Mon 2021-06-07 22:02:31, John Ogness wrote:
> dump_stack() implements its own cpu-reentrant spinning lock to
> best-effort serialize stack traces in the printk log. However,
> there are other functions (such as show_regs()) that can also
> benefit from this serialization.
> 
> Move the cpu-reentrant spinning lock (cpu lock) into new helper
> functions printk_cpu_lock_irqsave()/printk_cpu_unlock_irqrestore()
> so that it is available for others as well. For !CONFIG_SMP the
> cpu lock is a NOP.
> 
> Note that having multiple cpu locks in the system can easily
> lead to deadlock. Code needing a cpu lock should use the
> printk cpu lock, since the printk cpu lock could be acquired
> from any code and any context.
> 
> Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx>

There are some nits below but the patch looks fine to me as it.

Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3532,3 +3532,78 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
> +void printk_cpu_lock_irqsave(bool *lock_flag, unsigned long *irq_flags)
> +{
> +	int old;
> +	int cpu;
> +
> +retry:
> +	local_irq_save(*irq_flags);
> +
> +	cpu = smp_processor_id();
> +
> +	old = atomic_cmpxchg(&printk_cpulock_owner, -1, cpu);
> +	if (old == -1) {
> +		/* This CPU is now the owner. */
> +

Superfluous space?

> +		*lock_flag = true;

The original name name "was_locked" was more descriptive. I agree that
it was not good for an API. What about keeping the inverted logic and
calling it "lock_nested" ?

I do not resist on any change. The logic is trivial so...

> +
> +	} else if (old == cpu) {
> +		/* This CPU is already the owner. */
> +
> +		*lock_flag = false;
> +

Even more superfluous spaces?

> +	} else {
> +		local_irq_restore(*irq_flags);
> +
> +		/*
> +		 * Wait for the lock to release before jumping to cmpxchg()
> +		 * in order to mitigate the thundering herd problem.
> +		 */
> +		do {
> +			cpu_relax();
> +		} while (atomic_read(&printk_cpulock_owner) != -1);
> +
> +		goto retry;
> +	}
> +}
> +EXPORT_SYMBOL(printk_cpu_lock_irqsave);
> +
> +/*
> + * printk_cpu_unlock_irqrestore: Release the printk cpu-reentrant spinning
> + *                               lock and restore interrupts.
> + * @lock_flag: The current lock state.
> + * @irq_flags: The current irq state.

"The current" is a bit misleading. Both values actually describe
the state before the related printk_cpu_lock_irqsave().
What about something like?

  * @lock_nested: Lock state set when the lock was taken.
  * @irq_flags: IRQ flags stored when the lock was taken.


> + *
> + * Release the lock. The calling processor must be the owner of the lock.
> + *
> + * It is safe to call this function from any context and state.
> + */
> +void printk_cpu_unlock_irqrestore(bool lock_flag, unsigned long irq_flags)
> +{
> +	if (lock_flag) {
> +		atomic_set(&printk_cpulock_owner, -1);
> +
> +		local_irq_restore(irq_flags);
> +	}
> +}
> +EXPORT_SYMBOL(printk_cpu_unlock_irqrestore);
> +#endif /* CONFIG_SMP */

 Best Regards,
 Petr