Re: [PATCH v19 22/40] dept: track PG_locked with dept

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