Messages in this thread | | | Date | Tue, 8 Jun 2021 13:40:32 +0200 | From | Petr Mladek <> | Subject | Re: [PATCH next v2 1/2] dump_stack: move cpu lock to printk.c |
| |
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@linutronix.de>
There are some nits below but the patch looks fine to me as it.
Reviewed-by: Petr Mladek <pmladek@suse.com>
> --- 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
| |