Re: [PATCH 4/5] mm/madvise: thread all madvise state through madv_behavior

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

On Fri, Jun 20, 2025 at 04:32:48PM +0200, Vlastimil Babka wrote:
> On 6/19/25 22:26, Lorenzo Stoakes wrote:
> > Doing so means we can get rid of all the weird struct vm_area_struct **prev
> > stuff, everything becomes consistent and in future if we want to make
> > change to behaviour there's a single place where all relevant state is
> > stored.
> >
> > This also allows us to update try_vma_read_lock() to be a little more
> > succinct and set up state for us, as well as cleaning up
> > madvise_update_vma().
> >
> > We also update the debug assertion prior to madvise_update_vma() to assert
> > that this is a write operation as correctly pointed out by Barry in the
> > relevant thread.
> >
> > We can't reasonably update the madvise functions that live outside of
> > mm/madvise.c so we leave those as-is.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

Thanks!

>
> The prev manipulation is indeed confusing, looking forward to the next patch...

Yeah this was the whole motivation for the series, ultimately...

>
> Nits:
>
> > ---
> >  mm/madvise.c | 283 ++++++++++++++++++++++++++-------------------------
> >  1 file changed, 146 insertions(+), 137 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 6faa38b92111..86fe04aa7c88 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -74,6 +74,8 @@ struct madvise_behavior {
> >  	 * traversing multiple VMAs, this is updated for each.
> >  	 */
> >  	struct madvise_behavior_range range;
> > +	/* The VMA and VMA preceding it (if applicable) currently targeted. */
> > +	struct vm_area_struct *prev, *vma;
>
> Would also do separate lines here.

Ack will do.

>
> > -static long madvise_dontneed_free(struct vm_area_struct *vma,
> > -				  struct vm_area_struct **prev,
> > -				  unsigned long start, unsigned long end,
> > -				  struct madvise_behavior *madv_behavior)
> > +static long madvise_dontneed_free(struct madvise_behavior *madv_behavior)
> >  {
> > +	struct mm_struct *mm = madv_behavior->mm;
> > +	struct madvise_behavior_range *range = &madv_behavior->range;
> >  	int behavior = madv_behavior->behavior;
> > -	struct mm_struct *mm = vma->vm_mm;
> >
> > -	*prev = vma;
> > -	if (!madvise_dontneed_free_valid_vma(vma, start, &end, behavior))
> > +	madv_behavior->prev = madv_behavior->vma;
> > +	if (!madvise_dontneed_free_valid_vma(madv_behavior))
> >  		return -EINVAL;
> >
> > -	if (start == end)
> > +	if (range->start == range->end)
> >  		return 0;
> >
> > -	if (!userfaultfd_remove(vma, start, end)) {
> > -		*prev = NULL; /* mmap_lock has been dropped, prev is stale */
> > +	if (!userfaultfd_remove(madv_behavior->vma, range->start, range->end)) {
> > +		struct vm_area_struct *vma;
> > +
> > +		madv_behavior->prev = NULL; /* mmap_lock has been dropped, prev is stale */
> >
> >  		mmap_read_lock(mm);
> > -		vma = vma_lookup(mm, start);
> > +		madv_behavior->vma = vma = vma_lookup(mm, range->start);
>
> This replaces vma in madv_behavior...

Yeah note that the lock is dropped here also :) so the previous VMA is
completely invalidated, so this is valid.

>
> > @@ -1617,23 +1625,19 @@ int madvise_walk_vmas(struct madvise_behavior *madv_behavior)
> >  	struct mm_struct *mm = madv_behavior->mm;
> >  	struct madvise_behavior_range *range = &madv_behavior->range;
> >  	unsigned long end = range->end;
> > -	struct vm_area_struct *vma;
> > -	struct vm_area_struct *prev;
> >  	int unmapped_error = 0;
> >  	int error;
> > +	struct vm_area_struct *vma;
> >
> >  	/*
> >  	 * If VMA read lock is supported, apply madvise to a single VMA
> >  	 * tentatively, avoiding walking VMAs.
> >  	 */
> > -	if (madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK) {
> > -		vma = try_vma_read_lock(madv_behavior);
> > -		if (vma) {
> > -			prev = vma;
> > -			error = madvise_vma_behavior(vma, &prev, madv_behavior);
> > -			vma_end_read(vma);
> > -			return error;
> > -		}
> > +	if (madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK &&
> > +	    try_vma_read_lock(madv_behavior)) {
> > +		error = madvise_vma_behavior(madv_behavior);
> > +		vma_end_read(madv_behavior->vma);
> > +		return error;
>
> And here we could potentially do vma_end_read() on the replaced vma. And
> it's exactly cases using madvise_dontneed_free() that use
> MADVISE_VMA_READ_LOCK mode. But it's not an issue as try_vma_read_lock()
> will fail with uffd and that vma replacement scenario is tied to
> userfaultfd_remove(). It's just quite tricky, hm...

Yeah this kind of thing is explicitly why Barry excluded uffd replace from the
per-VMA lock I think.

So we absolutely can't use the per-VMA lock in conjunction with
uffd_remove(). I'll add a debug assert in madvise_dontneed_free() to make this
clearer.

Will add in madvise_remove() too as that does the same thing there, just to make
sure we have assert coverage.

>
> >  	}
> >
> >  	/*
> > @@ -1641,11 +1645,13 @@ int madvise_walk_vmas(struct madvise_behavior *madv_behavior)
> >  	 * ranges, just ignore them, but return -ENOMEM at the end.
> >  	 * - different from the way of handling in mlock etc.
> >  	 */
> > -	vma = find_vma_prev(mm, range->start, &prev);
> > +	vma = find_vma_prev(mm, range->start, &madv_behavior->prev);
> >  	if (vma && range->start > vma->vm_start)
> > -		prev = vma;
> > +		madv_behavior->prev = vma;
> >
> >  	for (;;) {
> > +		struct vm_area_struct *prev;
> > +
> >  		/* Still start < end. */
> >  		if (!vma)
> >  			return -ENOMEM;
> > @@ -1662,13 +1668,16 @@ int madvise_walk_vmas(struct madvise_behavior *madv_behavior)
> >  		range->end = min(vma->vm_end, end);
> >
> >  		/* Here vma->vm_start <= range->start < range->end <= (end|vma->vm_end). */
> > -		error = madvise_vma_behavior(vma, &prev, madv_behavior);
> > +		madv_behavior->vma = vma;
> > +		error = madvise_vma_behavior(madv_behavior);
> >  		if (error)
> >  			return error;
> > +		prev = madv_behavior->prev;
> > +
> >  		range->start = range->end;
> >  		if (prev && range->start < prev->vm_end)
> >  			range->start = prev->vm_end;
> > -		if (range->start >= range->end)
> > +		if (range->start >= end)
> >  			break;
> >  		if (prev)
> >  			vma = find_vma(mm, prev->vm_end);
>