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

[Bart Van Assche]

On Fri, 2019-02-08 at 11:43 +-0000, Will Deacon wrote:
+AD4 I've also been trying to understand why it's necessary to check both of the
+AD4 pending+AF8-free entries, and I'm still struggling somewhat. It's true that the
+AD4 wakeup in get+AF8-pending+AF8-free+AF8-lock() could lead to both entries being used
+AD4 without the RCU call back running in between, however in this scenario then
+AD4 any list entries marked for freeing in the first pf will have been unhashed
+AD4 and therefore made unreachable to look+AF8-up+AF8-lock+AF8-class().
+AD4 
+AD4 So I think the concern remains that entries are somehow remaining visible
+AD4 after being zapped.
+AD4 
+AD4 You mentioned earlier in the thread that people actually complained about
+AD4 list corruption if you only checked the current pf:
+AD4 
+AD4   +AHw The list+AF8-del+AF8-rcu() call must only happen once. I ran into complaints
+AD4   +AHw reporting that the list+AF8-del+AF8-rcu() call triggered list corruption. This
+AD4   +AHw change made these complaints disappear.
+AD4 
+AD4 Do you have any more details about these complaints (e.g. kernel logs etc)?
+AD4 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+ADs do :+ADs 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 +ACI-entries somehow remain visible after being
zapped+ACI: in a previous version of this patch series a struct list+AF8-head was
added in struct lock+AF8-list. That list head was used to maintain a linked list
of all elements of the list+AF8-entries+AFsAXQ array that are in use. zap+AF8-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+AF8-class() whether or not a list
entry was being removed because it got removed from that list before
zap+AF8-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+AF8-class()) must check all three bitmaps. The
only alternative I see when using bitmaps is that zap+AF8-class() clears the
bits in list+AF8-entries+AF8-in+AF8-use for bits that are being freed and that
alloc+AF8-list+AF8-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.