On Mon, Jul 06, 2026 at 07:05:44PM +0100, Matthew Wilcox wrote:
> On Mon, Jul 06, 2026 at 03:19:10PM +0900, Byungchul Park wrote:
> > Makes dept able to track PG_locked waits and events, which will be
> > useful in practice. See the following link that shows dept worked with
> > PG_locked and detected real issues in practice:
> >
> >
> > https://lore.kernel.org/lkml/[email protected]/
>
> > @@ -219,6 +220,7 @@ struct page {
> > struct page *kmsan_shadow;
> > struct page *kmsan_origin;
> > #endif
> > + struct dept_ext_wgen pg_locked_wgen;
> > } _struct_page_alignment;
>
> I may not understand this quite correctly, but I think that tracking
> PG_locked dependencies in the struct page has both false positive and
> false negative problems.
>
> Imagine we have a file mapping M1 containing folio F1 at index 0 and F2
> at index 1. It is correct locking order to lock F1 before locking F2
> (for example when doing writeback). Later, M1 has its folios reclaimed
> and returned to the free pool. Then each is added to mapping M2, this
> time with folio F2 at index 8 and F1 at index 9. Now the correct order
> to lock these folios in the order F2 followed by F1.
First of all, I appreciate your feedback. Thanks!
That case doesn't generate any dependency unless any other waits are
involved in. That should be handled in xxx_nested manner e.g.
folio_lock_nested() that I need to introduce. The work is in progress.
> I don't see a part of this patch where we clear pg_locked_wgen when the
> page is returned to the page allocator. Maybe I missed that.
You are right. pg_locked_wgen doesn't get cleared. However, DEPT works
this way:
folio_lock()
wait_for_pg_locked_cleared()
set_pg_locked() // (1) update pg_locked_wgen to the current wgen
... // there might be other waits
folio_unlock()
clear_pg_locked() // (2) check if there have been any waits since (1)
In other words, it's guranteed that pg_locked_wgen has been updated e.i.
(1) when DEPT refers to pg_locked_wgen e.i. (2). So I don't think it's
a problem.
> I think we should be tracking PG_locked dependencies in the owner
> of the folio. For files, that would be in the struct address_space.
> For anon memory, I think that's in the anon_vma, but if somebody told
> me it was in some other structure, I wouldn't argue with them.
I think it's a good point but it's a classification issue. folios owned
by struct address_space should be classified to e.g. address_space_class
and ones owned by struct anon_vma should be classified to e.g.
anon_vma_class. I will work on it to apply the insight you just gave
but better do it as follow-up patches since the initial patchset is
already too big to get reviewed.
> This requires slightly more complexity than lockdep currently has.
> We don't want to use a lockdep class for each folio, obviously. So we
> need something to say "I already have folio F1 locked, is it OK to lock
>From DEPT's perspective, folio_lock(F1) and folio_lock(F2) are waits and
folio_unlock(F1) and folio_unlock(F2) are events. Since DEPT tracks
dependencies with specified classes between waits and events, DEPT's
interest in the following example is to detect a situation like:
< context X >
folio_lock(address_space_class'ed F1)
...
folio_lock(anon_vma_class'ed F2)
...
folio_unlock(anon_vma_class'ed F2)
...
folio_unlock(address_space_class'ed F1)
< context Y >
folio_lock(anon_vma_class'ed any folio)
...
folio_lock(address_space_class'ed any folio)
...
folio_unlock(address_space_class'ed any folio)
...
folio_unlock(anon_vma_class'ed any folio)
However, the following pattern should be manually annotated by
developers like using folio_lock_nested() or something. DEPT cannot
work with it automatically:
folio_lock(address_space_class'ed F1)
...
folio_lock(address_space_class'ed F2)
...
folio_unlock(address_space_class'ed F2)
...
folio_unlock(address_space_class'ed F1)
or
folio_lock(anon_vma_class'ed F1)
...
folio_lock(anon_vma_class'ed F2)
...
folio_unlock(anon_vma_class'ed F2)
...
folio_unlock(anon_vma_class'ed F1)
These should be explicitly annotated by developers if it's intended:
folio_lock(address_space_class'ed F1)
...
folio_lock_nested(address_space_class'ed F2)
...
folio_unlock(address_space_class'ed F2)
...
folio_unlock(address_space_class'ed F1)
or
folio_lock(anon_vma_class'ed F1)
...
folio_lock_nested(anon_vma_class'ed F2)
...
folio_unlock(anon_vma_class'ed F2)
...
folio_unlock(anon_vma_class'ed F1)
> folio F2?". Essentially figuring out how we can track all folios in a
> given mapping the same way, and making sure that we don't deadlock on
> folios in the same mapping.
At the moment, as I told you, DEPT cannot work with dependencies between
the same class'ed folios. However, it'd be much better if DEPT can work
with even those cases. Could you provide a scenario where a deadlock
happens between the same class'ed ones? Any idea how to detect for the
cases?
> If F1 and F2 are in different mappings, it's not a deadlock if F1 is in a
> filesystem mapping and F2 is in its backing dev. It's also not a deadlock
> if F1 and F2 are both filesystem folios and the inodes are both locked.
> See vfs_lock_two_folios() in fs/remap_range.c.
Yeah.. DEPT is a tracker to track dependencies between waits and events
even across different contexts, but not a magic unfortunately. That
lock ordering issue - with the same class'ed ones - should be resolved
in the manual manner as vfs_lock_two_folios() does.
> I have much less knowledge about anonymous memory locking order.
> Maybe it doesn't happen. Or about locking one anon and one file folio.
> For slab memory, we don't sleep on PG_locked (it's used as a spinlock bit).
> For other kinds of memory ... I don't know. Page migration is fun.
Anyway, the sophisticated classification you mentioned is necessary for
DEPT to be better especially for folio locking mechanism.
Thanks again!
Byungchul
