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

[Sebastian Andrzej Siewior]

On 2020-05-25 09:01:39 [+0200], Ingo Molnar wrote:
> 
> * Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> wrote:
> 
> > From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxx/

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