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

[Ingo Molnar <mingo@kernel.org>]

* 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