lkml.org 
[lkml]   [2021]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH next v2 1/2] dump_stack: move cpu lock to printk.c
Date
On 2021-06-08, Petr Mladek <pmladek@suse.com> wrote:
> 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?

I was concerned that people may associate the comment with the following
line of code. Especially in the next patch when many more lines are
added. The comment is for the whole conditional block.

>> + *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...

I wanted it to be an opaque variable, which is why it is named so. But I
can rename it for v3. There is no need to debate naming here.

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

This code is not trivial and it is absolutely critical when we introduce
atomic consoles. My goal is clarity rather than compactness
(particularly after proper memory barrier comments are added).

>> + } 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.

OK. I will make this change for v3.

John Ogness

\
 
 \ /
  Last update: 2021-06-08 15:56    [W:0.112 / U:1.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site