Re: [PATCH v4 04/15] lockdep: Add a function building a chain between two classes

[Peter Zijlstra]

On Fri, Dec 09, 2016 at 02:12:00PM +0900, Byungchul Park wrote:
> add_chain_cache() should be used in the context where the hlock is
> owned since it might be racy in another context. However crossrelease
> feature needs to build a chain between two locks regardless of context.
> So introduce a new function making it possible.
> 
> Signed-off-by: Byungchul Park <byungchul.park@xxxxxxx>
> ---
>  kernel/locking/lockdep.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 5df56aa..111839f 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -2105,6 +2105,62 @@ static int check_no_collision(struct task_struct *curr,
>  	return 1;
>  }
>  
> +/*
> + * This is for building a chain between just two different classes,
> + * instead of adding a new hlock upon current, which is done by
> + * add_chain_cache().
> + *
> + * This can be called in any context with two classes, while
> + * add_chain_cache() must be done within the lock owener's context
> + * since it uses hlock which might be racy in another context.
> + */
> +static inline int add_chain_cache_classes(unsigned int prev,
> +					  unsigned int next,
> +					  unsigned int irq_context,
> +					  u64 chain_key)
> +{
> +	struct hlist_head *hash_head = chainhashentry(chain_key);
> +	struct lock_chain *chain;
> +
> +	/*
> +	 * Allocate a new chain entry from the static array, and add
> +	 * it to the hash:
> +	 */
> +
> +	/*
> +	 * We might need to take the graph lock, ensure we've got IRQs
> +	 * disabled to make this an IRQ-safe lock.. for recursion reasons
> +	 * lockdep won't complain about its own locking errors.
> +	 */
> +	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
> +		return 0;
> +
> +	if (unlikely(nr_lock_chains >= MAX_LOCKDEP_CHAINS)) {
> +		if (!debug_locks_off_graph_unlock())
> +			return 0;
> +
> +		print_lockdep_off("BUG: MAX_LOCKDEP_CHAINS too low!");
> +		dump_stack();
> +		return 0;
> +	}
> +
> +	chain = lock_chains + nr_lock_chains++;
> +	chain->chain_key = chain_key;
> +	chain->irq_context = irq_context;
> +	chain->depth = 2;
> +	if (likely(nr_chain_hlocks + chain->depth <= MAX_LOCKDEP_CHAIN_HLOCKS)) {
> +		chain->base = nr_chain_hlocks;
> +		nr_chain_hlocks += chain->depth;
> +		chain_hlocks[chain->base] = prev - 1;
> +		chain_hlocks[chain->base + 1] = next -1;
> +	}

You didn't copy this part right. There is no error when >
MAX_LOCKDEP_CHAIN_HLOCKS.


> +	hlist_add_head_rcu(&chain->entry, hash_head);
> +	debug_atomic_inc(chain_lookup_misses);
> +	inc_chains();
> +
> +	return 1;
> +}
> +
>  static inline int add_chain_cache(struct task_struct *curr,
>  				  struct held_lock *hlock,
>  				  u64 chain_key)
> -- 
> 1.9.1
>