Message-ID: <CAGsJ_4xHH6hnksBgHgwqDTtHt4Py5sEu9zCFNSsr9J3cKhWU9Q@mail.gmail.com>
Date: Wed, 28 May 2025 17:01:32 +0800
From: Barry Song <21cnbao@...il.com>
To: Lokesh Gidra <lokeshgidra@...gle.com>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, akpm@...ux-foundation.org, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	Barry Song <v-songbaohua@...o.com>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, 
	David Hildenbrand <david@...hat.com>, Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>, 
	Suren Baghdasaryan <surenb@...gle.com>, Tangquan Zheng <zhengtangquan@...o.com>
Subject: Re: [PATCH RFC] mm: use per_vma lock for MADV_DONTNEED

On Wed, May 28, 2025 at 4:40 AM Lokesh Gidra <lokeshgidra@...gle.com> wrote:
>
> Thanks a lot, Barry. This was overdue.
>
> On Tue, May 27, 2025 at 2:20 AM Lorenzo Stoakes
> <lorenzo.stoakes@...cle.com> wrote:
> >
> > Overall - thanks for this, and I'm not sure why we didn't think of doing
> > this sooner :P this seems like a super valid thing to try to use the vma
> > lock with.
> >
> > I see you've cc'd Suren who has the most expertise in this and can
> > hopefully audit this and ensure all is good, but from the process address
> > doc (see below), I think we're good to just have the VMA stabilised for a
> > zap.
> >
> > On Tue, May 27, 2025 at 04:41:45PM +1200, Barry Song wrote:
> > > From: Barry Song <v-songbaohua@...o.com>
> > >
> > > Certain madvise operations, especially MADV_DONTNEED, occur far more
> > > frequently than other madvise options, particularly in native and Java
> > > heaps for dynamic memory management.
>
> Actually, it would be great if you can also optimize the MADV_FREE
> case. There are quite a few performance critical situations where
> MADV_FREE is performed and are done within a single VMA.

Yes, that was definitely on the list. However, the reason MADV_FREE wasn't
included in this patch is because madvise_free_single_vma() calls
walk_page_range(), which requires the mmap_lock to locate the VMA using
find_vma()—which feels redundant and awkward, since we already have the VMA.

We should be able to use walk_page_range_vma() instead, given that the VMA
is already known. But even walk_page_range_vma() asserts that the mmap_lock
is held, so the issue remains.

int walk_page_range_vma(struct vm_area_struct *vma, unsigned long start,
unsigned long end, const struct mm_walk_ops *ops,
void *private)
{
...
process_mm_walk_lock(walk.mm, ops->walk_lock);
...
return __walk_page_range(start, end, &walk);
}

MADV_DONTNEED doesn't use walk_page_range() at all. So I'd prefer to
settle MADV_DONTNEED first as the initial step.

We need more time to investigate the behavior and requirements of
walk_page_range().

> >
> > Ack yeah, I have gathered that this is the case previously.
> >
> > >
> > > Currently, the mmap_lock is always held during these operations, even when
> > > unnecessary. This causes lock contention and can lead to severe priority
> > > inversion, where low-priority threads—such as Android's HeapTaskDaemon—
> > > hold the lock and block higher-priority threads.
> >
> > That's very nasty... we definitely want to eliminate as much mmap_lock
> > contention as possible.
> >
> > >
> > > This patch enables the use of per-VMA locks when the advised range lies
> > > entirely within a single VMA, avoiding the need for full VMA traversal. In
> > > practice, userspace heaps rarely issue MADV_DONTNEED across multiple VMAs.
> >
> > Yeah this single VMA requirement is obviously absolutely key.
> >
> > As per my docs [0] actually, for zapping a single VMA, 'The VMA need only be
> > kept stable for this operation.' (I had to look this up to remind myself :P)
> >
> > [0]: https://kernel.org/doc/html/latest/mm/process_addrs.html
> >
> > So we actually... should be good here, locking-wise.
> >
> > >
> > > Tangquan’s testing shows that over 99.5% of memory reclaimed by Android
> > > benefits from this per-VMA lock optimization. After extended runtime,
> > > 217,735 madvise calls from HeapTaskDaemon used the per-VMA path, while
> > > only 1,231 fell back to mmap_lock.
>
> I am quite confident that this pattern is not limited to
> HeapTaskDaemon. For instance, I'm sure the Scudo allocator must also
> be calling MADV_DONTNEED within single VMAs.

That's right. We've also optimized the native heaps.

> >
> > Thanks, this sounds really promising!
> >
> > I take it then you have as a result, heavily tested this change?
> >
> > >
> > > To simplify handling, the implementation falls back to the standard
> > > mmap_lock if userfaultfd is enabled on the VMA, avoiding the complexity of
> > > userfaultfd_remove().
> >
> > Oh GOD do I hate how we implement uffd. Have I ever mentioned that? Well,
> > let me mention it again...
> >
> > >
> > > Cc: "Liam R. Howlett" <Liam.Howlett@...cle.com>
> > > Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > > Cc: David Hildenbrand <david@...hat.com>
> > > Cc: Vlastimil Babka <vbabka@...e.cz>
> > > Cc: Jann Horn <jannh@...gle.com>
> > > Cc: Suren Baghdasaryan <surenb@...gle.com>
> > > Cc: Lokesh Gidra <lokeshgidra@...gle.com>
> > > Cc: Tangquan Zheng <zhengtangquan@...o.com>
> > > Signed-off-by: Barry Song <v-songbaohua@...o.com>
> > > ---
> > >  mm/madvise.c | 34 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 8433ac9b27e0..da016a1d0434 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -1817,6 +1817,39 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
> > >
> > >       if (madvise_should_skip(start, len_in, behavior, &error))
> > >               return error;
> > > +
> > > +     /*
> > > +      * MADV_DONTNEED is commonly used with userspace heaps and most often
> > > +      * affects a single VMA. In these cases, we can use per-VMA locks to
> > > +      * reduce contention on the mmap_lock.
> > > +      */
> > > +     if (behavior == MADV_DONTNEED || behavior == MADV_DONTNEED_LOCKED) {
> >
> > So firstly doing this here means process_madvise() doesn't get this benefit, and
> > we're inconsistent between the two which we really want to avoid.
> >
> > But secondly - we definitely need to find a better way to do this :) this
> > basically follows the 'ignore the existing approach and throw in an if
> > (special case) { ... }' pattern that I feel we really need to do all we can
> > to avoid in the kernel.
> >
> > This lies the way of uffd, hugetlb, and thus horrors beyond imagining.
> >
> > I can see why you did this as this is kind of special-cased a bit, and we
> > already do this kind of thing all over the place but let's try to avoid
> > this here.
> >
> > So I suggest:
> >
> > - Remove any code for this from do_madvise() and thus make it available to
> >   process_madvise() also.
> >
> > - Try to avoid the special casing here as much as humanly possible :)
> >
> > - Update madvise_lock()/unlock() to get passed a pointer to struct
> >   madvise_behavior to which we can add a boolean or even better I think -
> >   an enum indicating which lock type was taken (this can simplify
> >   madvise_unlock() also).
> >
> > - Update madvise_lock() to do all of the checks below, we already
> >   effectively do a switch (behavior) so it's not so crazy to do this. And
> >   you can also do the fallthrough logic there.
> >
> > - Obviously madvise_unlock() can be updated to do vma_end_read().
> >
> > > +             struct vm_area_struct *prev, *vma;
> > > +             unsigned long untagged_start, end;
> > > +
> > > +             untagged_start = untagged_addr(start);
> > > +             end = untagged_start + len_in;
> > > +             vma = lock_vma_under_rcu(mm, untagged_start);
> > > +             if (!vma)
> > > +                     goto lock;
> > > +             if (end > vma->vm_end || userfaultfd_armed(vma)) {
> > > +                     vma_end_read(vma);
> > > +                     goto lock;
> > > +             }
> > > +             if (unlikely(!can_modify_vma_madv(vma, behavior))) {
> > > +                     error = -EPERM;
> > > +                     vma_end_read(vma);
> > > +                     goto out;
> > > +             }
> > > +             madvise_init_tlb(&madv_behavior, mm);
> > > +             error = madvise_dontneed_free(vma, &prev, untagged_start,
> > > +                             end, &madv_behavior);
> > > +             madvise_finish_tlb(&madv_behavior);
> > > +             vma_end_read(vma);
> > > +             goto out;
> > > +     }
> > > +
> > > +lock:
> > >       error = madvise_lock(mm, behavior);
> > >       if (error)
> > >               return error;
> > > @@ -1825,6 +1858,7 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
> > >       madvise_finish_tlb(&madv_behavior);
> > >       madvise_unlock(mm, behavior);
> > >
> > > +out:
> > >       return error;
> > >  }
> > >
> > > --
> > > 2.39.3 (Apple Git-146)
> > >
> >
> > Cheers, Lorenzo

Thanks
Barry

Powered by blists - more mailing lists