linux-kernel - Re: [PATCH v2 1/7] locking: Introduce local

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200525112638.5p2ctr5l26uzergb@linutronix.de>
Date:   Mon, 25 May 2020 13:26:38 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Will Deacon <will@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH v2 1/7] locking: Introduce local_lock()

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