On Wed, May 11, 2022 at 07:04:51PM +0900, Hyeonggon Yoo wrote: > On Wed, May 11, 2022 at 08:39:29AM +0900, Byungchul Park wrote: > > On Tue, May 10, 2022 at 08:18:12PM +0900, Hyeonggon Yoo wrote: > > > On Mon, May 09, 2022 at 09:16:37AM +0900, Byungchul Park wrote: > > > > CASE 1. > > > > > > > > lock L with depth n > > > > lock_nested L' with depth n + 1 > > > > ... > > > > unlock L' > > > > unlock L > > > > > > > > This case is allowed by Lockdep. > > > > This case is allowed by DEPT cuz it's not a deadlock. > > > > > > > > CASE 2. > > > > > > > > lock L with depth n > > > > lock A > > > > lock_nested L' with depth n + 1 > > > > ... > > > > unlock L' > > > > unlock A > > > > unlock L > > > > > > > > This case is allowed by Lockdep. > > > > This case is *NOT* allowed by DEPT cuz it's a *DEADLOCK*. > > > > > > Yeah, in previous threads we discussed this [1] > > > > > > And the case was: > > > scan_mutex -> object_lock -> kmemleak_lock -> object_lock > > > And dept reported: > > > object_lock -> kmemleak_lock, kmemleak_lock -> object_lock as > > > deadlock. > > > > > > But IIUC - What DEPT reported happens only under scan_mutex and it > > > is not simple just not to take them because the object can be > > > removed from the list and freed while scanning via kmemleak_free() > > > without kmemleak_lock and object_lock. The above kmemleak sequence shouldn't deadlock since those locks, even if taken in a different order, are serialised by scan_mutex. For various reasons, trying to reduce the latency, I ended up with some fine-grained, per-object locking. For object allocation (rbtree modification) and tree search, we use kmemleak_lock. During scanning (which can take minutes under scan_mutex), we want to prevent (a) long latencies and (b) freeing the object being scanned. We release the locks regularly for (a) and hold the object->lock for (b). In another thread Byungchul mentioned: | context X context Y | | lock mutex A lock mutex A | lock B lock C | lock C lock B | unlock C unlock B | unlock B unlock C | unlock mutex A unlock mutex A | | In my opinion, lock B and lock C are unnecessary if they are always | along with lock mutex A. Or we should keep correct lock order across all | the code. If these are the only two places, yes, locks B and C would be unnecessary. But we have those locks acquired (not nested) on the allocation path (kmemleak_lock) and freeing path (object->lock). We don't want to block those paths while scan_mutex is held. That said, we may be able to use a single kmemleak_lock for everything. The object freeing path may be affected slightly during scanning but the code does release it every MAX_SCAN_SIZE bytes. It may even get slightly faster as we'd hammer a single lock (I'll do some benchmarks). But from a correctness perspective, I think the DEPT tool should be improved a bit to detect when such out of order locking is serialised by an enclosing lock/mutex. -- Catalin
