Re: [PATCH printk v1 01/13] printk: rename cpulock functions

From: John Ogness <john.ogness@linutronix.de>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Marco Elver <elver@google.com>,
	Stephen Boyd <swboyd@chromium.org>,
	Alexander Potapenko <glider@google.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH printk v1 01/13] printk: rename cpulock functions
Date: Fri, 11 Feb 2022 15:48:08 +0106	[thread overview]
Message-ID: <87fsopcvnj.fsf@jogness.linutronix.de> (raw)
In-Reply-To: <YgZaMkUU5Ve2GV9D@alley>

On 2022-02-11, Petr Mladek <pmladek@suse.com> wrote:
> On Mon 2022-02-07 20:49:11, John Ogness wrote:
>> Since the printk cpulock is CPU-reentrant and since it is used
>> in all contexts, its usage must be carefully considered and
>> most likely will require programming locklessly. To avoid
>> mistaking the printk cpulock as a typical lock, rename it to
>> cpu_sync. The main functions then become:
>> 
>>     printk_cpu_sync_get_irqsave(flags);
>>     printk_cpu_sync_put_irqrestore(flags);
>
> It is possible that I will understand the motivation later when
> reading the entire patchset. But my initial reaction is confusion ;-)

Actually, the motivation comes from a discussion we had during the RT
Track at Plumbers 2021 [0]. It isn't a lock and so we didn't want to
call it a lock. (More below.)

> From mo POV, it is a lock. It tries to get exclusive access and
> has to wait until the current owner releases it.

It is only exclusive for a CPU. If another context on that CPU tries to
get the "lock" it will succeed. For example:

process context lock() -> success
--- INTERRUPT ---
irq context lock() -> success
--- NMI ---
nmi context lock() -> success

None of these contexts can assume that they have synchronized access
because clearly they have all interrupted each other. If an object does
not provide synchronized access to data, then "lock" is probably not a
good name for that object.

> As you say: "its usage must be carefully considered and most likely
> will  require programming locklessly." I guess that it is related to:
>
>     + There is a risk of deadlocks that are typically associated with
>       locks. After all the word "lock" is part of "deadlock".
>
>     + It requires lockless programming because it is supposed to be
>       terminal lock. It means that no other locks should be taken
>       under it.

It is because (as in the example above), taking this "lock" does not
provide synchronization to data. It is only synchronizing between
CPUs. It was Steven's suggestion to call the thing a cpu_sync object and
nobody in the RT Track seemed to disagree.

> Is there any other API using this naming scheme, please?

No.

> I have get() and put() associated with reference counting. But it has
> an opposite meaning. It usually guards an object from freeing as long
> as there is at least one user. And it allows to have many users.

This _is_ reference counting. In fact, if you look at the implementation
you see:

    atomic_inc(&printk_cpu_sync_nested);

It is allowing multiple users (from the same CPU).

> Regarding the reentrancy. It seems that "_nested" suffix is used for
> this type of locks, for example, mutex_lock_nested(),
> spin_lock_nested().
>
> It might be enough to add "_nested" suffix and explain why it has
> to be used carefully (terminal lock) in a comment.

The internal counter is called "_nested" to make it clear to us printk
developers. IMO the common _get and _put semantics are appropriate
here. The important thing is that the word "lock" is removed. It is not
a lock.

John

[0] https://youtu.be/cZUzc0U1jJ4?t=12946