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: <20200525070139.GB329373@gmail.com>
Date:   Mon, 25 May 2020 09:01:39 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
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()

* 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'?

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