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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9B149C001B0 for ; Fri, 14 Jul 2023 17:59:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 00FCB8D0009; Fri, 14 Jul 2023 13:59:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F02968D0005; Fri, 14 Jul 2023 13:59:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DC9C08D0009; Fri, 14 Jul 2023 13:59:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id C9EAB8D0005 for ; Fri, 14 Jul 2023 13:59:50 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 9A1F0C033E for ; Fri, 14 Jul 2023 17:59:50 +0000 (UTC) X-FDA: 81010980540.07.F3F4E1E Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf06.hostedemail.com (Postfix) with ESMTP id 78E06180010 for ; Fri, 14 Jul 2023 17:59:48 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf06.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1689357589; h=from:from:sender: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=T3tNEGJMwqKf/ASqQpR9OxwcjuK/zkeiXZa7FfOSoNE=; b=qZiuxOoB7m0ryWw2vnN0j7CAYVOyHTwMHPyWsyKePW2pjHjbrzRKi4FSouEMT/ilKPPFrI ixd5I8MCzHzibJFJnB6GiFoa5LpBSQ2Jc5bfioJqRWVBufO8MRpw+yBVSa9mDDbicqtZ6y lSD5KLfyqy7S7amUusnyn9/MSS5mCaM= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf06.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1689357589; a=rsa-sha256; cv=none; b=gNkJZWFWyXYIP4kQxDkITjPYhrM6Ec+/OKwxae3oHexmbDwmh8nVMi4LCV8x/D2Kmjd03v +x0tZoadqVxPzB9Imq1g+Dv03KXdAo6LJkO6M6NNEGCDfgt8fd8eE2iI8f4f+jWjbkbc/p oAkzILERE309zhur0lcHqqJx060BJZw= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 7CC451570; Fri, 14 Jul 2023 11:00:29 -0700 (PDT) Received: from [10.57.76.27] (unknown [10.57.76.27]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 93F7F3F67D; Fri, 14 Jul 2023 10:59:44 -0700 (PDT) Message-ID: <432490d1-8d1e-1742-295a-d6e60a054ab6@arm.com> Date: Fri, 14 Jul 2023 18:59:42 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH v3 3/4] mm: FLEXIBLE_THP for improved performance To: Yu Zhao Cc: Andrew Morton , Matthew Wilcox , "Kirill A. Shutemov" , Yin Fengwei , David Hildenbrand , Catalin Marinas , Will Deacon , Anshuman Khandual , Yang Shi , "Huang, Ying" , Zi Yan , Luis Chamberlain , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org References: <20230714160407.4142030-1-ryan.roberts@arm.com> <20230714161733.4144503-3-ryan.roberts@arm.com> From: Ryan Roberts In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 78E06180010 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: uor7uduxeyb3p6e9e5d8paih66bg8erm X-HE-Tag: 1689357588-171907 X-HE-Meta: U2FsdGVkX1/dOqkv77RAs8S9Zh8OMLChmBoZmEgyRj1i6m49oI92sA35LgL0ef4mS+oNMMhlVQboNuPHJkV4frRd6Sk9Ey9FLrqvak9PMPz3Gboc+CRpvrTQuOv8ebQuo6IqAnCPOl8a9sk+9vWosiBwa//3O3jc/Gx0pxN5SdwDyrWEwy84Ze9jDsayWuuO7Gmkk5PleUuWJNbmsSMCcYIlFmaljQOXE+hbx0Od20EqnrRGMpBrAW9P9mrBBox5yq4aeJzaef/4UGD0MRgObFX3KlEF/InPbHfrLgTFe8Lp+BtshWhYOHuxGbD+EdVyv/aF/esLQLJUUge8v8xfreL4e80hByK1RZ62DT8ciTZKK0SdKRKdk5lkBZBnqn+lRfjOxqC8SoBc2SAyHPSbaRWNdqBjZMFmpGLoZorKWGP0PP32GXQKZA2M+N8xkpXJFKiK7XkbQeWQ8r9FnCkv2vuPKiorRnGXzuGiBeFOenYITNla8Vt+3v6GhJ5iBdpiaBax1Sf5JbQI+jRPkArr4ot5QabhU/aWlebK4IL/+cKTLDVKicq6IcIGyYs9EcA8eznby9UWXl5R7fQLA4A10qFi3W+VhSH2ZkjGEPf72PbGyjXHMvlCkdcPzLq6yC97dXPplxodThDcJ5s1VODScqGl+13MgUDPIP5hLW3Onw+j8d8qcrXQFf5LoDne2Er0pCC1AGaVSRx0IkI1D5K3me5QOXQ11aZY6ntd5d28HAvnURVAIGPxoenoxogqCNy2FRhtJ06zePIJekdSiHN3LB6MNhC6LnRf4vGT34nFwVwNsEk4rGbgYeR3XRBw8Kcrkc98k2iO7FXHufLy8fnLX+36t8P5ho322X87jVdsG1BIvbhjgxmOzTWRvkHW5ZRbpi6WiJST/HGQ6H5ptSn6J81C4cBoPBQpCNR9u3fGp6DnAiRI1W+uXqY6/WO3J6ef2R1NTE/4h9lLMDHD8nW sfp1rf10 P52M+3nCVvj5lpMHbNS1iIKxvOXupGOPg3yB5IFrBR1necLbgbNz4CPmPuI2EcSRgrqc++fHJnTbglINSB5RO7Ewo4lyKmBWfRA6Jg7NS3dKwHHq+/UT7oMzvqt+0yTAF5VH2B77GY8olsEEedG+G4FrhH4KCVyDvu8qayj4GM9D8E8mq+jtKZ5A0p7lOX6oLJe+fu4h51VZQ4T86sJV/fx4MmQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 14/07/2023 18:17, Yu Zhao wrote: > On Fri, Jul 14, 2023 at 10:17 AM Ryan Roberts wrote: >> >> Introduce FLEXIBLE_THP feature, which allows anonymous memory to be >> allocated in large folios of a determined order. All pages of the large >> folio are pte-mapped during the same page fault, significantly reducing >> the number of page faults. The number of per-page operations (e.g. ref >> counting, rmap management lru list management) are also significantly >> reduced since those ops now become per-folio. >> >> The new behaviour is hidden behind the new FLEXIBLE_THP Kconfig, which >> defaults to disabled for now; The long term aim is for this to defaut to >> enabled, but there are some risks around internal fragmentation that >> need to be better understood first. >> >> When enabled, the folio order is determined as such: For a vma, process >> or system that has explicitly disabled THP, we continue to allocate >> order-0. THP is most likely disabled to avoid any possible internal >> fragmentation so we honour that request. >> >> Otherwise, the return value of arch_wants_pte_order() is used. For vmas >> that have not explicitly opted-in to use transparent hugepages (e.g. >> where thp=madvise and the vma does not have MADV_HUGEPAGE), then >> arch_wants_pte_order() is limited by the new cmdline parameter, >> `flexthp_unhinted_max`. This allows for a performance boost without >> requiring any explicit opt-in from the workload while allowing the >> sysadmin to tune between performance and internal fragmentation. >> >> arch_wants_pte_order() can be overridden by the architecture if desired. >> Some architectures (e.g. arm64) can coalsece TLB entries if a contiguous >> set of ptes map physically contigious, naturally aligned memory, so this >> mechanism allows the architecture to optimize as required. >> >> If the preferred order can't be used (e.g. because the folio would >> breach the bounds of the vma, or because ptes in the region are already >> mapped) then we fall back to a suitable lower order; first >> PAGE_ALLOC_COSTLY_ORDER, then order-0. >> >> Signed-off-by: Ryan Roberts >> --- >> .../admin-guide/kernel-parameters.txt | 10 + >> mm/Kconfig | 10 + >> mm/memory.c | 187 ++++++++++++++++-- >> 3 files changed, 190 insertions(+), 17 deletions(-) >> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >> index a1457995fd41..405d624e2191 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -1497,6 +1497,16 @@ >> See Documentation/admin-guide/sysctl/net.rst for >> fb_tunnels_only_for_init_ns >> >> + flexthp_unhinted_max= >> + [KNL] Requires CONFIG_FLEXIBLE_THP enabled. The maximum >> + folio size that will be allocated for an anonymous vma >> + that has neither explicitly opted in nor out of using >> + transparent hugepages. The size must be a power-of-2 in >> + the range [PAGE_SIZE, PMD_SIZE). A larger size improves >> + performance by reducing page faults, while a smaller >> + size reduces internal fragmentation. Default: max(64K, >> + PAGE_SIZE). Format: size[KMG]. >> + > > Let's split this parameter into a separate patch. Ha - I had it as a separate patch originally, but thought you'd ask for it to be a single patch so squashed it ;-). I can separate it again, no problem. > > And I'm going to ask many questions about it (I can live with a sysctl > parameter but this boot parameter is unacceptable to me). Please do. Hopefully the thread with DavidH against v2 gives the rationale. Care to elaborate on why its unacceptable? > >> diff --git a/mm/memory.c b/mm/memory.c >> index 01f39e8144ef..e8bc729efb9d 100644 >> --- a/mm/memory.c >> +++ b/mm/memory.c >> @@ -4050,6 +4050,148 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) >> return ret; >> } >> >> +static bool vmf_pte_range_changed(struct vm_fault *vmf, int nr_pages) >> +{ >> + int i; >> + >> + if (nr_pages == 1) >> + return vmf_pte_changed(vmf); >> + >> + for (i = 0; i < nr_pages; i++) { >> + if (!pte_none(ptep_get_lockless(vmf->pte + i))) >> + return true; >> + } >> + >> + return false; >> +} >> + >> +#ifdef CONFIG_FLEXIBLE_THP >> +static int flexthp_unhinted_max_order = >> + ilog2(SZ_64K > PAGE_SIZE ? SZ_64K : PAGE_SIZE) - PAGE_SHIFT; >> + >> +static int __init parse_flexthp_unhinted_max(char *s) >> +{ >> + unsigned long long size = memparse(s, NULL); >> + >> + if (!is_power_of_2(size) || size < PAGE_SIZE || size > PMD_SIZE) { >> + pr_warn("flexthp: flexthp_unhinted_max=%s must be power-of-2 between PAGE_SIZE (%lu) and PMD_SIZE (%lu), ignoring\n", >> + s, PAGE_SIZE, PMD_SIZE); >> + return 1; >> + } >> + >> + flexthp_unhinted_max_order = ilog2(size) - PAGE_SHIFT; >> + >> + /* THP machinery requires at least 3 struct pages for meta data. */ >> + if (flexthp_unhinted_max_order == 1) >> + flexthp_unhinted_max_order--; >> + >> + return 1; >> +} >> + >> +__setup("flexthp_unhinted_max=", parse_flexthp_unhinted_max); >> + >> +static int anon_folio_order(struct vm_area_struct *vma) >> +{ >> + int order; >> + >> + /* >> + * If THP is explicitly disabled for either the vma, the process or the >> + * system, then this is very likely intended to limit internal >> + * fragmentation; in this case, don't attempt to allocate a large >> + * anonymous folio. >> + * >> + * Else, if the vma is eligible for thp, allocate a large folio of the >> + * size preferred by the arch. Or if the arch requested a very small >> + * size or didn't request a size, then use PAGE_ALLOC_COSTLY_ORDER, >> + * which still meets the arch's requirements but means we still take >> + * advantage of SW optimizations (e.g. fewer page faults). >> + * >> + * Finally if thp is enabled but the vma isn't eligible, take the >> + * arch-preferred size and limit it to the flexthp_unhinted_max cmdline >> + * parameter. This allows a sysadmin to tune performance vs internal >> + * fragmentation. >> + */ >> + >> + if ((vma->vm_flags & VM_NOHUGEPAGE) || >> + test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags) || >> + !hugepage_flags_enabled()) >> + order = 0; >> + else { >> + order = max(arch_wants_pte_order(), PAGE_ALLOC_COSTLY_ORDER); >> + >> + if (!hugepage_vma_check(vma, vma->vm_flags, false, true, true)) >> + order = min(order, flexthp_unhinted_max_order); >> + } >> + >> + return order; >> +} >> + >> +static int alloc_anon_folio(struct vm_fault *vmf, struct folio **folio) >> +{ >> + int i; >> + gfp_t gfp; >> + pte_t *pte; >> + unsigned long addr; >> + struct vm_area_struct *vma = vmf->vma; >> + int prefer = anon_folio_order(vma); >> + int orders[] = { >> + prefer, >> + prefer > PAGE_ALLOC_COSTLY_ORDER ? PAGE_ALLOC_COSTLY_ORDER : 0, >> + 0, >> + }; >> + >> + *folio = NULL; >> + >> + if (vmf_orig_pte_uffd_wp(vmf)) >> + goto fallback; >> + >> + for (i = 0; orders[i]; i++) { >> + addr = ALIGN_DOWN(vmf->address, PAGE_SIZE << orders[i]); >> + if (addr >= vma->vm_start && >> + addr + (PAGE_SIZE << orders[i]) <= vma->vm_end) >> + break; >> + } >> + >> + if (!orders[i]) >> + goto fallback; >> + >> + pte = pte_offset_map(vmf->pmd, vmf->address & PMD_MASK); >> + if (!pte) >> + return -EAGAIN; > > It would be a bug if this happens. So probably -EINVAL? Not sure what you mean? Hugh Dickins' series that went into v6.5-rc1 makes it possible for pte_offset_map() to fail (if I understood correctly) and we have to handle this. The intent is that we will return from the fault without making any change, then we will refault and try again. > >> + >> + for (; orders[i]; i++) { >> + addr = ALIGN_DOWN(vmf->address, PAGE_SIZE << orders[i]); >> + vmf->pte = pte + pte_index(addr); >> + if (!vmf_pte_range_changed(vmf, 1 << orders[i])) >> + break; >> + } >> + >> + vmf->pte = NULL; >> + pte_unmap(pte); >> + >> + gfp = vma_thp_gfp_mask(vma); >> + >> + for (; orders[i]; i++) { >> + addr = ALIGN_DOWN(vmf->address, PAGE_SIZE << orders[i]); >> + *folio = vma_alloc_folio(gfp, orders[i], vma, addr, true); >> + if (*folio) { >> + clear_huge_page(&(*folio)->page, addr, 1 << orders[i]); >> + return 0; >> + } >> + } >> + >> +fallback: >> + *folio = vma_alloc_zeroed_movable_folio(vma, vmf->address); >> + return *folio ? 0 : -ENOMEM; >> +} >> +#else >> +static inline int alloc_anon_folio(struct vm_fault *vmf, struct folio **folio) > > Drop "inline" (it doesn't do anything in .c). There are 38 instances of inline in memory.c alone, so looks like a well used convention, even if the compiler may choose to ignore. Perhaps you can educate me; what's the benefit of dropping it? > > The rest looks good to me. Great - just incase it wasn't obvious, I decided not to overwrite vmf->address with the aligned version, as you suggested, for 2 reasons; 1) address is const in the struct, so would have had to change that. 2) there is a uffd path that can be taken after the vmf->address fixup would have occured and the path consumes that member, so it would have had to be un-fixed-up making it more messy than the way I opted for. Thanks for the quick review as always!