On 20/09/2023 03:47, Muchun Song wrote:
>> On Sep 19, 2023, at 23:09, Joao Martins <joao.m.martins@xxxxxxxxxx> wrote:
>> On 19/09/2023 09:57, Muchun Song wrote:
>>>> On Sep 19, 2023, at 16:55, Joao Martins <joao.m.martins@xxxxxxxxxx> wrote:
>>>> On 19/09/2023 09:41, Muchun Song wrote:
>>>>>> On Sep 19, 2023, at 16:26, Joao Martins <joao.m.martins@xxxxxxxxxx> wrote:
>>>>>> On 19/09/2023 07:42, Muchun Song wrote:
>>>>>>> On 2023/9/19 07:01, Mike Kravetz wrote:
>>>>>>>> list_for_each_entry(folio, folio_list, lru) {
>>>>>>>> int ret = __hugetlb_vmemmap_optimize(h, &folio->page,
>>>>>>>> &vmemmap_pages);
>>>>>>>
>>>>>>> This is unlikely to be failed since the page table allocation
>>>>>>> is moved to the above
>>>>>>
>>>>>>> (Note that the head vmemmap page allocation
>>>>>>> is not mandatory).
>>>>>>
>>>>>> Good point that I almost forgot
>>>>>>
>>>>>>> So we should handle the error case in the above
>>>>>>> splitting operation.
>>>>>>
>>>>>> But back to the previous discussion in v2... the thinking was that /some/ PMDs
>>>>>> got split, and say could allow some PTE remapping to occur and free some pages
>>>>>> back (each page allows 6 more splits worst case). Then the next
>>>>>> __hugetlb_vmemmap_optimize() will have to split PMD pages again for those
>>>>>> hugepages that failed the batch PMD split (as we only defer the PTE remap tlb
>>>>>> flush in this stage).
>>>>>
>>>>> Oh, yes. Maybe we could break the above traversal as early as possible
>>>>> once we enter an ENOMEM?
>>>>>
>>>>
>>>> Sounds good -- no point in keep trying to split if we are failing with OOM.
>>>>
>>>> Perhaps a comment in both of these clauses (the early break on split and the OOM
>>>> handling in batch optimize) could help make this clear.
>>>
>>> Make sense.
>>
>> These are the changes I have so far for this patch based on the discussion so
>> far. For next one it's at the end:
>
> Code looks good to me. One nit below.
>
Thanks
>>
>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
>> index e8bc2f7567db..d9c6f2cf698c 100644
>> --- a/mm/hugetlb_vmemmap.c
>> +++ b/mm/hugetlb_vmemmap.c
>> @@ -27,7 +27,8 @@
>> * @reuse_addr: the virtual address of the @reuse_page page.
>> * @vmemmap_pages: the list head of the vmemmap pages that can be freed
>> * or is mapped from.
>> - * @flags: used to modify behavior in bulk operations
>> + * @flags: used to modify behavior in vmemmap page table walking
>> + * operations.
>> */
>> struct vmemmap_remap_walk {
>> void (*remap_pte)(pte_t *pte, unsigned long addr,
>> @@ -36,6 +37,8 @@ struct vmemmap_remap_walk {
>> struct page *reuse_page;
>> unsigned long reuse_addr;
>> struct list_head *vmemmap_pages;
>> +
>> +/* Skip the TLB flush when we split the PMD */
>> #define VMEMMAP_SPLIT_NO_TLB_FLUSH BIT(0)
>> unsigned long flags;
>> };
>> @@ -132,7 +135,7 @@ static int vmemmap_pmd_range(pud_t *pud, unsigned long addr,
>> int ret;
>>
>> ret = split_vmemmap_huge_pmd(pmd, addr & PMD_MASK,
>> - walk->flags & VMEMMAP_SPLIT_NO_TLB_FLUSH);
>> + !(walk->flags & VMEMMAP_SPLIT_NO_TLB_FLUSH));
>> if (ret)
>> return ret;
>>
>> @@ -677,13 +680,13 @@ 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)
>> +static int 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;
>> + return 0;
>>
>> vmemmap_end = vmemmap_start + hugetlb_vmemmap_size(h);
>> vmemmap_reuse = vmemmap_start;
>> @@ -693,7 +696,7 @@ static void hugetlb_vmemmap_split(const struct hstate *h,
>> struct page *head)
>> * Split PMDs on the vmemmap virtual address range [@vmemmap_start,
>> * @vmemmap_end]
>> */
>> - vmemmap_remap_split(vmemmap_start, vmemmap_end, vmemmap_reuse);
>> + return vmemmap_remap_split(vmemmap_start, vmemmap_end, vmemmap_reuse);
>> }
>>
>> void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head
>> *folio_list)
>> @@ -701,8 +704,18 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h,
>> struct list_head *folio_l
>> struct folio *folio;
>> LIST_HEAD(vmemmap_pages);
>>
>> - list_for_each_entry(folio, folio_list, lru)
>> - hugetlb_vmemmap_split(h, &folio->page);
>> + list_for_each_entry(folio, folio_list, lru) {
>> + int ret = hugetlb_vmemmap_split(h, &folio->page);
>> +
>> + /*
>> + * Spliting the PMD requires allocating a page, thus lets fail
> ^^^^ ^^^
> Splitting page table page
>
> I'd like to specify the functionality of the allocated page.
>
OK
>> + * early once we encounter the first OOM. No point in retrying
>> + * as it can be dynamically done on remap with the memory
>> + * we get back from the vmemmap deduplication.
>> + */
>> + if (ret == -ENOMEM)
>> + break;
>> + }
>>
>> flush_tlb_all();
>>
>> For patch 7, I only have commentary added derived from this earlier discussion
>> above:
>>
>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
>> index d9c6f2cf698c..f6a1020a4b6a 100644
>> --- a/mm/hugetlb_vmemmap.c
>> +++ b/mm/hugetlb_vmemmap.c
>> @@ -40,6 +40,8 @@ struct vmemmap_remap_walk {
>>
>> /* Skip the TLB flush when we split the PMD */
>> #define VMEMMAP_SPLIT_NO_TLB_FLUSH BIT(0)
>> +/* Skip the TLB flush when we remap the PTE */
>> #define VMEMMAP_REMAP_NO_TLB_FLUSH BIT(1)
>> unsigned long flags;
>> };
>>
>> @@ -721,19 +739,28 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h,
>> struct list_head *folio_l
>>
>> list_for_each_entry(folio, folio_list, lru) {
>> int ret = __hugetlb_vmemmap_optimize(h, &folio->page,
>> &vmemmap_pages,
>> VMEMMAP_REMAP_NO_TLB_FLUSH);
>>
>> /*
>> * Pages to be freed may have been accumulated. If we
>> * encounter an ENOMEM, free what we have and try again.
>> + * This can occur in the case that both spliting fails
> ^^^
> splitting
>
ok
>> + * halfway and head page allocation also failed. In this
> ^^^^^^^
> head vmemmap page
>
ok
> Otherwise:
>
> Reviewed-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
>
Thanks, I assume that's for both patches?
> Thanks.
>
>> + * case __hugetlb_vmemmap_optimize() would free memory
>> + * allowing more vmemmap remaps to occur.
>> */
>> if (ret == -ENOMEM && !list_empty(&vmemmap_pages)) {
>>
>
>
