Re: [PATCH v5 6/7] locking/lockdep: Reuse freed chain_hlocks entries

[Waiman Long <longman@redhat.com>]

On 2/4/20 7:36 AM, Peter Zijlstra wrote:
> On Mon, Feb 03, 2020 at 11:41:46AM -0500, Waiman Long wrote:
>> +	if (unlikely(size < 2))
>> +		return; // XXX leak!
> Stuck this on top...
>
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -2631,7 +2631,8 @@ struct lock_chain lock_chains[MAX_LOCKDE
>  static DECLARE_BITMAP(lock_chains_in_use, MAX_LOCKDEP_CHAINS);
>  static u16 chain_hlocks[MAX_LOCKDEP_CHAIN_HLOCKS];
>  unsigned long nr_zapped_lock_chains;
> -unsigned int nr_free_chain_hlocks;	/* Free cfhain_hlocks in buckets */
> +unsigned int nr_free_chain_hlocks;	/* Free chain_hlocks in buckets */
> +unsigned int nr_lost_chain_hlocks;	/* Lost chain_hlocks */
>  unsigned int nr_large_chain_blocks;	/* size > MAX_CHAIN_BUCKETS */
>  
>  /*
> @@ -2718,8 +2719,17 @@ static inline void add_chain_block(int o
>  	int bucket = size_to_bucket(size);
>  	int next = chain_block_buckets[bucket];
>  
> -	if (unlikely(size < 2))
> -		return; // XXX leak!
> +	if (unlikely(size < 2)) {
> +		/*
> +		 * We can't store single entries on the freelist. Leak them.
> +		 *
> +		 * One possible way out would be to uniquely mark them, other
> +		 * than with CHAIN_BLK_FLAG, such that we can recover them when
> +		 * the block before it is re-added.
> +		 */
> +		nr_lost_chain_hlocks++;
> +		return;
> +	}
>  
>  	init_chain_block(offset, next, bucket, size);
>  	chain_block_buckets[bucket] = offset;
> @@ -2798,8 +2808,8 @@ static int alloc_chain_hlocks(int req)
>  
>  search:
>  	/*
> -	 * linear search in the 'dump' bucket; look for an exact match or the
> -	 * largest block.
> +	 * linear search of the variable sized freelist; look for an exact
> +	 * match or the largest block.
>  	 */
>  	for_each_chain_block(0, prev, curr, next) {
>  		size = chain_block_size(curr);
> --- a/kernel/locking/lockdep_internals.h
> +++ b/kernel/locking/lockdep_internals.h
> @@ -141,6 +141,7 @@ extern unsigned int nr_hardirq_chains;
>  extern unsigned int nr_softirq_chains;
>  extern unsigned int nr_process_chains;
>  extern unsigned int nr_free_chain_hlocks;
> +extern unsigned int nr_lost_chain_hlocks;
>  extern unsigned int nr_large_chain_blocks;
>  
>  extern unsigned int max_lockdep_depth;
> --- a/kernel/locking/lockdep_proc.c
> +++ b/kernel/locking/lockdep_proc.c
> @@ -278,9 +278,11 @@ static int lockdep_stats_show(struct seq
>  #ifdef CONFIG_PROVE_LOCKING
>  	seq_printf(m, " dependency chains:             %11lu [max: %lu]\n",
>  			lock_chain_count(), MAX_LOCKDEP_CHAINS);
> -	seq_printf(m, " dependency chain hlocks:       %11lu [max: %lu]\n",
> -			MAX_LOCKDEP_CHAIN_HLOCKS - nr_free_chain_hlocks,
> +	seq_printf(m, " dependency chain hlocks used:  %11lu [max: %lu]\n",
> +			MAX_LOCKDEP_CHAIN_HLOCKS - (nr_free_chain_hlocks - nr_lost_chain_hlocks),
>  			MAX_LOCKDEP_CHAIN_HLOCKS);
> +	seq_printf(m, " dependency chain hlocks free:  %11lu\n", nr_free_chain_hlocks);
> +	seq_printf(m, " dependency chain hlocks lost:  %11lu\n", nr_lost_chain_hlocks);
>  #endif
>  
>  #ifdef CONFIG_TRACE_IRQFLAGS
>
Sure. Will do that.

Thanks for the suggestion.

Cheers,
Longman