On 06/09/2023 10:32, Muchun Song wrote:
>> 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.
This means going back to the v1. I thought we agreed to consolidate/simplify
into one flag, and use @remap_pte to differentiate between split and remap.
