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

Sebastian Andrzej Siewior
2020-05-25T11:26:42Z

On 2020-05-25 09:01:39 [+0200], Ingo Molnar wrote:
>
> * 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'?

|git grep "struct \<spinlock\>"

and I did convert them spinlock_t and got even asked why
https://lore.kernel.org/driverdev-devel/20190706100253.GA20497@kroah.com/

but yes. I can stick to local_lock_t instead.

> 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;
> > };

Wasn't aware as this is considered bloating.

> ( 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. )

Okay, okay, I'm all yours.

> Thanks,
>
> Ingo

Sebastian