Re: [PATCH v6 00/16] locking/lockdep: Add support for dynamic keys

From: Bart Van Assche <bvanassche@acm.org>
To: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	mingo@redhat.com, tj@kernel.org, longman@redhat.com,
	johannes.berg@intel.com, linux-kernel@vger.kernel.org,
	Paul McKenney <paulmck@linux.vnet.ibm.com>
Subject: Re: [PATCH v6 00/16] locking/lockdep: Add support for dynamic keys
Date: Fri, 08 Feb 2019 08:31:58 -0800	[thread overview]
Message-ID: <1549643518.34241.101.camel@acm.org> (raw)
In-Reply-To: <20190208114315.GD6972@fuggles.cambridge.arm.com>

On Fri, 2019-02-08 at 11:43 +0000, Will Deacon wrote:
> I've also been trying to understand why it's necessary to check both of the
> pending_free entries, and I'm still struggling somewhat. It's true that the
> wakeup in get_pending_free_lock() could lead to both entries being used
> without the RCU call back running in between, however in this scenario then
> any list entries marked for freeing in the first pf will have been unhashed
> and therefore made unreachable to look_up_lock_class().
> 
> So I think the concern remains that entries are somehow remaining visible
> after being zapped.
> 
> You mentioned earlier in the thread that people actually complained about
> list corruption if you only checked the current pf:
> 
>   | The list_del_rcu() call must only happen once. I ran into complaints
>   | reporting that the list_del_rcu() call triggered list corruption. This
>   | change made these complaints disappear.
> 
> Do you have any more details about these complaints (e.g. kernel logs etc)?
> Failing that, any idea how to reproduce them?

Hi Will,

The approach I use to test this patch series is to run the following shell
code for several days:

    git clone https://github.com/osandov/blktests/
    cd blktests
    make
    while ./check -q srp; do :; done

This test not only triggers plenty of lock and unlock calls but also
frequently causes kernel modules to be loaded and unloaded.

The oldest kernel logs I have in the VM I use for testing this patch series
are four weeks old. Sorry but that means that these logs do not go back far
enough to retrieve the list corruption issue I mentioned in a previous
e-mail.

Regarding the concern that "entries somehow remain visible after being
zapped": in a previous version of this patch series a struct list_head was
added in struct lock_list. That list head was used to maintain a linked list
of all elements of the list_entries[] array that are in use. zap_class()
used that list to iterate over all list entries that are in use. With that
approach it was not necessary to check in zap_class() whether or not a list
entry was being removed because it got removed from that list before
zap_class() was called again. I removed that list head because Peter asked
me reduce the amount of memory required at runtime. Using one bitmap to
track list entries that are in use and using two bitmaps to track list
entries that are being freed implies that code that iterates over all
list entries that are in use (zap_class()) must check all three bitmaps. The
only alternative I see when using bitmaps is that zap_class() clears the
bits in list_entries_in_use for bits that are being freed and that
alloc_list_entry() checks the two bitmaps with list entries that are being
freed. I'm not sure whether one of these two approaches is really better
than the other.

Bart.