Re: + kernel-locking-lockdepc-make-lockdep-initialize-itself-on-demand.patch added to -mm tree

[Ingo Molnar]

* akpm@xxxxxxxxxxxxxxxxxxxx <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> 
> The patch titled
>      Subject: kernel/locking/lockdep.c: make lockdep initialize itself on demand
> has been added to the -mm tree.  Its filename is
>      kernel-locking-lockdepc-make-lockdep-initialize-itself-on-demand.patch
> 
> This patch should soon appear at
>     http://ozlabs.org/~akpm/mmots/broken-out/kernel-locking-lockdepc-make-lockdep-initialize-itself-on-demand.patch
> and later at
>     http://ozlabs.org/~akpm/mmotm/broken-out/kernel-locking-lockdepc-make-lockdep-initialize-itself-on-demand.patch
> 
> Before you just go and hit "reply", please:
>    a) Consider who else should be cc'ed
>    b) Prefer to cc a suitable mailing list as well
>    c) Ideally: find the original patch on the mailing list and do a
>       reply-to-all to that, adding suitable additional cc's
> 
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
> 
> The -mm tree is included into linux-next and is updated
> there every 3-4 working days
> 
> ------------------------------------------------------
> From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Subject: kernel/locking/lockdep.c: make lockdep initialize itself on demand
> 
> Mike said:
> 
> : CONFIG_UBSAN_ALIGNMENT breaks x86-64 kernel with lockdep enabled, i.  e
> : kernel with CONFIG_UBSAN_ALIGNMENT fails to load without even any error
> : message.
> : 
> : The problem is that ubsan callbacks use spinlocks and might be called
> : before lockdep is initialized.  Particularly this line in the
> : reserve_ebda_region function causes problem:
> : 
> : lowmem = *(unsigned short *)__va(BIOS_LOWMEM_KILOBYTES);
> : 
> : If i put lockdep_init() before reserve_ebda_region call in
> : x86_64_start_reservations kernel loads well.
> 
> Fix this ordering issue permanently: change lockdep so that it ensures
> that the hash tables are initialized when they are about to be used.
> 
> The overhead will be pretty small: a test-n-branch in places where lockdep
> is about to do a lot of work anyway.
> 
> Possibly lockdep_initialized should be made __read_mostly.
> 
> A better fix would be to simply initialize these (32768 entry) arrays of
> empty list_heads at compile time, but I don't think there's a way of
> teaching gcc to do this.
> 
> We could write a little script which, at compile time, emits a file
> containing
> 
> 	[0] = LIST_HEAD_INIT(__chainhash_table[0]),
> 	[1] = LIST_HEAD_INIT(__chainhash_table[1]),
> 	...
> 	[32767] = LIST_HEAD_INIT(__chainhash_table[32767]),
> 
> and then #include this file into lockdep.c.  Sounds like a lot of fuss.
> 
> Reported-by: Mike Krinkin <krinkin.m.u@xxxxxxxxx>
> Cc: Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
> 
>  kernel/locking/lockdep.c |   35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff -puN kernel/locking/lockdep.c~kernel-locking-lockdepc-make-lockdep-initialize-itself-on-demand kernel/locking/lockdep.c
> --- a/kernel/locking/lockdep.c~kernel-locking-lockdepc-make-lockdep-initialize-itself-on-demand
> +++ a/kernel/locking/lockdep.c
> @@ -290,9 +290,20 @@ LIST_HEAD(all_lock_classes);
>  #define CLASSHASH_BITS		(MAX_LOCKDEP_KEYS_BITS - 1)
>  #define CLASSHASH_SIZE		(1UL << CLASSHASH_BITS)
>  #define __classhashfn(key)	hash_long((unsigned long)key, CLASSHASH_BITS)
> -#define classhashentry(key)	(classhash_table + __classhashfn((key)))
>  
> -static struct list_head classhash_table[CLASSHASH_SIZE];
> +static struct list_head __classhash_table[CLASSHASH_SIZE];
> +
> +static inline struct list_head *get_classhash_table(void)
> +{
> +	if (unlikely(!lockdep_initialized))
> +		lockdep_init();
> +	return __classhash_table;
> +}
> +
> +static inline struct list_head *classhashentry(struct lockdep_subclass_key *key)
> +{
> +	return get_classhash_table() + __classhashfn(key);
> +}
>  
>  /*
>   * We put the lock dependency chains into a hash-table as well, to cache
> @@ -301,9 +312,15 @@ static struct list_head classhash_table[
>  #define CHAINHASH_BITS		(MAX_LOCKDEP_CHAINS_BITS-1)
>  #define CHAINHASH_SIZE		(1UL << CHAINHASH_BITS)
>  #define __chainhashfn(chain)	hash_long(chain, CHAINHASH_BITS)
> -#define chainhashentry(chain)	(chainhash_table + __chainhashfn((chain)))
>  
> -static struct list_head chainhash_table[CHAINHASH_SIZE];
> +static struct list_head __chainhash_table[CHAINHASH_SIZE];
> +
> +static struct list_head *chainhashentry(unsigned long chain)
> +{
> +	if (unlikely(!lockdep_initialized))
> +		lockdep_init();
> +	return __chainhash_table + __chainhashfn(chain);
> +}

Yuck, I don't really like this.

Lockdep initialization must happen early on, and it should happen in a well 
defined place, not be opportunistic (and relatively random) like this, making it 
dependent on config options and calling contexts.

Also, in addition to properly ordering UBSAN initialization, how about fixing the 
silent crash by adding a lockdep warning to that place instead of an auto-init? 

The warning will turn lockdep off safely and will generate an actionable kernel 
message and stackdump upon which the init ordering fix can be done.

Thanks,

	Ingo