Re: [External] [PATCH v2 09/11] hugetlb: batch PMD split for bulk vmemmap dedup

[Muchun Song]

> On Sep 6, 2023, at 17:26, Joao Martins <joao.m.martins@xxxxxxxxxx> wrote:
> 
> 
> 
> On 06/09/2023 10:11, Muchun Song wrote:
>> On Wed, Sep 6, 2023 at 4:25 PM Muchun Song <muchun.song@xxxxxxxxx> wrote:
>>> 
>>> 
>>> 
>>> On 2023/9/6 05:44, Mike Kravetz wrote:
>>>> From: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>>> 
>>>> In an effort to minimize amount of TLB flushes, batch all PMD splits
>>>> belonging to a range of pages in order to perform only 1 (global) TLB
>>>> flush.
>>>> 
>>>> Rebased and updated by Mike Kravetz
>>>> 
>>>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>>> Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
>>>> ---
>>>>  mm/hugetlb_vmemmap.c | 72 +++++++++++++++++++++++++++++++++++++++++---
>>>>  1 file changed, 68 insertions(+), 4 deletions(-)
>>>> 
>>>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
>>>> index a715712df831..d956551699bc 100644
>>>> --- a/mm/hugetlb_vmemmap.c
>>>> +++ b/mm/hugetlb_vmemmap.c
>>>> @@ -37,7 +37,7 @@ struct vmemmap_remap_walk {
>>>>      struct list_head        *vmemmap_pages;
>>>>  };
>>>> 
>>>> -static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start)
>>>> +static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, bool flush)
>>>>  {
>>>>      pmd_t __pmd;
>>>>      int i;
>>>> @@ -80,7 +80,8 @@ static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start)
>>>>              /* Make pte visible before pmd. See comment in pmd_install(). */
>>>>              smp_wmb();
>>>>              pmd_populate_kernel(&init_mm, pmd, pgtable);
>>>> -             flush_tlb_kernel_range(start, start + PMD_SIZE);
>>>> +             if (flush)
>>>> +                     flush_tlb_kernel_range(start, start + PMD_SIZE);
>>>>      } else {
>>>>              pte_free_kernel(&init_mm, pgtable);
>>>>      }
>>>> @@ -127,11 +128,20 @@ static int vmemmap_pmd_range(pud_t *pud, unsigned long addr,
>>>>      do {
>>>>              int ret;
>>>> 
>>>> -             ret = split_vmemmap_huge_pmd(pmd, addr & PMD_MASK);
>>>> +             ret = split_vmemmap_huge_pmd(pmd, addr & PMD_MASK,
>>>> +                             walk->remap_pte != NULL);
>>> 
>>> It is bettter to only make @walk->remap_pte indicate whether we should go
>>> to the last page table level. I suggest reusing VMEMMAP_NO_TLB_FLUSH
>>> to indicate whether we should flush the TLB at pmd level. It'll be more
>>> clear.
>>> 
>>>>              if (ret)
>>>>                      return ret;
>>>> 
>>>>              next = pmd_addr_end(addr, end);
>>>> +
>>>> +             /*
>>>> +              * We are only splitting, not remapping the hugetlb vmemmap
>>>> +              * pages.
>>>> +              */
>>>> +             if (!walk->remap_pte)
>>>> +                     continue;
>>>> +
>>>>              vmemmap_pte_range(pmd, addr, next, walk);
>>>>      } while (pmd++, addr = next, addr != end);
>>>> 
>>>> @@ -198,7 +208,8 @@ static int vmemmap_remap_range(unsigned long start, unsigned long end,
>>>>                      return ret;
>>>>      } while (pgd++, addr = next, addr != end);
>>>> 
>>>> -     flush_tlb_kernel_range(start, end);
>>>> +     if (walk->remap_pte)
>>>> +             flush_tlb_kernel_range(start, end);
>>>> 
>>>>      return 0;
>>>>  }
>>>> @@ -297,6 +308,35 @@ static void vmemmap_restore_pte(pte_t *pte, unsigned long addr,
>>>>      set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
>>>>  }
>>>> 
>>>> +/**
>>>> + * vmemmap_remap_split - split the vmemmap virtual address range [@start, @end)
>>>> + *                      backing PMDs of the directmap into PTEs
>>>> + * @start:     start address of the vmemmap virtual address range that we want
>>>> + *             to remap.
>>>> + * @end:       end address of the vmemmap virtual address range that we want to
>>>> + *             remap.
>>>> + * @reuse:     reuse address.
>>>> + *
>>>> + * Return: %0 on success, negative error code otherwise.
>>>> + */
>>>> +static int vmemmap_remap_split(unsigned long start, unsigned long end,
>>>> +                             unsigned long reuse)
>>>> +{
>>>> +     int ret;
>>>> +     struct vmemmap_remap_walk walk = {
>>>> +             .remap_pte      = NULL,
>>>> +     };
>>>> +
>>>> +     /* See the comment in the vmemmap_remap_free(). */
>>>> +     BUG_ON(start - reuse != PAGE_SIZE);
>>>> +
>>>> +     mmap_read_lock(&init_mm);
>>>> +     ret = vmemmap_remap_range(reuse, end, &walk);
>>>> +     mmap_read_unlock(&init_mm);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>>  /**
>>>>   * vmemmap_remap_free - remap the vmemmap virtual address range [@start, @end)
>>>>   *                  to the page which @reuse is mapped to, then free vmemmap
>>>> @@ -602,11 +642,35 @@ void hugetlb_vmemmap_optimize(const struct hstate *h, struct page *head)
>>>>      free_vmemmap_page_list(&vmemmap_pages);
>>>>  }
>>>> 
>>>> +static void hugetlb_vmemmap_split(const struct hstate *h, struct page *head)
>>>> +{
>>>> +     unsigned long vmemmap_start = (unsigned long)head, vmemmap_end;
>>>> +     unsigned long vmemmap_reuse;
>>>> +
>>>> +     if (!vmemmap_should_optimize(h, head))
>>>> +             return;
>>>> +
>>>> +     vmemmap_end     = vmemmap_start + hugetlb_vmemmap_size(h);
>>>> +     vmemmap_reuse   = vmemmap_start;
>>>> +     vmemmap_start   += HUGETLB_VMEMMAP_RESERVE_SIZE;
>>>> +
>>>> +     /*
>>>> +      * Split PMDs on the vmemmap virtual address range [@vmemmap_start,
>>>> +      * @vmemmap_end]
>>>> +      */
>>>> +     vmemmap_remap_split(vmemmap_start, vmemmap_end, vmemmap_reuse);
>>>> +}
>>>> +
>>>>  void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head *folio_list)
>>>>  {
>>>>      struct folio *folio;
>>>>      LIST_HEAD(vmemmap_pages);
>>>> 
>>>> +     list_for_each_entry(folio, folio_list, lru)
>>>> +             hugetlb_vmemmap_split(h, &folio->page);
>>> 
>>> Maybe it is reasonable to add a return value to hugetlb_vmemmap_split()
>>> to indicate whether it has done successfully, if it fails, it must be
>>> OOM, in which case, there is no sense to continue to split the page table
>>> and optimize the vmemmap pages subsequently, right?
>> 
>> Sorry, it is reasonable to continue to optimize the vmemmap pages
>> subsequently since it should succeed because those vmemmap pages
>> have been split successfully previously.
>> 
>> Seems we should continue to optimize vmemmap once hugetlb_vmemmap_split()
>> fails, then we will have more memory to continue to split. 
> 
> Good point
> 
>> But it will
>> make hugetlb_vmemmap_optimize_folios() a little complex. I'd like to
>> hear you guys' opinions here.
>> 
> I think it won't add that much complexity if we don't optimize too much of the
> slowpath (when we are out of memory). In the batch freeing patch we could
> additionally test the return value of __hugetlb_vmemmap_optimize() for ENOMEM
> and free the currently stored vmemmap_pages (if any), and keep iterating the
> optimize loop. Should be simple enough and make this a bit more resilient to
> that scenario.

Yep, we could try this.

> But we would need to keep the earlier check you commented above
> (where we use @remap_pte to defer PMD flush).

I think 2 flags will suitable for you, one is VMEMMAP_REMAP_NO_TLB_FLUSH,
another is VMEMMAP_SPLIT_NO_TLB_FLUSH.

Thanks.

> 
>> Thanks.
>> 
>>> 
>>> Thanks.
>>> 
>>>> +
>>>> +     flush_tlb_all();
>>>> +
>>>>      list_for_each_entry(folio, folio_list, lru)
>>>>              __hugetlb_vmemmap_optimize(h, &folio->page, &vmemmap_pages);