Re: [PATCH v1 9/9] mm: Use unmap_desc struct for freeing page tables.

[Suren Baghdasaryan <surenb@google.com>]

On Tue, Sep 9, 2025 at 12:10 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> Pass through the unmap_desc to free_pgtables() because it almost has
> everything necessary and is already on the stack.
>
> Updates testing code as necessary.
>
> No functional changes intended.
>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>

Reviewed-by: Suren Baghdasaryan <surenb@google.com>

> ---
>  mm/internal.h                    |  5 +----
>  mm/memory.c                      | 21 ++++++++++-----------
>  mm/mmap.c                        |  6 +++---
>  mm/vma.c                         |  7 ++-----
>  tools/testing/vma/vma_internal.h | 11 ++---------
>  5 files changed, 18 insertions(+), 32 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index 1239944f2830a..f22329967e908 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -445,10 +445,7 @@ bool __folio_end_writeback(struct folio *folio);
>  void deactivate_file_folio(struct folio *folio);
>  void folio_activate(struct folio *folio);
>
> -void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
> -                  struct vm_area_struct *start_vma, unsigned long floor,
> -                  unsigned long ceiling, unsigned long tree_max,
> -                  bool mm_wr_locked);
> +void free_pgtables(struct mmu_gather *tlb, struct unmap_desc *desc);
>
>  void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte);
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 8d4d976311037..98c5ffd28a109 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -387,15 +387,14 @@ void free_pgd_range(struct mmu_gather *tlb,
>   * The tree_max differs from the ceiling when a dup_mmap() failed and the tree

"ceiling" is not a parameter anymore, so the comments should also change.

>   * has unrelated data to the mm_struct being torn down.
>   */
> -void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
> -                  struct vm_area_struct *vma, unsigned long floor,
> -                  unsigned long ceiling, unsigned long tree_max,
> -                  bool mm_wr_locked)
> +void free_pgtables(struct mmu_gather *tlb, struct unmap_desc *desc)
>  {
>         struct unlink_vma_file_batch vb;
> +       struct ma_state *mas = desc->mas;
> +       struct vm_area_struct *vma = desc->first;
>
>         /* underflow can happen and is fine */
> -       WARN_ON_ONCE(tree_max - 1 > ceiling - 1);
> +       WARN_ON_ONCE(desc->tree_max - 1 > desc->last_pgaddr - 1);
>
>         tlb_free_vmas(tlb);
>
> @@ -407,13 +406,13 @@ void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
>                  * Note: USER_PGTABLES_CEILING may be passed as ceiling and may
>                  * be 0.  This will underflow and is okay.
>                  */
> -               next = mas_find(mas, tree_max - 1);
> +               next = mas_find(mas, desc->tree_max - 1);
>
>                 /*
>                  * Hide vma from rmap and truncate_pagecache before freeing
>                  * pgtables
>                  */
> -               if (mm_wr_locked)
> +               if (desc->mm_wr_locked)
>                         vma_start_write(vma);
>                 unlink_anon_vmas(vma);
>
> @@ -425,16 +424,16 @@ void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
>                  */
>                 while (next && next->vm_start <= vma->vm_end + PMD_SIZE) {
>                         vma = next;
> -                       next = mas_find(mas, tree_max - 1);
> -                       if (mm_wr_locked)
> +                       next = mas_find(mas, desc->tree_max - 1);
> +                       if (desc->mm_wr_locked)
>                                 vma_start_write(vma);
>                         unlink_anon_vmas(vma);
>                         unlink_file_vma_batch_add(&vb, vma);
>                 }
>                 unlink_file_vma_batch_final(&vb);
>
> -               free_pgd_range(tlb, addr, vma->vm_end,
> -                       floor, next ? next->vm_start : ceiling);
> +               free_pgd_range(tlb, addr, vma->vm_end, desc->first_pgaddr,
> +                              next ? next->vm_start : desc->last_pgaddr);

Much better names IMO. Thank you!

>                 vma = next;
>         } while (vma);
>  }
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 6011f62b0a294..9908481452780 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1311,10 +1311,10 @@ void exit_mmap(struct mm_struct *mm)
>          */
>         mm_flags_set(MMF_OOM_SKIP, mm);
>         mmap_write_lock(mm);
> +       unmap.mm_wr_locked = true;
>         mt_clear_in_rcu(&mm->mm_mt);
> -       vma_iter_set(&vmi, vma->vm_end);
> -       free_pgtables(&tlb, &vmi.mas, vma, FIRST_USER_ADDRESS,
> -                     USER_PGTABLES_CEILING, USER_PGTABLES_CEILING, true);
> +       vma_iter_set(&vmi, unmap.tree_reset);
> +       free_pgtables(&tlb, &unmap);
>         tlb_finish_mmu(&tlb);
>
>         /*
> diff --git a/mm/vma.c b/mm/vma.c
> index ad64cd9795ef3..ba155a539d160 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -476,16 +476,13 @@ void remove_vma(struct vm_area_struct *vma)
>  void unmap_region(struct unmap_desc *desc)
>  {
>         struct mm_struct *mm = desc->first->vm_mm;
> -       struct ma_state *mas = desc->mas;
>         struct mmu_gather tlb;
>
>         tlb_gather_mmu(&tlb, mm);
>         update_hiwater_rss(mm);
>         unmap_vmas(&tlb, desc);
> -       mas_set(mas, desc->tree_reset);
> -       free_pgtables(&tlb, mas, desc->first, desc->first_pgaddr,
> -                     desc->last_pgaddr, desc->tree_max,
> -                     desc->mm_wr_locked);
> +       mas_set(desc->mas, desc->tree_reset);
> +       free_pgtables(&tlb, desc);
>         tlb_finish_mmu(&tlb);
>  }
>
> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index d73ad4747d40a..435c5a24480bc 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -892,17 +892,10 @@ static inline void unmap_vmas(struct mmu_gather *tlb, struct unmap_desc *unmap)
>         (void)unmap;
>  }
>
> -static inline void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
> -                  struct vm_area_struct *vma, unsigned long floor,
> -                  unsigned long ceiling, unsigned long tree_max,
> -                  bool mm_wr_locked)
> +static inline void free_pgtables(struct mmu_gather *tlb, struct unmap_desc *desc)
>  {
>         (void)tlb;
> -       (void)mas;
> -       (void)vma;
> -       (void)floor;
> -       (void)ceiling;
> -       (void)mm_wr_locked;
> +       (void)desc;
>  }
>
>  static inline void mapping_unmap_writable(struct address_space *)
> --
> 2.47.2
>