Re: [PATCH v3 6/8] locking/lockdep: Reuse freed chain_hlocks entries

[Waiman Long]

On 1/16/20 4:13 PM, Peter Zijlstra wrote:
> On Wed, Jan 15, 2020 at 04:43:11PM -0500, Waiman Long wrote:
>> +static inline int alloc_chain_hlocks_from_buckets(int size)
>> +{
>> +	int prev, curr, next;
>> +
>> +	if (!nr_free_chain_hlocks)
>> +		return -1;
>> +
>> +	if (size <= MAX_CHAIN_BUCKETS) {
>> +		curr = chain_block_buckets[size - 1];
>> +		if (curr < 0)
>> +			return -1;
>> +
>> +		chain_block_buckets[size - 1] = next_chain_block(curr);
>> +		nr_free_chain_hlocks -= size;
>> +		return curr;
>> +	}
>> +
>> +	/*
>> +	 * Look for a free chain block of the given size
>> +	 *
>> +	 * It is rare to have a lock chain with depth > MAX_CHAIN_BUCKETS.
>> +	 * It is also more expensive as we may iterate the whole list
>> +	 * without finding one.
>> +	 */
>> +	for_each_chain_block(0, prev, curr, next) {
>> +		next = next_chain_block(curr);
>> +		if (chain_block_size(curr) == size) {
>> +			set_chain_block(prev, 0, next);
>> +			nr_free_chain_hlocks -= size;
>> +			nr_large_chain_blocks--;
>> +			return curr;
>> +		}
>> +	}
>> +	return -1;
>> +}
>> +static int alloc_chain_hlocks(int size)
>> +{
>> +	int curr;
>> +
>> +	if (size < 2)
>> +		size = 2;
>> +
>> +	curr = alloc_chain_hlocks_from_buckets(size);
>> +	if (curr >= 0)
>> +		return curr;
>> +
>> +	BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));
>> +	BUILD_BUG_ON((1UL << 6)  <= ARRAY_SIZE(current->held_locks));
>> +	BUILD_BUG_ON((1UL << 8*sizeof(chain_hlocks[0])) <=
>> +		     ARRAY_SIZE(lock_classes));
>> +
>> +	/*
>> +	 * Allocate directly from chain_hlocks.
>> +	 */
>> +	if (likely(nr_chain_hlocks + size <= MAX_LOCKDEP_CHAIN_HLOCKS)) {
>> +		curr = nr_chain_hlocks;
>> +		nr_chain_hlocks += size;
>> +		return curr;
>> +	}
>> +	if (!debug_locks_off_graph_unlock())
>> +		return -1;
>> +
>> +	print_lockdep_off("BUG: MAX_LOCKDEP_CHAIN_HLOCKS too low!");
>> +	dump_stack();
>> +	return -1;
>> +}
> Argh, that's still _two_ half allocators.
>
> Here, please try this one, it seems to boot. It compiles with some
> noise, but that is because GCC is stupid and I'm too tired.
>
> ---
>
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1071,15 +1071,22 @@ static inline void check_data_structures
>  
>  #endif /* CONFIG_DEBUG_LOCKDEP */
>  
> +static void init_chain_block_buckets(void);
> +
>  /*
>   * Initialize the lock_classes[] array elements, the free_lock_classes list
>   * and also the delayed_free structure.
>   */
>  static void init_data_structures_once(void)
>  {
> -	static bool ds_initialized, rcu_head_initialized;
> +	static bool ds_initialized, rcu_head_initialized, chain_block_initialized;
>  	int i;
>  
> +	if (!chain_block_initialized) {
> +		chain_block_initialized = true;
> +		init_chain_block_buckets();
> +	}
> +
>  	if (likely(rcu_head_initialized))
>  		return;

Oh, I was not aware that there is such a init_data_structure_once()
function. I don't think we need a chain_block_initialized. The
ds_initialized should be enough and the init_chain_block_buckets() can
be put to the end of the function.

Other than that, the rests look OK to me so far. I will try it out tomorrow.

Thanks,
Longman