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 B3946C021A0 for ; Mon, 17 Feb 2025 04:48:37 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 32C0A28002D; Sun, 16 Feb 2025 23:48:37 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2DCCD28002A; Sun, 16 Feb 2025 23:48:37 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1CBD028002D; Sun, 16 Feb 2025 23:48:37 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id F21F928002A for ; Sun, 16 Feb 2025 23:48:36 -0500 (EST) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 9394A816F1 for ; Mon, 17 Feb 2025 04:48:36 +0000 (UTC) X-FDA: 83128205832.11.0B04EDE Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf10.hostedemail.com (Postfix) with ESMTP id BF228C0002 for ; Mon, 17 Feb 2025 04:48:34 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf10.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1739767715; 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=tW75+n8L4UP9fwvag6ULQcpZ0mCuLWSE3rOx+1N0Hl0=; b=xQe305LU7QmUQI/Ye6pKQdLA/V3uiUFKZy+eOKmtHaZG2/farRxeD9xMibSY9Z8OsWqJRy yXNGCWJbdUKeB0DQxyFNkPGDZt39uk+TczWef0YvujZjS/4Wcu1RDWbEs5bI3dyl/b3GLy OPgggiZEn2VHV/At/RpFuXb+tosENcQ= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1739767715; a=rsa-sha256; cv=none; b=WobUlMW2qLLvHmSAoib0hH6R57kPvDPF9uyWevxk4clPWlhYYt4HQCqLnN2Fk7ifFBQt1s kH6Z7EXpQA5Fo+badTouNp5FxFZekBG7VdO9ZkzSpwM8JBIlJ/TYjErSomYkpaSNd4AMVW uYbbkGW/PLcFLYzxU27D0YRu8Vgn62Y= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf10.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com 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 723011063; Sun, 16 Feb 2025 20:48:53 -0800 (PST) Received: from [10.162.16.135] (unknown [10.162.16.135]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 745A53F59E; Sun, 16 Feb 2025 20:48:29 -0800 (PST) Message-ID: <77dfb8ae-2a57-4d76-a3d2-8a486dd9721b@arm.com> Date: Mon, 17 Feb 2025 10:18:26 +0530 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 16/16] arm64/mm: Defer barriers when updating kernel mappings To: Ryan Roberts , Catalin Marinas , Will Deacon , Muchun Song , Pasha Tatashin , Andrew Morton , Uladzislau Rezki , Christoph Hellwig , Mark Rutland , Ard Biesheuvel , Dev Jain , Alexandre Ghiti , Steve Capper , Kevin Brodsky Cc: linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20250205151003.88959-1-ryan.roberts@arm.com> <20250205151003.88959-17-ryan.roberts@arm.com> <9bc5527e-16f4-45cc-aced-55b1ace6c143@arm.com> <0052097e-2284-4f9e-b37c-2ca2de527667@arm.com> <43d852bb-ab5f-40be-b188-166c57ab795c@arm.com> Content-Language: en-US From: Anshuman Khandual In-Reply-To: <43d852bb-ab5f-40be-b188-166c57ab795c@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Queue-Id: BF228C0002 X-Rspamd-Server: rspam07 X-Stat-Signature: rbsjbm97j37ofkqa9htk6gsszp4h3z1i X-HE-Tag: 1739767714-368553 X-HE-Meta: U2FsdGVkX19EPdFORYn8mUpfwoHs++e0wFeRZKS2+WzmLFsfhw/wPQGJT8/9r+TaTvj/D/KfoG4czjkCi2XnIFWMWhTuNQfWmS3cw5+F//850kIw1cVbuLXMGI1IFT6Uv6Uint3j/ukgFo3PIqnTsvNJhx7UTvF1upNNhh3LKfCVEmu+nIR55fniD+3F7p50aYXZ+rdAa6CzwMj3my/qcZRQEPuPLwCVb42LTnpWBqG+MQ8UrSX3k8PpSZ9+179vbcPUbeLj4Jl++QCVuGBOqfeb+hbFkuFrBWfqQbF5pWjHARiDn/L4iJ2TPqGtlOXmRbfKoBCXhVjYwo43zCMmslKsSCgSe6mBBiem+Bs4oKOnnXiizVxsyRZ0+JSorffVbAeZu0cQpBEIUCOXvhTDsj33URQK3Ii4Gl8Aedm+RC/RiMuM/SGgJerx2ktQze3RUKMgzR/TYkPz8KrENJqWe5rq5pwtWV2RckmWtYtaLZEze0UOYFGLGUecdq3Oo2uUf2EtMhp2cPaENHTPaVQWGm69lQLRSlo+hD4+qvX2iUjXxGqmx5N5thpIPfii4iPwqQW/b6w+EpYGzrlG6elAUEaSvVFfZUYWD0QLRb7ybrs9LgZ8tMl9HBDlkbGfuCqYn0AHcvJWrnnBgEHJL38eQZTAiyGO8qQ1ccEfdXTqsuOUWdwiNnO1i19NVY88X2wMBvALq9WzQ0742byt3sJchoCVzqeYMQpGV9qryUwDYmT/fxSmD468yWJYB7VEbdD4U8bZ2/QXH0lNLu9ku1iQ8+sFlhjC6DZZonvsInJK5nyqh2FyQi7fuKquZSMi7fr41scjnUWR9mauy2y6hBEezz7WkHHyKmv76VzBEA6ieL2BRNW9DGQbRJgtRNgMosE+sB/L0LjVssJFXIUW2MX5Hr9BPrzi8aqhsEfiObv6UdkXdQK91Zb6mmHMACwjbR4pGWsfN1nTQEZoFqPvTJJ t+q1iFHE 5QTI/qz1VnWAIDh9XCv7DGOkrxGwGEKdTfVllPl7VYq1uWGW2UNh1/22H1C5EmLI3YicTxOWFqpa/aEq13tzJGeIXD4Ii5mM+ddbb90yrX6RlelOdO5EFVLdL8f8HTXdUbQhcxQhPZ+dw2/XBoU0H5tScw4vLUNAw5p5KHhDv9kMfoxrGFEypwmy6UaR/gPZ3l+Gw1A8skrXDmtA= 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: List-Subscribe: List-Unsubscribe: On 2/13/25 15:08, Ryan Roberts wrote: > On 13/02/2025 05:30, Anshuman Khandual wrote: >> >> >> On 2/10/25 16:42, Ryan Roberts wrote: >>> On 10/02/2025 08:03, Anshuman Khandual wrote: >>>> >>>> >>>> On 2/5/25 20:39, Ryan Roberts wrote: >>>>> Because the kernel can't tolerate page faults for kernel mappings, when >>>>> setting a valid, kernel space pte (or pmd/pud/p4d/pgd), it emits a >>>>> dsb(ishst) to ensure that the store to the pgtable is observed by the >>>>> table walker immediately. Additionally it emits an isb() to ensure that >>>>> any already speculatively determined invalid mapping fault gets >>>>> canceled.> >>>>> We can improve the performance of vmalloc operations by batching these >>>>> barriers until the end of a set up entry updates. The newly added >>>>> arch_update_kernel_mappings_begin() / arch_update_kernel_mappings_end() >>>>> provide the required hooks. >>>>> >>>>> vmalloc improves by up to 30% as a result. >>>>> >>>>> Two new TIF_ flags are created; TIF_KMAP_UPDATE_ACTIVE tells us if we >>>>> are in the batch mode and can therefore defer any barriers until the end >>>>> of the batch. TIF_KMAP_UPDATE_PENDING tells us if barriers are queued to >>>>> be emited at the end of the batch. >>>> >>>> Why cannot this be achieved with a single TIF_KMAP_UPDATE_ACTIVE which is >>>> set in __begin(), cleared in __end() and saved across a __switch_to(). >>> >>> So unconditionally emit the barriers in _end(), and emit them in __switch_to() >>> if TIF_KMAP_UPDATE_ACTIVE is set? >> >> Right. >> >>> >>> I guess if calling _begin() then you are definitely going to be setting at least >>> 1 PTE. So you can definitely emit the barriers unconditionally. I was trying to >>> protect against the case where you get pre-empted (potentially multiple times) >>> while in the loop. The TIF_KMAP_UPDATE_PENDING flag ensures you only emit the >>> barriers when you definitely need to. Without it, you would have to emit on >>> every pre-emption even if no more PTEs got set. >>> >>> But I suspect this is a premature optimization. Probably it will never occur. So >> >> Agreed. > > Having done this simplification, I've just noticed that one of the > arch_update_kernel_mappings_begin/end callsites is __apply_to_page_range() which > gets called for user space mappings as well as kernel mappings. So actually with > the simplification I'll be emitting barriers even when only user space mappings > were touched. Right, that will not be desirable. > > I think there are a couple of options to fix this: > > - Revert to the 2 flag approach. For the user space case, I'll get to _end() and > notice that no barriers are queued so will emit nothing. > > - Only set TIF_KMAP_UPDATE_ACTIVE if the address range passed to _begin() is a > kernel address range. I guess that's just a case of checking if the MSB is set > in "end"? > > - pass mm to _begin() and only set TIF_KMAP_UPDATE_ACTIVE if mm == &init_mm. I > guess this should be the same as option 2. > > I'm leaning towards option 2. But I have a niggling feeling that my proposed > check isn't quite correct. What do you think? Option 2 and 3 looks better than the two flags approach proposed earlier. But is not option 3 bit more simplistic than option 2 ? Does getting struct mm argument into these function create more code churn ? > > Thanks, > Ryan > > >> >>> I'll simplify as you suggest. >>> >>> Thanks, >>> Ryan >>> >>>> >>>>> >>>>> Signed-off-by: Ryan Roberts >>>>> --- >>>>> arch/arm64/include/asm/pgtable.h | 65 +++++++++++++++++++--------- >>>>> arch/arm64/include/asm/thread_info.h | 2 + >>>>> arch/arm64/kernel/process.c | 20 +++++++-- >>>>> 3 files changed, 63 insertions(+), 24 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h >>>>> index ff358d983583..1ee9b9588502 100644 >>>>> --- a/arch/arm64/include/asm/pgtable.h >>>>> +++ b/arch/arm64/include/asm/pgtable.h >>>>> @@ -39,6 +39,41 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> + >>>>> +static inline void emit_pte_barriers(void) >>>>> +{ >>>>> + dsb(ishst); >>>>> + isb(); >>>>> +} >>>> >>>> There are many sequence of these two barriers in this particular header, >>>> hence probably a good idea to factor this out into a common helper. >>>>>> + >>>>> +static inline void queue_pte_barriers(void) >>>>> +{ >>>>> + if (test_thread_flag(TIF_KMAP_UPDATE_ACTIVE)) { >>>>> + if (!test_thread_flag(TIF_KMAP_UPDATE_PENDING)) >>>>> + set_thread_flag(TIF_KMAP_UPDATE_PENDING); >>>>> + } else >>>>> + emit_pte_barriers(); >>>>> +} >>>>> + >>>>> +#define arch_update_kernel_mappings_begin arch_update_kernel_mappings_begin >>>>> +static inline void arch_update_kernel_mappings_begin(unsigned long start, >>>>> + unsigned long end) >>>>> +{ >>>>> + set_thread_flag(TIF_KMAP_UPDATE_ACTIVE); >>>>> +} >>>>> + >>>>> +#define arch_update_kernel_mappings_end arch_update_kernel_mappings_end >>>>> +static inline void arch_update_kernel_mappings_end(unsigned long start, >>>>> + unsigned long end, >>>>> + pgtbl_mod_mask mask) >>>>> +{ >>>>> + if (test_thread_flag(TIF_KMAP_UPDATE_PENDING)) >>>>> + emit_pte_barriers(); >>>>> + >>>>> + clear_thread_flag(TIF_KMAP_UPDATE_PENDING); >>>>> + clear_thread_flag(TIF_KMAP_UPDATE_ACTIVE); >>>>> +} >>>>> >>>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE >>>>> #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE >>>>> @@ -323,10 +358,8 @@ static inline void __set_pte_complete(pte_t pte) >>>>> * Only if the new pte is valid and kernel, otherwise TLB maintenance >>>>> * or update_mmu_cache() have the necessary barriers. >>>>> */ >>>>> - if (pte_valid_not_user(pte)) { >>>>> - dsb(ishst); >>>>> - isb(); >>>>> - } >>>>> + if (pte_valid_not_user(pte)) >>>>> + queue_pte_barriers(); >>>>> } >>>>> >>>>> static inline void __set_pte(pte_t *ptep, pte_t pte) >>>>> @@ -791,10 +824,8 @@ static inline void set_pmd(pmd_t *pmdp, pmd_t pmd) >>>>> >>>>> WRITE_ONCE(*pmdp, pmd); >>>>> >>>>> - if (pmd_valid_not_user(pmd)) { >>>>> - dsb(ishst); >>>>> - isb(); >>>>> - } >>>>> + if (pmd_valid_not_user(pmd)) >>>>> + queue_pte_barriers(); >>>>> } >>>>> >>>>> static inline void pmd_clear(pmd_t *pmdp) >>>>> @@ -869,10 +900,8 @@ static inline void set_pud(pud_t *pudp, pud_t pud) >>>>> >>>>> WRITE_ONCE(*pudp, pud); >>>>> >>>>> - if (pud_valid_not_user(pud)) { >>>>> - dsb(ishst); >>>>> - isb(); >>>>> - } >>>>> + if (pud_valid_not_user(pud)) >>>>> + queue_pte_barriers(); >>>>> } >>>>> >>>>> static inline void pud_clear(pud_t *pudp) >>>>> @@ -960,10 +989,8 @@ static inline void set_p4d(p4d_t *p4dp, p4d_t p4d) >>>>> >>>>> WRITE_ONCE(*p4dp, p4d); >>>>> >>>>> - if (p4d_valid_not_user(p4d)) { >>>>> - dsb(ishst); >>>>> - isb(); >>>>> - } >>>>> + if (p4d_valid_not_user(p4d)) >>>>> + queue_pte_barriers(); >>>>> } >>>>> >>>>> static inline void p4d_clear(p4d_t *p4dp) >>>>> @@ -1098,10 +1125,8 @@ static inline void set_pgd(pgd_t *pgdp, pgd_t pgd) >>>>> >>>>> WRITE_ONCE(*pgdp, pgd); >>>>> >>>>> - if (pgd_valid_not_user(pgd)) { >>>>> - dsb(ishst); >>>>> - isb(); >>>>> - } >>>>> + if (pgd_valid_not_user(pgd)) >>>>> + queue_pte_barriers(); >>>>> } >>>>> >>>>> static inline void pgd_clear(pgd_t *pgdp) >>>>> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h >>>>> index 1114c1c3300a..382d2121261e 100644 >>>>> --- a/arch/arm64/include/asm/thread_info.h >>>>> +++ b/arch/arm64/include/asm/thread_info.h >>>>> @@ -82,6 +82,8 @@ void arch_setup_new_exec(void); >>>>> #define TIF_SME_VL_INHERIT 28 /* Inherit SME vl_onexec across exec */ >>>>> #define TIF_KERNEL_FPSTATE 29 /* Task is in a kernel mode FPSIMD section */ >>>>> #define TIF_TSC_SIGSEGV 30 /* SIGSEGV on counter-timer access */ >>>>> +#define TIF_KMAP_UPDATE_ACTIVE 31 /* kernel map update in progress */ >>>>> +#define TIF_KMAP_UPDATE_PENDING 32 /* kernel map updated with deferred barriers */ >>>>> >>>>> #define _TIF_SIGPENDING (1 << TIF_SIGPENDING) >>>>> #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) >>>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >>>>> index 42faebb7b712..1367ec6407d1 100644 >>>>> --- a/arch/arm64/kernel/process.c >>>>> +++ b/arch/arm64/kernel/process.c >>>>> @@ -680,10 +680,10 @@ struct task_struct *__switch_to(struct task_struct *prev, >>>>> gcs_thread_switch(next); >>>>> >>>>> /* >>>>> - * Complete any pending TLB or cache maintenance on this CPU in case >>>>> - * the thread migrates to a different CPU. >>>>> - * This full barrier is also required by the membarrier system >>>>> - * call. >>>>> + * Complete any pending TLB or cache maintenance on this CPU in case the >>>>> + * thread migrates to a different CPU. This full barrier is also >>>>> + * required by the membarrier system call. Additionally it is required >>>>> + * for TIF_KMAP_UPDATE_PENDING, see below. >>>>> */ >>>>> dsb(ish); >>>>> >>>>> @@ -696,6 +696,18 @@ struct task_struct *__switch_to(struct task_struct *prev, >>>>> /* avoid expensive SCTLR_EL1 accesses if no change */ >>>>> if (prev->thread.sctlr_user != next->thread.sctlr_user) >>>>> update_sctlr_el1(next->thread.sctlr_user); >>>>> + else if (unlikely(test_thread_flag(TIF_KMAP_UPDATE_PENDING))) { >>>>> + /* >>>>> + * In unlikely event that a kernel map update is on-going when >>>>> + * preemption occurs, we must emit_pte_barriers() if pending. >>>>> + * emit_pte_barriers() consists of "dsb(ishst); isb();". The dsb >>>>> + * is already handled above. The isb() is handled if >>>>> + * update_sctlr_el1() was called. So only need to emit isb() >>>>> + * here if it wasn't called. >>>>> + */ >>>>> + isb(); >>>>> + clear_thread_flag(TIF_KMAP_UPDATE_PENDING); >>>>> + } >>>>> >>>>> /* the actual thread switch */ >>>>> last = cpu_switch_to(prev, next); >>> >