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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <878s3kihpk.fsf@jogness.linutronix.de>
Date:   Tue, 08 Jun 2021 15:55:35 +0200
From:   John Ogness <john.ogness@...utronix.de>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Peter Zijlstra <peterz@...radead.org>,
        Marco Elver <elver@...gle.com>,
        Alexander Potapenko <glider@...gle.com>,
        Stephen Boyd <swboyd@...omium.org>
Subject: Re: [PATCH next v2 1/2] dump_stack: move cpu lock to printk.c

On 2021-06-08, Petr Mladek <pmladek@...e.com> wrote:
> Reviewed-by: Petr Mladek <pmladek@...e.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