Re: [PATCH v1 8/9] mm/vma: Use unmap_desc in vms_clear_ptes() and exit_mmap()

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250911 05:53]:
> On Tue, Sep 09, 2025 at 03:09:44PM -0400, Liam R. Howlett wrote:
> > vms_clear_ptes() is slightly different than other callers to
> > unmap_region() and so had the unmapping open-coded.  Using the new
> > structure it is now possible to special-case the struct setup instead of
> > having the open-coded function.
> >
> > exit_mmap() also calls unmap_vmas() with many arguemnts.  Using the
> > unmap_all_init() function to set the unmap descriptor for all vmas makes
> > this a bit easier to read.
> >
> > Update to the vma test code is necessary to ensure testing continues to
> > function.
> >
> > No functional changes intended.
> >
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > ---
> >  include/linux/mm.h               |  3 ---
> >  mm/internal.h                    |  3 +++
> >  mm/memory.c                      | 24 ++++++++------------
> >  mm/mmap.c                        |  5 +++-
> >  mm/vma.c                         | 39 ++++++++++++++++++--------------
> >  mm/vma.h                         | 14 ++++++++++++
> >  tools/testing/vma/vma_internal.h | 14 ++++--------
> >  7 files changed, 56 insertions(+), 46 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 892fe5dbf9de0..23eb59d543390 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2450,9 +2450,6 @@ static inline void zap_vma_pages(struct vm_area_struct *vma)
> >  	zap_page_range_single(vma, vma->vm_start,
> >  			      vma->vm_end - vma->vm_start, NULL);
> >  }
> > -void unmap_vmas(struct mmu_gather *tlb, struct ma_state *mas,
> > -		struct vm_area_struct *start_vma, unsigned long start,
> > -		unsigned long end, unsigned long tree_end, bool mm_wr_locked);
> >
> >  struct mmu_notifier_range;
> >
> > diff --git a/mm/internal.h b/mm/internal.h
> > index d295252407fee..1239944f2830a 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -197,6 +197,9 @@ static inline void vma_close(struct vm_area_struct *vma)
> >  	}
> >  }
> >
> > +/* unmap_vmas is in mm/memory.c */
> > +void unmap_vmas(struct mmu_gather *tlb, struct unmap_desc *unmap);
> > +
> >  #ifdef CONFIG_MMU
> >
> >  /* Flags for folio_pte_batch(). */
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 829cd94950182..8d4d976311037 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -2084,12 +2084,7 @@ static void unmap_single_vma(struct mmu_gather *tlb,
> >  /**
> >   * unmap_vmas - unmap a range of memory covered by a list of vma's
> >   * @tlb: address of the caller's struct mmu_gather
> > - * @mas: the maple state
> > - * @vma: the starting vma
> > - * @start_addr: virtual address at which to start unmapping
> > - * @end_addr: virtual address at which to end unmapping
> > - * @tree_end: The maximum index to check
> > - * @mm_wr_locked: lock flag
> > + * @unmap: The unmap_desc
> >   *
> >   * Unmap all pages in the vma list.
> >   *
> > @@ -2102,11 +2097,9 @@ static void unmap_single_vma(struct mmu_gather *tlb,
> >   * ensure that any thus-far unmapped pages are flushed before unmap_vmas()
> >   * drops the lock and schedules.
> >   */
> > -void unmap_vmas(struct mmu_gather *tlb, struct ma_state *mas,
> > -		struct vm_area_struct *vma, unsigned long start_addr,
> > -		unsigned long end_addr, unsigned long tree_end,
> > -		bool mm_wr_locked)
> > +void unmap_vmas(struct mmu_gather *tlb, struct unmap_desc *unmap)
> >  {
> > +	struct vm_area_struct *vma;
> >  	struct mmu_notifier_range range;
> >  	struct zap_details details = {
> >  		.zap_flags = ZAP_FLAG_DROP_MARKER | ZAP_FLAG_UNMAP,
> > @@ -2114,17 +2107,18 @@ void unmap_vmas(struct mmu_gather *tlb, struct ma_state *mas,
> >  		.even_cows = true,
> >  	};
> >
> > +	vma = unmap->first;
> >  	mmu_notifier_range_init(&range, MMU_NOTIFY_UNMAP, 0, vma->vm_mm,
> > -				start_addr, end_addr);
> > +				unmap->vma_min, unmap->vma_max);
> >  	mmu_notifier_invalidate_range_start(&range);
> >  	do {
> > -		unsigned long start = start_addr;
> > -		unsigned long end = end_addr;
> > +		unsigned long start = unmap->vma_min;
> > +		unsigned long end = unmap->vma_max;
> >  		hugetlb_zap_begin(vma, &start, &end);
> >  		unmap_single_vma(tlb, vma, start, end, &details,
> > -				 mm_wr_locked);
> > +				 unmap->mm_wr_locked);
> >  		hugetlb_zap_end(vma, &details);
> > -		vma = mas_find(mas, tree_end - 1);
> > +		vma = mas_find(unmap->mas, unmap->tree_max - 1);
> 
> An aside, but do kinda see David's point about min, max & start, end & floor
> ceililng, perhaps the prefix_ is enough to differentiate these and we could be
> as consistent as we can be?

Okay, I'll try to use start/end with prefix.  This means we'll have
pagetables ceiling/floor being set to start/end now, but anything we do
here is confusing anyways.  We're going from a state where page tables
used one min/max while vma used another.  I went with min/max because it
was the shortest and would make it more compact.  I used start/end where
they existed previously.

> 
> >  	} while (vma);
> >  	mmu_notifier_invalidate_range_end(&range);
> >  }
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 5c9bd3f20e53f..6011f62b0a294 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1280,10 +1280,12 @@ void exit_mmap(struct mm_struct *mm)
> >  	struct vm_area_struct *vma;
> >  	unsigned long nr_accounted = 0;
> >  	VMA_ITERATOR(vmi, mm, 0);
> > +	struct unmap_desc unmap;
> 
> Perhaps :
> 
> 	struct unmap_desc unnmap = {
> 		.mm_wr_locked = false,
> 	};
> 
> To be clear? I mean it does mean we're initialising some fields twice
> though... but at least avoids uninitialised state if we add new fields.
> 
> However I see you're already seeing mm_wr_locked to false in
> unmap_all_init() anyway?
> 
> Ideally we'd have something like UNMAP_STATE_VMA() here, but ofc you don't
> have the VMA yet.
> 
> So probably something like what you've done here is the way to go, but I'd
> rather we initialise this var in case of adding fields later.
> 
> Then again... you would have to make sure the _init() function set all
> fields anyway so maybe not necessary. Hmm.


The setting of mm_wr_locked = false was not necessary below, so I think
this will be addressed when I drop that.

> 
> >
> >  	/* mm's last user has gone, and its about to be pulled down */
> >  	mmu_notifier_release(mm);
> >
> > +	unmap.mm_wr_locked = false;
> >  	mmap_read_lock(mm);
> >  	arch_exit_mmap(mm);
> >
> > @@ -1295,11 +1297,12 @@ void exit_mmap(struct mm_struct *mm)
> >  		goto destroy;
> >  	}
> >
> > +	unmap_all_init(&unmap, &vmi, vma);
> >  	flush_cache_mm(mm);
> >  	tlb_gather_mmu_fullmm(&tlb, mm);
> >  	/* update_hiwater_rss(mm) here? but nobody should be looking */
> >  	/* Use ULONG_MAX here to ensure all VMAs in the mm are unmapped */
> > -	unmap_vmas(&tlb, &vmi.mas, vma, 0, ULONG_MAX, ULONG_MAX, false);
> > +	unmap_vmas(&tlb, &unmap);
> >  	mmap_read_unlock(mm);
> >
> >  	/*
> > diff --git a/mm/vma.c b/mm/vma.c
> > index c92384975cbb2..ad64cd9795ef3 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -481,8 +481,7 @@ void unmap_region(struct unmap_desc *desc)
> >
> >  	tlb_gather_mmu(&tlb, mm);
> >  	update_hiwater_rss(mm);
> > -	unmap_vmas(&tlb, mas, desc->first, desc->vma_min, desc->vma_max,
> > -		   desc->vma_max, desc->mm_wr_locked);
> > +	unmap_vmas(&tlb, desc);
> 
> Nit, perhaps didn't notice on previous commit actually, but you're being
> inconsistent between naming this 'desc' and 'unmap'. Let's choose one and
> stick with it.

Yeah.  Not sure what's better, I guess unmap - but it's overused.

> 
> >  	mas_set(mas, desc->tree_reset);
> >  	free_pgtables(&tlb, mas, desc->first, desc->first_pgaddr,
> >  		      desc->last_pgaddr, desc->tree_max,
> > @@ -1213,26 +1212,32 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >  static inline void vms_clear_ptes(struct vma_munmap_struct *vms,
> >  		    struct ma_state *mas_detach, bool mm_wr_locked)
> >  {
> > -	struct mmu_gather tlb;
> > +	struct unmap_desc unmap = {
> > +		.mas = mas_detach,
> > +		.first = vms->vma,
> > +		/* start and end may be different if there is no prev or next vma. */
> > +		.first_pgaddr = vms->unmap_start,
> > +		.last_pgaddr = vms->unmap_end,
> > +		.vma_min = vms->start,
> > +		.vma_max = vms->end,
> > +		/*
> > +		 * The tree limits and reset differ from the normal case since it's a
> > +		 * side-tree
> > +		 */
> > +		.tree_reset = 1,
> > +		.tree_max = vms->vma_count,
> > +		/*
> > +		 * We can free page tables without write-locking mmap_lock because VMAs
> > +		 * were isolated before we downgraded mmap_lock.
> > +		 */
> > +		.mm_wr_locked = mm_wr_locked,
> 
> These comments are great thanks! :)
> 
> > +	};
> >
> >  	if (!vms->clear_ptes) /* Nothing to do */
> >  		return;
> >
> > -	/*
> > -	 * We can free page tables without write-locking mmap_lock because VMAs
> > -	 * were isolated before we downgraded mmap_lock.
> > -	 */
> >  	mas_set(mas_detach, 1);
> > -	tlb_gather_mmu(&tlb, vms->vma->vm_mm);
> > -	update_hiwater_rss(vms->vma->vm_mm);
> > -	unmap_vmas(&tlb, mas_detach, vms->vma, vms->start, vms->end,
> > -		   vms->vma_count, mm_wr_locked);
> > -
> > -	mas_set(mas_detach, 1);
> > -	/* start and end may be different if there is no prev or next vma. */
> > -	free_pgtables(&tlb, mas_detach, vms->vma, vms->unmap_start,
> > -		      vms->unmap_end, vms->unmap_end, mm_wr_locked);
> > -	tlb_finish_mmu(&tlb);
> > +	unmap_region(&unmap);
> 
> HMmm why are we removing all these? I'm guessing this is a separate
> refactoring, could we perhaps break this out, as it doesn't seem to quite
> belong in this patch?
> 
> I mean this is really nice :) just I think belongs in another patch, unless
> you feel it's really tied to this one/necessary here?
> 
> Sorry to be a pain... :)

This is the special case that is now handled by setting up the struct
instead of open coding it.

Okay, I'll expand it to more patches.

> 
> >  	vms->clear_ptes = false;
> >  }
> >
> > diff --git a/mm/vma.h b/mm/vma.h
> > index 4edd5d26ffcfc..8b55a0c73d097 100644
> > --- a/mm/vma.h
> > +++ b/mm/vma.h
> > @@ -164,6 +164,20 @@ struct unmap_desc {
> >  	bool mm_wr_locked;            /* If the mmap write lock is held */
> >  };
> >
> > +static inline void unmap_all_init(struct unmap_desc *desc,
> > +		struct vma_iterator *vmi, struct vm_area_struct *vma)
> > +{
> > +	desc->mas = &vmi->mas;
> > +	desc->first = vma;
> > +	desc->first_pgaddr = FIRST_USER_ADDRESS;
> > +	desc->last_pgaddr = USER_PGTABLES_CEILING;
> > +	desc->vma_min = 0;
> > +	desc->vma_max = ULONG_MAX;
> > +	desc->tree_max = ULONG_MAX;
> > +	desc->tree_reset = vma->vm_end;
> > +	desc->mm_wr_locked = false;
> > +}
> > +
> >  #define UNMAP_REGION(name, _vmi, _vma, _vma_min, _vma_max, _prev, _next)      \
> >  	struct unmap_desc name = {                                          \
> >  		.mas = &(_vmi)->mas,                                          \
> > diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> > index 823d379e1fac2..d73ad4747d40a 100644
> > --- a/tools/testing/vma/vma_internal.h
> > +++ b/tools/testing/vma/vma_internal.h
> > @@ -884,18 +884,12 @@ static inline void update_hiwater_vm(struct mm_struct *)
> >  {
> >  }
> >
> > -static inline void unmap_vmas(struct mmu_gather *tlb, struct ma_state *mas,
> > -		      struct vm_area_struct *vma, unsigned long start_addr,
> > -		      unsigned long end_addr, unsigned long tree_end,
> > -		      bool mm_wr_locked)
> > +struct unmap_desc;
> > +
> > +static inline void unmap_vmas(struct mmu_gather *tlb, struct unmap_desc *unmap)
> >  {
> >  	(void)tlb;
> > -	(void)mas;
> > -	(void)vma;
> > -	(void)start_addr;
> > -	(void)end_addr;
> > -	(void)tree_end;
> > -	(void)mm_wr_locked;
> > +	(void)unmap;
> >  }
> 
> Hmm why is this still here, I thought I'd removed all these (void)'s... I think
> actually that's what's in Vlasta's tree, so this will conflict unfortunately.
> 
> Anwyay, please just remove all these (void)'s, they're unnecessary.
> 
> (Also I'm sorry for making updating this file a thing, but I'm not sure there's
> a better way)
> 

Okay, I'll drop the voids.  I thought I got a complain if I didn't have
them (unused variables?).

Thanks,
Liam