Re: [PATCH RESEND v2 3/4] mm: sparsemem: use page table lock to protect kernel pmd operations

[Barry Song <21cnbao@gmail.com>]

On Sat, Sep 18, 2021 at 12:09 AM Muchun Song <songmuchun@bytedance.com> wrote:
>
> The init_mm.page_table_lock is used to protect kernel page tables, we
> can use it to serialize splitting vmemmap PMD mappings instead of mmap
> write lock, which can increase the concurrency of vmemmap_remap_free().
>

Curious what is the actual benefit we get in user scenarios from this patch,
1. we set bootargs to reserve hugetlb statically
2. we "echo" some figures to sys or proc.

In other words, Who is going to care about this concurrency?
Can we have some details on this to put in the commit log?

> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/ptdump.c         | 16 ++++++++++++----
>  mm/sparse-vmemmap.c | 49 ++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 46 insertions(+), 19 deletions(-)
>
> diff --git a/mm/ptdump.c b/mm/ptdump.c
> index da751448d0e4..eea3d28d173c 100644
> --- a/mm/ptdump.c
> +++ b/mm/ptdump.c
> @@ -40,8 +40,10 @@ static int ptdump_pgd_entry(pgd_t *pgd, unsigned long addr,
>         if (st->effective_prot)
>                 st->effective_prot(st, 0, pgd_val(val));
>
> -       if (pgd_leaf(val))
> +       if (pgd_leaf(val)) {
>                 st->note_page(st, addr, 0, pgd_val(val));
> +               walk->action = ACTION_CONTINUE;
> +       }
>
>         return 0;
>  }
> @@ -61,8 +63,10 @@ static int ptdump_p4d_entry(p4d_t *p4d, unsigned long addr,
>         if (st->effective_prot)
>                 st->effective_prot(st, 1, p4d_val(val));
>
> -       if (p4d_leaf(val))
> +       if (p4d_leaf(val)) {
>                 st->note_page(st, addr, 1, p4d_val(val));
> +               walk->action = ACTION_CONTINUE;
> +       }
>
>         return 0;
>  }
> @@ -82,8 +86,10 @@ static int ptdump_pud_entry(pud_t *pud, unsigned long addr,
>         if (st->effective_prot)
>                 st->effective_prot(st, 2, pud_val(val));
>
> -       if (pud_leaf(val))
> +       if (pud_leaf(val)) {
>                 st->note_page(st, addr, 2, pud_val(val));
> +               walk->action = ACTION_CONTINUE;
> +       }
>
>         return 0;
>  }
> @@ -101,8 +107,10 @@ static int ptdump_pmd_entry(pmd_t *pmd, unsigned long addr,
>
>         if (st->effective_prot)
>                 st->effective_prot(st, 3, pmd_val(val));
> -       if (pmd_leaf(val))
> +       if (pmd_leaf(val)) {
>                 st->note_page(st, addr, 3, pmd_val(val));
> +               walk->action = ACTION_CONTINUE;
> +       }
>
>         return 0;
>  }
> diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
> index 62e3d20648ce..e636943ccfc4 100644
> --- a/mm/sparse-vmemmap.c
> +++ b/mm/sparse-vmemmap.c
> @@ -64,8 +64,8 @@ struct vmemmap_remap_walk {
>   */
>  #define NR_RESET_STRUCT_PAGE           3
>
> -static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start,
> -                                 struct vmemmap_remap_walk *walk)
> +static int __split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start,
> +                                   struct vmemmap_remap_walk *walk)
>  {
>         pmd_t __pmd;
>         int i;
> @@ -87,15 +87,37 @@ static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start,
>                 set_pte_at(&init_mm, addr, pte, entry);
>         }
>
> -       /* Make pte visible before pmd. See comment in __pte_alloc(). */
> -       smp_wmb();
> -       pmd_populate_kernel(&init_mm, pmd, pgtable);
> +       spin_lock(&init_mm.page_table_lock);
> +       if (likely(pmd_leaf(*pmd))) {
> +               /* Make pte visible before pmd. See comment in __pte_alloc(). */
> +               smp_wmb();
> +               pmd_populate_kernel(&init_mm, pmd, pgtable);
> +               flush_tlb_kernel_range(start, start + PMD_SIZE);
> +               spin_unlock(&init_mm.page_table_lock);
>
> -       flush_tlb_kernel_range(start, start + PMD_SIZE);
> +               return 0;
> +       }
> +       spin_unlock(&init_mm.page_table_lock);
> +       pte_free_kernel(&init_mm, pgtable);
>
>         return 0;
>  }
>
> +static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start,
> +                                 struct vmemmap_remap_walk *walk)
> +{
> +       int ret;
> +
> +       spin_lock(&init_mm.page_table_lock);
> +       ret = pmd_leaf(*pmd);
> +       spin_unlock(&init_mm.page_table_lock);
> +
> +       if (ret)
> +               ret = __split_vmemmap_huge_pmd(pmd, start, walk);
> +
> +       return ret;
> +}
> +
>  static void vmemmap_pte_range(pmd_t *pmd, unsigned long addr,
>                               unsigned long end,
>                               struct vmemmap_remap_walk *walk)
> @@ -132,13 +154,12 @@ static int vmemmap_pmd_range(pud_t *pud, unsigned long addr,
>
>         pmd = pmd_offset(pud, addr);
>         do {
> -               if (pmd_leaf(*pmd)) {
> -                       int ret;
> +               int ret;
> +
> +               ret = split_vmemmap_huge_pmd(pmd, addr & PMD_MASK, walk);
> +               if (ret)
> +                       return ret;
>
> -                       ret = split_vmemmap_huge_pmd(pmd, addr & PMD_MASK, walk);
> -                       if (ret)
> -                               return ret;
> -               }
>                 next = pmd_addr_end(addr, end);
>                 vmemmap_pte_range(pmd, addr, next, walk);
>         } while (pmd++, addr = next, addr != end);
> @@ -321,10 +342,8 @@ int vmemmap_remap_free(unsigned long start, unsigned long end,
>          */
>         BUG_ON(start - reuse != PAGE_SIZE);
>
> -       mmap_write_lock(&init_mm);
> +       mmap_read_lock(&init_mm);
>         ret = vmemmap_remap_range(reuse, end, &walk);
> -       mmap_write_downgrade(&init_mm);
> -
>         if (ret && walk.nr_walked) {
>                 end = reuse + walk.nr_walked * PAGE_SIZE;
>                 /*
> --
> 2.11.0
>

Thanks
barry