From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id BBB7BCD493B for ; Thu, 21 Sep 2023 01:42:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229763AbjIUBnD (ORCPT ); Wed, 20 Sep 2023 21:43:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40278 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229560AbjIUBnC (ORCPT ); Wed, 20 Sep 2023 21:43:02 -0400 Received: from out-211.mta0.migadu.com (out-211.mta0.migadu.com [IPv6:2001:41d0:1004:224b::d3]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5EB29AF for ; Wed, 20 Sep 2023 18:42:55 -0700 (PDT) Content-Type: text/plain; charset=us-ascii DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1695260570; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Qt4/u/Z/9S9BRMM2co5Z0O1C9g/l+Q+KFSOnwWrp7+U=; b=ttwwEhnqBeL3QRFwp7bl2WRZqLoKC7YJBXDbCmduHiU3UvLiww7rYeJs2Vb1fFIZKhscLa G0AyDyy2LtV/mC2Q0KcDupuJM2iuvRCV6NmqkWBSafKLgr+nxU3ft9IG3u0l2RMS9QffLw 07/i1VGAWivqpI3aHNmOkKgcSm2OPE8= Mime-Version: 1.0 Subject: Re: [PATCH v4 6/8] hugetlb: batch PMD split for bulk vmemmap dedup X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Muchun Song In-Reply-To: <257b5833-5aaf-4748-a576-7610bd36e632@oracle.com> Date: Thu, 21 Sep 2023 09:42:10 +0800 Cc: Mike Kravetz , Muchun Song , Oscar Salvador , David Hildenbrand , Miaohe Lin , David Rientjes , Anshuman Khandual , Naoya Horiguchi , Barry Song <21cnbao@gmail.com>, Michal Hocko , Matthew Wilcox , Xiongchun Duan , Linux-MM , Andrew Morton , LKML 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> <07192BE2-C66E-4F74-8F76-05F57777C6B7@linux.dev> <83B874B6-FF22-4588-90A9-31644D598032@linux.dev> <5bd9c4f5-3411-4327-a495-ce6672150977@oracle.com> <257b5833-5aaf-4748-a576-7610bd36e632@oracle.com> To: Joao Martins X-Migadu-Flow: FLOW_OUT Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > On Sep 20, 2023, at 18:39, Joao Martins = wrote: >=20 > On 20/09/2023 03:47, Muchun Song wrote: >>> On Sep 19, 2023, at 23:09, Joao Martins = wrote: >>> On 19/09/2023 09:57, Muchun Song wrote: >>>>> On Sep 19, 2023, at 16:55, Joao Martins = wrote: >>>>> On 19/09/2023 09:41, Muchun Song wrote: >>>>>>> On Sep 19, 2023, at 16:26, Joao Martins = 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 >>=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)) = {