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

Peter Zijlstra
2017-01-10T21:00:42Z

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@lge.com>
> ---
> 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
>