Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0
Subject: Re: [PATCH v4 6/8] hugetlb: batch PMD split for bulk vmemmap dedup
From:   Muchun Song <muchun.song@linux.dev>
In-Reply-To: <257b5833-5aaf-4748-a576-7610bd36e632@oracle.com>
Date:   Thu, 21 Sep 2023 09:42:10 +0800
Cc:     Mike Kravetz <mike.kravetz@oracle.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Oscar Salvador <osalvador@suse.de>,
        David Hildenbrand <david@redhat.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        David Rientjes <rientjes@google.com>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        Naoya Horiguchi <naoya.horiguchi@linux.dev>,
        Barry Song <21cnbao@gmail.com>, Michal Hocko <mhocko@suse.com>,
        Matthew Wilcox <willy@infradead.org>,
        Xiongchun Duan <duanxiongchun@bytedance.com>,
        Linux-MM <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <98613579-0644-4532-8DCC-D4BA7183AB15@linux.dev>
References: <20230918230202.254631-1-mike.kravetz@oracle.com>
 <20230918230202.254631-7-mike.kravetz@oracle.com>
 <9c627733-e6a2-833b-b0f9-d59552f6ab0d@linux.dev>
 <d8ca9ff5-3160-49a1-947a-de4998887dce@oracle.com>
 <07192BE2-C66E-4F74-8F76-05F57777C6B7@linux.dev>
 <d1c4ee80-2096-4949-af9f-d55dcac071cb@oracle.com>
 <83B874B6-FF22-4588-90A9-31644D598032@linux.dev>
 <5bd9c4f5-3411-4327-a495-ce6672150977@oracle.com>
 <A8712DC1-7A22-4A59-BB2C-F7089D8FCC16@linux.dev>
 <257b5833-5aaf-4748-a576-7610bd36e632@oracle.com>
To:     Joao Martins <joao.m.martins@oracle.com>
Precedence: bulk


> On Sep 20, 2023, at 18:39, Joao Martins <joao.m.martins@oracle.com> =
wrote:
>=20
> On 20/09/2023 03:47, Muchun Song wrote:
>>> On Sep 19, 2023, at 23:09, Joao Martins <joao.m.martins@oracle.com> =
wrote:
>>> On 19/09/2023 09:57, Muchun Song wrote:
>>>>> On Sep 19, 2023, at 16:55, Joao Martins =
<joao.m.martins@oracle.com> wrote:
>>>>> On 19/09/2023 09:41, Muchun Song wrote:
>>>>>>> On Sep 19, 2023, at 16:26, Joao Martins =
<joao.m.martins@oracle.com> 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 =3D __hugetlb_vmemmap_optimize(h, &folio->page,
>>>>>>>>>                               &vmemmap_pages);
>>>>>>>>=20
>>>>>>>> This is unlikely to be failed since the page table allocation
>>>>>>>> is moved to the above=20
>>>>>>>=20
>>>>>>>> (Note that the head vmemmap page allocation
>>>>>>>> is not mandatory).=20
>>>>>>>=20
>>>>>>> Good point that I almost forgot
>>>>>>>=20
>>>>>>>> So we should handle the error case in the above
>>>>>>>> splitting operation.
>>>>>>>=20
>>>>>>> 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).
>>>>>>=20
>>>>>> Oh, yes. Maybe we could break the above traversal as early as =
possible
>>>>>> once we enter an ENOMEM?
>>>>>>=20
>>>>>=20
>>>>> Sounds good -- no point in keep trying to split if we are failing =
with OOM.
>>>>>=20
>>>>> 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.
>>>>=20
>>>> Make sense.
>>>=20
>>> 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:
>>=20
>> Code looks good to me. One nit below.
>>=20
> Thanks
>=20
>>>=20
>>> 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;
>>>=20
>>>               ret =3D 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;
>>>=20
>>> @@ -677,13 +680,13 @@ void hugetlb_vmemmap_optimize(const struct =
hstate *h,
>>> struct page *head)
>>>       free_vmemmap_page_list(&vmemmap_pages);
>>> }
>>>=20
>>> -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 =3D (unsigned long)head, =
vmemmap_end;
>>>       unsigned long vmemmap_reuse;
>>>=20
>>>       if (!vmemmap_should_optimize(h, head))
>>> -               return;
>>> +               return 0;
>>>=20
>>>       vmemmap_end     =3D vmemmap_start + hugetlb_vmemmap_size(h);
>>>       vmemmap_reuse   =3D 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);
>>> }
>>>=20
>>> 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);
>>>=20
>>> -       list_for_each_entry(folio, folio_list, lru)
>>> -               hugetlb_vmemmap_split(h, &folio->page);
>>> +       list_for_each_entry(folio, folio_list, lru) {
>>> +               int ret =3D hugetlb_vmemmap_split(h, &folio->page);
>>> +
>>> +               /*
>>> +                * Spliting the PMD requires allocating a page, thus =
lets fail
>>                      ^^^^                                 ^^^
>>                    Splitting                           page table =
page
>>=20
>> I'd like to specify the functionality of the allocated page.
>>=20
> OK
>=20
>>> +                * 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 =3D=3D -ENOMEM)
>>> +                       break;
>>> +       }
>>>=20
>>>       flush_tlb_all();
>>>=20
>>> For patch 7, I only have commentary added derived from this earlier =
discussion
>>> above:
>>>=20
>>> 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 {
>>>=20
>>> /* 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;
>>> };
>>>=20
>>> @@ -721,19 +739,28 @@ void hugetlb_vmemmap_optimize_folios(struct =
hstate *h,
>>> struct list_head *folio_l
>>>=20
>>>       list_for_each_entry(folio, folio_list, lru) {
>>>               int ret =3D __hugetlb_vmemmap_optimize(h, =
&folio->page,
>>>                                              &vmemmap_pages,
>>>                                              =
VMEMMAP_REMAP_NO_TLB_FLUSH);
>>>=20
>>>               /*
>>>                * 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
>>=20
>=20
> ok
>=20
>>> +                * halfway and head page allocation also failed. In =
this
>>                                 ^^^^^^^
>>                            head vmemmap page
>>=20
> ok
>=20
>> Otherwise:
>>=20
>> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
>>=20
>=20
> Thanks, I assume that's for both patches?

Yes. Thanks.

>=20
>> Thanks.
>>=20
>>> +                * case __hugetlb_vmemmap_optimize() would free =
memory
>>> +                * allowing more vmemmap remaps to occur.
>>>                */
>>>               if (ret =3D=3D -ENOMEM && !list_empty(&vmemmap_pages)) =
{