Re: [PATCH RFC v2] mm: use per_vma lock for MADV_DONTNEED

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Fri, Jun 06, 2025 at 07:18:25PM +1200, Barry Song wrote:
> On Wed, Jun 4, 2025 at 8:17 AM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Tue, Jun 3, 2025 at 11:43 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > Hi Barry,
> > >
> > > As promised, I enclose a patch to give a sense of how I think we might
> > > thread state through this operation.
> > >
> > > There's a todo on the untagged stuff so you can figure that out. This is
> > > based on the v1 so it might not encompass everything you addressed in the
> > > v2.
> > >
> > > Passing in madv_behavior to madvise_walk_vmas() twice kinda sucks, I
> > > _despise_ the void *arg function ptr stuff there added just for the anon
> > > vma name stuff (ughhh) so might be the only sensible way of threading
> > > state.
> > >
> > > I don't need any attribution, so please use this patch as you see
> > > fit/adapt/delete/do whatever with it, just an easier way for me to show the
> > > idea!
> > >
> > > I did some very basic testing and it seems to work, but nothing deeper.
> > >
> > > Cheers, Lorenzo
>
> Really appreciate your work on this, Lorenzo.

No problem! Hopefully having it in code form makes things clearer on how I
felt we could thread state through here.

The truth is the madvise code, despite SJ's great efforts to improve it
(and indeed he has!), is still a bit of a mess so it's fiddly. More work to
do there I think!

Obviously you are replying to Suren below but I will be so bold as to
quickly give my thoughts here :P

>
> > >
> > > ----8<----
> > > From ff4ba0115cb31a0630b6f8c02c68f11b3fb71f7a Mon Sep 17 00:00:00 2001
> > > From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > > Date: Tue, 3 Jun 2025 18:22:55 +0100
> > > Subject: [PATCH] mm/madvise: support VMA read locks for MADV_DONTNEED[_LOCKED]
> > >
> > > Refactor the madvise() code to retain state about the locking mode utilised
> > > for traversing VMAs.
> > >
> > > Then use this mechanism to permit VMA locking to be done later in the
> > > madvise() logic and also to allow altering of the locking mode to permit
> > > falling back to an mmap read lock if required.
> > >
> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > > ---
> > >  mm/madvise.c | 174 +++++++++++++++++++++++++++++++++++++--------------
> > >  1 file changed, 127 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 5f7a66a1617e..a3a6d73d0bd5 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -48,38 +48,19 @@ struct madvise_walk_private {
> > >         bool pageout;
> > >  };
> > >
> > > +enum madvise_lock_mode {
> > > +       MADVISE_NO_LOCK,
> > > +       MADVISE_MMAP_READ_LOCK,
> > > +       MADVISE_MMAP_WRITE_LOCK,
> > > +       MADVISE_VMA_READ_LOCK,
> > > +};
> > > +
> > >  struct madvise_behavior {
> > >         int behavior;
> > >         struct mmu_gather *tlb;
> > > +       enum madvise_lock_mode lock_mode;
> > >  };
> > >
> > > -/*
> > > - * Any behaviour which results in changes to the vma->vm_flags needs to
> > > - * take mmap_lock for writing. Others, which simply traverse vmas, need
> > > - * to only take it for reading.
> > > - */
> > > -static int madvise_need_mmap_write(int behavior)
> > > -{
> > > -       switch (behavior) {
> > > -       case MADV_REMOVE:
> > > -       case MADV_WILLNEED:
> > > -       case MADV_DONTNEED:
> > > -       case MADV_DONTNEED_LOCKED:
> > > -       case MADV_COLD:
> > > -       case MADV_PAGEOUT:
> > > -       case MADV_FREE:
> > > -       case MADV_POPULATE_READ:
> > > -       case MADV_POPULATE_WRITE:
> > > -       case MADV_COLLAPSE:
> > > -       case MADV_GUARD_INSTALL:
> > > -       case MADV_GUARD_REMOVE:
> > > -               return 0;
> > > -       default:
> > > -               /* be safe, default to 1. list exceptions explicitly */
> > > -               return 1;
> > > -       }
> > > -}
> > > -
> > >  #ifdef CONFIG_ANON_VMA_NAME
> > >  struct anon_vma_name *anon_vma_name_alloc(const char *name)
> > >  {
> > > @@ -1486,6 +1467,43 @@ static bool process_madvise_remote_valid(int behavior)
> > >         }
> > >  }
> > >
> > > +/*
> > > + * Try to acquire a VMA read lock if possible.
> > > + *
> > > + * We only support this lock over a single VMA, which the input range must
> > > + * span.either partially or fully.
> > > + *
> > > + * This function always returns with an appropriate lock held. If a VMA read
> > > + * lock could be acquired, we return the locked VMA.
> > > + *
> > > + * If a VMA read lock could not be acquired, we return NULL and expect caller to
> >
> > Worth mentioning that the function itself will fall back to taking
> > mmap_read_lock in such a case.
> >
> > > + * fallback to mmap lock behaviour.
> > > + */
> > > +static struct vm_area_struct *try_vma_read_lock(struct mm_struct *mm,
> > > +               struct madvise_behavior *madv_behavior,
> > > +               unsigned long start, unsigned long end)
> > > +{
> > > +       struct vm_area_struct *vma;
> > > +
> > > +       if (!madv_behavior || madv_behavior->lock_mode != MADVISE_VMA_READ_LOCK)
> >
> > nit: I think it would be better to do this check before calling
> > try_vma_read_lock(). IMHO it does not make sense to call
> > try_vma_read_lock() when lock_mode != MADVISE_VMA_READ_LOCK. It also
> > makes reading this function easier. The first time I looked at it and
> > saw "return NULL" in one place that takes mmap_read_lock() and another
> > place which returns the same NULL but does not take mmap_lock really
> > confused me.
> >
> > > +               return NULL;
> > > +
> > > +       vma = lock_vma_under_rcu(mm, start);
> > > +       if (!vma)
> > > +               goto take_mmap_read_lock;
> > > +       /* We must span only a single VMA, uffd unsupported. */
> > > +       if (end > vma->vm_end || userfaultfd_armed(vma)) {
> >
> > vma->vm_end is not inclusive, so the above condition I think should be
> > (end >= vma->vm_end || ...)
>
> I don't quite understand why `end > vma->vm_end` would be a problem.
> For a VMA with `vm_start = X` and `vm_end = X + 0x1000`, wouldn't the
> range `(X, X + 0x1000)`—where `end == vm_end`—still be a valid
> candidate for using a per-VMA lock?
>
> We're checking which cases are not eligible for per-VMA locking,
> not which ones are.

Yeah agreed.

>
> >
> > > +               vma_end_read(vma);
> > > +               goto take_mmap_read_lock;
> > > +       }
> > > +       return vma;
> > > +
> > > +take_mmap_read_lock:
> > > +       mmap_read_lock(mm);
> > > +       madv_behavior->lock_mode = MADVISE_MMAP_READ_LOCK;
> > > +       return NULL;
> > > +}
> > > +
> > >  /*
> > >   * Walk the vmas in range [start,end), and call the visit function on each one.
> > >   * The visit function will get start and end parameters that cover the overlap
> > > @@ -1496,7 +1514,8 @@ static bool process_madvise_remote_valid(int behavior)
> > >   */
> > >  static
> > >  int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> > > -                     unsigned long end, void *arg,
> > > +                     unsigned long end, struct madvise_behavior *madv_behavior,
> > > +                     void *arg,
> > >                       int (*visit)(struct vm_area_struct *vma,
> > >                                    struct vm_area_struct **prev, unsigned long start,
> > >                                    unsigned long end, void *arg))
> > > @@ -1505,6 +1524,15 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> > >         struct vm_area_struct *prev;
> > >         unsigned long tmp;
> > >         int unmapped_error = 0;
> > > +       int error;
> > > +
> > > +       /* If VMA read lock supported, we apply advice to a single VMA only. */
> > > +       vma = try_vma_read_lock(mm, madv_behavior, start, end);
> > > +       if (vma) {
> > > +               error = visit(vma, &prev, start, end, arg);
> > > +               vma_end_read(vma);
> > > +               return error;
> > > +       }
> > >
> > >         /*
> > >          * If the interval [start,end) covers some unmapped address
> > > @@ -1516,8 +1544,6 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> > >                 prev = vma;
> > >
> > >         for (;;) {
> > > -               int error;
> > > -
> > >                 /* Still start < end. */
> > >                 if (!vma)
> > >                         return -ENOMEM;
> > > @@ -1598,34 +1624,86 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > >         if (end == start)
> > >                 return 0;
> > >
> > > -       return madvise_walk_vmas(mm, start, end, anon_name,
> > > +       return madvise_walk_vmas(mm, start, end, anon_name, NULL,
>
> I think this should be:
>
>  +       return madvise_walk_vmas(mm, start, end, NULL, anon_name,

Whoops! :) you're right!

I kind of hate having to pass the state like this but I guess for now we'll
live with it and can refactor this whole thing later perhaps.

>
> > >                                  madvise_vma_anon_name);
> > >  }
> > >  #endif /* CONFIG_ANON_VMA_NAME */
> > >
> > > -static int madvise_lock(struct mm_struct *mm, int behavior)
> > > +
> > > +/*
> > > + * Any behaviour which results in changes to the vma->vm_flags needs to
> > > + * take mmap_lock for writing. Others, which simply traverse vmas, need
> > > + * to only take it for reading.
> > > + */
> > > +static enum madvise_lock_mode get_lock_mode(struct madvise_behavior *madv_behavior)
> > >  {
> > > +       int behavior = madv_behavior->behavior;
> > > +
> > >         if (is_memory_failure(behavior))
> > > -               return 0;
> > > +               return MADVISE_NO_LOCK;
> > >
> > > -       if (madvise_need_mmap_write(behavior)) {
> > > +       switch (behavior) {
> > > +       case MADV_REMOVE:
> > > +       case MADV_WILLNEED:
> > > +       case MADV_COLD:
> > > +       case MADV_PAGEOUT:
> > > +       case MADV_FREE:
> > > +       case MADV_POPULATE_READ:
> > > +       case MADV_POPULATE_WRITE:
> > > +       case MADV_COLLAPSE:
> > > +       case MADV_GUARD_INSTALL:
> > > +       case MADV_GUARD_REMOVE:
> > > +               return MADVISE_MMAP_READ_LOCK;
> > > +       case MADV_DONTNEED:
> > > +       case MADV_DONTNEED_LOCKED:
> > > +               return MADVISE_VMA_READ_LOCK;
> > > +       default:
> > > +               return MADVISE_MMAP_WRITE_LOCK;
> > > +       }
> > > +}
> > > +
> > > +static int madvise_lock(struct mm_struct *mm,
> > > +               struct madvise_behavior *madv_behavior)
> > > +{
> > > +       enum madvise_lock_mode lock_mode = get_lock_mode(madv_behavior);
> > > +
> > > +       switch (lock_mode) {
> > > +       case MADVISE_NO_LOCK:
> > > +               break;
> > > +       case MADVISE_MMAP_WRITE_LOCK:
> > >                 if (mmap_write_lock_killable(mm))
> > >                         return -EINTR;
> > > -       } else {
> > > +               break;
> > > +       case MADVISE_MMAP_READ_LOCK:
> > >                 mmap_read_lock(mm);
> > > +               break;
> > > +       case MADVISE_VMA_READ_LOCK:
> > > +               /* We will acquire the lock per-VMA in madvise_walk_vmas(). */
> > > +               break;
> > >         }
> > > +
> > > +       madv_behavior->lock_mode = lock_mode;
> > >         return 0;
> > >  }
> > >
> > > -static void madvise_unlock(struct mm_struct *mm, int behavior)
> > > +static void madvise_unlock(struct mm_struct *mm,
> > > +               struct madvise_behavior *madv_behavior)
> > >  {
> > > -       if (is_memory_failure(behavior))
> > > +       switch (madv_behavior->lock_mode) {
> > > +       case  MADVISE_NO_LOCK:
> > >                 return;
> > > -
> > > -       if (madvise_need_mmap_write(behavior))
> > > +       case MADVISE_MMAP_WRITE_LOCK:
> > >                 mmap_write_unlock(mm);
> > > -       else
> > > +               break;
> > > +       case MADVISE_MMAP_READ_LOCK:
> > >                 mmap_read_unlock(mm);
> > > +               break;
> > > +       case MADVISE_VMA_READ_LOCK:
> > > +               /* We will drop the lock per-VMA in madvise_walk_vmas(). */
> > > +               break;
> > > +       }
> > > +
> > > +       madv_behavior->lock_mode = MADVISE_NO_LOCK;
> > >  }
> > >
> > >  static bool madvise_batch_tlb_flush(int behavior)
> > > @@ -1721,6 +1799,8 @@ static int madvise_do_behavior(struct mm_struct *mm,
> > >
> > >         if (is_memory_failure(behavior))
> > >                 return madvise_inject_error(behavior, start, start + len_in);
> > > +
> > > +       // TODO: handle untagged stuff here...
> > >         start = untagged_addr(start); //untagged_addr_remote(mm, start);
> > >         end = start + PAGE_ALIGN(len_in);
> > >
> > > @@ -1729,7 +1809,7 @@ static int madvise_do_behavior(struct mm_struct *mm,
> > >                 error = madvise_populate(mm, start, end, behavior);
> > >         else
> > >                 error = madvise_walk_vmas(mm, start, end, madv_behavior,
> > > -                                         madvise_vma_behavior);
> > > +                                         madv_behavior, madvise_vma_behavior);
> > >         blk_finish_plug(&plug);
> > >         return error;
> > >  }
> > > @@ -1817,13 +1897,13 @@ 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;
> > > -       error = madvise_lock(mm, behavior);
> > > +       error = madvise_lock(mm, &madv_behavior);
> > >         if (error)
> > >                 return error;
> > >         madvise_init_tlb(&madv_behavior, mm);
> > >         error = madvise_do_behavior(mm, start, len_in, &madv_behavior);
> > >         madvise_finish_tlb(&madv_behavior);
> > > -       madvise_unlock(mm, behavior);
> > > +       madvise_unlock(mm, &madv_behavior);
> > >
> > >         return error;
> > >  }
> > > @@ -1847,7 +1927,7 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
> > >
> > >         total_len = iov_iter_count(iter);
> > >
> > > -       ret = madvise_lock(mm, behavior);
> > > +       ret = madvise_lock(mm, &madv_behavior);
> > >         if (ret)
> > >                 return ret;
> > >         madvise_init_tlb(&madv_behavior, mm);
> > > @@ -1880,8 +1960,8 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
> > >
> > >                         /* Drop and reacquire lock to unwind race. */
> > >                         madvise_finish_tlb(&madv_behavior);
> > > -                       madvise_unlock(mm, behavior);
> > > -                       ret = madvise_lock(mm, behavior);
> > > +                       madvise_unlock(mm, &madv_behavior);
> > > +                       ret = madvise_lock(mm, &madv_behavior);
> > >                         if (ret)
> > >                                 goto out;
> > >                         madvise_init_tlb(&madv_behavior, mm);
> > > @@ -1892,7 +1972,7 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
> > >                 iov_iter_advance(iter, iter_iov_len(iter));
> > >         }
> > >         madvise_finish_tlb(&madv_behavior);
> > > -       madvise_unlock(mm, behavior);
> > > +       madvise_unlock(mm, &madv_behavior);
> > >
> > >  out:
> > >         ret = (total_len - iov_iter_count(iter)) ? : ret;
> > > --
> > > 2.49.0
>
> Thanks
> Barry