Re: [PATCH 6/8] mm/khugepaged: unmap pte before releasing vma write lock

[Nico Pache]

On Mon, Jul 6, 2026 at 1:34 PM Usama Arif <usama.arif@xxxxxxxxx> wrote:
>
> On Mon,  6 Jul 2026 09:44:53 -0600 Nico Pache <npache@xxxxxxxxxx> wrote:
>
> > We are currently dropping the anon_vma write lock before unmapping the
> > PTE. Although this is safe, due to us still holding the mmap_write_lock,
> > its safer and less confusing to switch the order of these two operations.
> >
> > Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
> > Signed-off-by: Nico Pache <npache@xxxxxxxxxx>
> > ---
> >  mm/khugepaged.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Would it not be better to add a comment here why its safe to do so, instead
> of extending lock hold period?

As per Documentation/mm/process_addrs.rst

->anon_vma.lock
     ->page_table_lock or pte_lock (anon_vma_prepare and various)

We must release the PTE first. This patch ensures we follow the proper
convention.

>
> I think its more confusing to hold a lock for longer than we should.

Its basically a no-op on modern systems, and due to the mmap_write
lock being held also would provide no real benefit to release it just
a few nanoseconds faster.

Cheers,
-- Nico

>
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index c4ea2dc1591b..3c6f1254deca 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -1548,10 +1548,10 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s
> >
> >       result = SCAN_SUCCEED;
> >  out_up_write:
> > -     if (anon_vma_locked)
> > -             anon_vma_unlock_write(vma->anon_vma);
> >       if (pte)
> >               pte_unmap(pte);
> > +     if (anon_vma_locked)
> > +             anon_vma_unlock_write(vma->anon_vma);
> >       mmap_write_unlock(mm);
> >  out_nolock:
> >       if (folio)
> > --
> > 2.54.0
> >
> >
>