On Tue, 2018-12-04 at 15:27 -0500, Waiman Long wrote:
> On 12/03/2018 07:28 PM, Bart Van Assche wrote:
> > +/* Must be called with the graph lock held. */
> > +static void remove_class_from_lock_chain(struct lock_chain *chain,
> > + struct lock_class *class)
> > +{
> > + u64 chain_key;
> > + int i;
> > +
> > +#ifdef CONFIG_PROVE_LOCKING
> > + for (i = chain->base; i < chain->base + chain->depth; i++) {
> > + if (chain_hlocks[i] != class - lock_classes)
> > + continue;
> > + if (--chain->depth == 0)
> > + break;
> > + memmove(&chain_hlocks[i], &chain_hlocks[i + 1],
> > + (chain->base + chain->depth - i) *
> > + sizeof(chain_hlocks[0]));
> > + /*
> > + * Each lock class occurs at most once in a
> > + * lock chain so once we found a match we can
> > + * break out of this loop.
> > + */
> > + break;
> > + }
> > + /*
> > + * Note: calling hlist_del_rcu() from inside a
> > + * hlist_for_each_entry_rcu() loop is safe.
> > + */
> > + if (chain->depth == 0) {
> > + /* To do: decrease chain count. See also inc_chains(). */
> > + hlist_del_rcu(&chain->entry);
> > + return;
> > + }
> > + chain_key = 0;
> > + for (i = chain->base; i < chain->base + chain->depth; i++)
> > + chain_key = iterate_chain_key(chain_key, chain_hlocks[i] + 1);
>
> Do you need to recompute the chain_key if no entry in the chain is removed?
Thanks for having pointed that out. I will modify this function such that the
chain key is only recalculated if necessary.
> >
> > @@ -4141,14 +4253,31 @@ static void zap_class(struct lock_class *class)
> > for (i = 0, entry = list_entries; i < nr_list_entries; i++, entry++) {
> > if (entry->class != class && entry->links_to != class)
> > continue;
> > + links_to = entry->links_to;
> > + WARN_ON_ONCE(entry->class == links_to);
> > list_del_rcu(&entry->entry);
> > + check_free_class(class);
>
> Is the check_free_class() call redundant? You are going to call it near
> the end below.
I think so. I will remove the check_free_class() that is inside the for-loop.
> > +static void reinit_class(struct lock_class *class)
> > +{
> > + void *const p = class;
> > + const unsigned int offset = offsetof(struct lock_class, key);
> > +
> > + WARN_ON_ONCE(!class->lock_entry.next);
> > + WARN_ON_ONCE(!list_empty(&class->locks_after));
> > + WARN_ON_ONCE(!list_empty(&class->locks_before));
> > + memset(p + offset, 0, sizeof(*class) - offset);
> > + WARN_ON_ONCE(!class->lock_entry.next);
> > + WARN_ON_ONCE(!list_empty(&class->locks_after));
> > + WARN_ON_ONCE(!list_empty(&class->locks_before));
> > }
>
> Is it safer to just reinit those fields before "key" instead of using
> memset()? Lockdep is slow anyway, doing that individually won't
> introduce any noticeable slowdown.
The warning statements will only be hit if the order of the struct lock_class
members
would be modified. I don't think that we need to change the approach of this
function.
> > @@ -4193,18 +4355,14 @@ void lockdep_free_key_range(void *start, unsigned
> > long size)
> > raw_local_irq_restore(flags);
> >
> > /*
> > - * Wait for any possible iterators from look_up_lock_class() to pass
> > - * before continuing to free the memory they refer to.
> > - *
> > - * sync_sched() is sufficient because the read-side is IRQ disable.
> > + * Do not wait for concurrent look_up_lock_class() calls. If any such
> > + * concurrent call would return a pointer to one of the lock classes
> > + * freed by this function then that means that there is a race in the
> > + * code that calls look_up_lock_class(), namely concurrently accessing
> > + * and freeing a synchronization object.
> > */
> > - synchronize_sched();
> >
> > - /*
> > - * XXX at this point we could return the resources to the pool;
> > - * instead we leak them. We would need to change to bitmap allocators
> > - * instead of the linear allocators we have now.
> > - */
> > + schedule_free_zapped_classes();
>
> Should you move the graph_unlock() and raw_lock_irq_restore() down to
> after this? The schedule_free_zapped_classes must be called with
> graph_lock held. Right?
I will modify this and other patches such that all
schedule_free_zapped_classes()
calls are protected by the graph lock.
Bart.
