Re: [PATCH v2 1/7] locking: Introduce local_lock()

Ingo Molnar
2020-05-25T07:01:45Z

* Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> From: Thomas Gleixner <tglx@linutronix.de>
>
> To address this PREEMPT_RT introduced the concept of local_locks which are
> strictly per CPU.

> +++ b/include/linux/locallock_internal.h
> @@ -0,0 +1,90 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_LOCALLOCK_H
> +# error "Do not include directly, include linux/locallock.h"
> +#endif
> +
> +#include <linux/percpu-defs.h>
> +#include <linux/lockdep.h>
> +
> +struct local_lock {
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> + struct lockdep_map dep_map;
> + struct task_struct *owner;
> +#endif
> +};

This this looks very nice to me, there's a minor data structure
nomenclature related comment I have:

So local locks were supposed to be a look-alike to all the other
locking constructs we have, spinlock_t in particular. Why isn't there
a local_lock_t, instead of requiring 'struct local_lock'?

This abbreviation signals that these are 'small' data structures on
mainline kernels (zero size in fact), but the other advantage is that
the shorter name would prevent bloating of previously compact
structure definitions, such as:

> struct squashfs_stream {
> - void *stream;
> + void *stream;
> + struct local_lock lock;
> };

This would become:

> struct squashfs_stream {
> void *stream;
> + locallock_t lock;
> };

( The other departure from spinlocks is that the 'spinlock_t' name,
without underscores, while making the API names such as spin_lock()
with an underscore, was a conscious didactic choice. Applying that
principle to local locks gives us the spinlock_t-equivalent name of
'locallock_t' - but the double 'l' reads a bit weirdly in this
context. So I think using 'local_lock_t' as the data structure is
probably the better approach. )

Thanks,

Ingo