From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B3256301704; Fri, 20 Mar 2026 15:06:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774019189; cv=none; b=Lsg6u0i9gcdRLBIRb5iS/LvWuNWiUKmD5qjHkGLGycNvWvvoz0N2r8xWDF2Ih91Hr2wghg2lkexpNQjT5aazfS5Vpop04SXwmB7Pmkgg0jnfyHkfc2doTNo01oQhGRVEMuxJvlXYD6JSLg7CMER29zKLJ0x3AL6Q6/OiZVtfYdU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774019189; c=relaxed/simple; bh=4U/RLB5BQTlqqUA+aRxQ/uoxRsEa6t8UeWu6iGAM7vU=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Wi2a+jkxt2UOQtnM8ELryjGQrVZ8+4qwmPnmmciJ6k7gPUEY/5EVwXqsmTrNPpgIVnsaZAhicL5jvWREve3JPhBnZ4vA8Bom0ewruPga4oKZpoY5HF5+KsGbu3QzvatxgQUOm1oeAEateAXWl0+3bcUUCTkwklAhTF0apmP2Frw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=c11vSApR; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="c11vSApR" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0BB13C2BCB0; Fri, 20 Mar 2026 15:06:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1774019189; bh=4U/RLB5BQTlqqUA+aRxQ/uoxRsEa6t8UeWu6iGAM7vU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=c11vSApRocA25rRA6R1Rg5PoZbJKYkvTzPDMmIS8hDyX2AXYHd8mYvTvGHczpbW0e ZICqmAxAG7qjbn/OIwttlwgweqfJ3T6xWbF6Jn4Ykx0PlThw+3A3UFYPQkowwyM+MR xph2xDZ24qQOpQjw0PB/HBpsqSQshv8HCPxdObcRx0m/O9B6JgKfWn2YSzucm3bNUO LGc+GK60aJmwylKmXisUdB28GjM0TrMGRhwdo3hoTW7hoBzOgalk4j+n72KHrlUXBf swr5E6qUe0gXbEloU8Cxg7ZJR5ROhpNrWzLfTbBhCZSP8AXpxr+lme2+aRUwijSnnb jet5knkPIKlag== Message-ID: <7e2aea13-e047-4891-bd6f-ff6705c4fc28@kernel.org> Date: Fri, 20 Mar 2026 16:06:15 +0100 Precedence: bulk X-Mailing-List: linux-fsdevel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 17/23] mm: convert do_brk_flags() to use vma_flags_t Content-Language: en-US To: "Lorenzo Stoakes (Oracle)" Cc: Andrew Morton , David Hildenbrand , "Liam R . Howlett" , Jann Horn , Pedro Falcato , Mike Rapoport , Suren Baghdasaryan , Kees Cook , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Vineet Gupta , Russell King , Catalin Marinas , Will Deacon , Brian Cain , Huacai Chen , WANG Xuerui , Thomas Bogendoerfer , Dinh Nguyen , Madhavan Srinivasan , Michael Ellerman , Nicholas Piggin , Christophe Leroy , Paul Walmsley , Palmer Dabbelt , Albert Ou , Alexandre Ghiti , Heiko Carstens , Vasily Gorbik , Alexander Gordeev , Christian Borntraeger , Sven Schnelle , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H . Peter Anvin" , Richard Weinberger , Anton Ivanov , Johannes Berg , Alexander Viro , Christian Brauner , Jan Kara , Xu Xin , Chengming Zhou , Michal Hocko , Paul Moore , Stephen Smalley , Ondrej Mosnacek , linux-snps-arc@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-hexagon@vger.kernel.org, loongarch@lists.linux.dev, linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-riscv@lists.infradead.org, linux-s390@vger.kernel.org, linux-um@lists.infradead.org, linux-fsdevel@vger.kernel.org, selinux@vger.kernel.org References: <981ed1afcd19115432e61778e7d226a36f8f5c2b.1773846935.git.ljs@kernel.org> <1d300b3b-2476-4381-b8df-a680f486b284@kernel.org> From: "Vlastimil Babka (SUSE)" In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 3/20/26 14:42, Lorenzo Stoakes (Oracle) wrote: >> >> More nits below: >> >> > diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h >> > index b39cc1127e1f..e25d0d18f6d7 100644 >> > --- a/arch/arm64/include/asm/page.h >> > +++ b/arch/arm64/include/asm/page.h >> > @@ -46,7 +46,12 @@ int pfn_is_map_memory(unsigned long pfn); >> > >> > #endif /* !__ASSEMBLER__ */ >> > >> > -#define VM_DATA_DEFAULT_FLAGS (VM_DATA_FLAGS_TSK_EXEC | VM_MTE_ALLOWED) >> > +#ifdef CONFIG_ARM64_MTE >> > +#define VMA_DATA_DEFAULT_FLAGS append_vma_flags(VMA_DATA_FLAGS_TSK_EXEC, \ >> > + VMA_MTE_ALLOWED_BIT) >> >> I wonder what's the bloat-o-meter impact of these #define's (this >> arm64-specific one isn't the only one) being no longer compile-time-constants? > > I mean there's a precedent for this, but the compiler _should_ figure out this > as a constant value, I have repeatedly confirmed that it's good at that in > godbolt, via make foo/bar.S etc. Great, thanks! > >> >> > */ >> > #define vma_desc_set_flags(desc, ...) \ >> > vma_desc_set_flags_mask(desc, mk_vma_flags(__VA_ARGS__)) >> > @@ -4059,7 +4071,6 @@ extern int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file); >> > extern struct file *get_mm_exe_file(struct mm_struct *mm); >> > extern struct file *get_task_exe_file(struct task_struct *task); >> > >> > -extern bool may_expand_vm(struct mm_struct *, vm_flags_t, unsigned long npages); >> > extern void vm_stat_account(struct mm_struct *, vm_flags_t, long npages); >> > >> > extern bool vma_is_special_mapping(const struct vm_area_struct *vma, >> > diff --git a/mm/internal.h b/mm/internal.h >> > index f98f4746ac41..80d8651441a7 100644 >> >> > diff --git a/mm/mprotect.c b/mm/mprotect.c >> > index 9681f055b9fc..eaa724b99908 100644 >> > --- a/mm/mprotect.c >> > +++ b/mm/mprotect.c >> >> > @@ -773,19 +778,24 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb, >> > >> > change_protection(tlb, vma, start, end, mm_cp_flags); >> > >> > - if ((oldflags & VM_ACCOUNT) && !(newflags & VM_ACCOUNT)) >> > + if (vma_flags_test(&old_vma_flags, VMA_ACCOUNT_BIT) && >> > + !vma_flags_test(&new_vma_flags, VMA_ACCOUNT_BIT)) >> > vm_unacct_memory(nrpages); >> > >> > /* >> > * Private VM_LOCKED VMA becoming writable: trigger COW to avoid major >> > * fault on access. >> > */ >> > - if ((oldflags & (VM_WRITE | VM_SHARED | VM_LOCKED)) == VM_LOCKED && >> > - (newflags & VM_WRITE)) { >> > - populate_vma_page_range(vma, start, end, NULL); >> > + if (vma_flags_test(&new_vma_flags, VMA_WRITE_BIT)) { >> > + const vma_flags_t mask = >> > + vma_flags_and(&old_vma_flags, VMA_WRITE_BIT, >> > + VMA_SHARED_BIT, VMA_LOCKED_BIT); >> > + >> > + if (vma_flags_same(&mask, VMA_LOCKED_BIT)) >> >> That converts the original logic 1:1, but I wonder if it's now feasible to >> write it more obviously as "VMA_LOCKED_BIT must be set, VM_WRITE_BIT and >> VM_SHARED_BIT must not" ? > > Hmm, I'm not sure if I can express this more clearly, it's a pain either > way. Could do: > > if (vma_flags_test(&new_vma_flags, VMA_WRITE_BIT) && > !vma_flags_test_any(&old_vma_flags, VMA_WRITE_BIT, VMA_SHARED_BIT)) > populate_vma_page_range(vma, start, end, NULL); It would be a bit more: if (vma_flags_test(&new_vma_flags, VMA_WRITE_BIT) && vma_flags_test(&old_vma_flags, VMA_LOCKED_BIT) && !vma_flags_test_any(&old_vma_flags, VMA_WRITE_BIT, VMA_SHARED_BIT)) populate_vma_page_range(vma, start, end, NULL); (or reordered in whatever way the short circuting works best here, the original code tested oldflags first). But yeah, at least to me it's more clear what that's testing and doesn't need to set up the intermediate mask variable, and VMA_LOCKED_BIT is there only once in the code now. The number of vma_flags_* operations is the same. >> >> > + populate_vma_page_range(vma, start, end, NULL); >> > } >> > >> > - vm_stat_account(mm, oldflags, -nrpages); >> > + vm_stat_account(mm, vma_flags_to_legacy(old_vma_flags), -nrpages); >> > vm_stat_account(mm, newflags, nrpages); >> > perf_event_mmap(vma); >> > return 0; >> >> > diff --git a/mm/vma.h b/mm/vma.h >> > index cf8926558bf6..1f2de6cb3b97 100644 >> > --- a/mm/vma.h >> > +++ b/mm/vma.h >> >> > +static inline bool is_data_mapping_vma_flags(const vma_flags_t *vma_flags) >> > +{ >> > + const vma_flags_t mask = vma_flags_and(vma_flags, >> > + VMA_WRITE_BIT, VMA_SHARED_BIT, VMA_STACK_BIT); >> > + >> > + return vma_flags_same(&mask, VMA_WRITE_BIT); >> >> Ditto? > > I may do both as a follow up patch given the series is a pain to rework at this point > and I want at least the first version to be like-for-like intentionally. OK sure. >> >> > +} >> > >> > static inline void vma_iter_config(struct vma_iterator *vmi, >> > unsigned long index, unsigned long last) >> > diff --git a/mm/vma_exec.c b/mm/vma_exec.c >> > index 8134e1afca68..5cee8b7efa0f 100644 > > Thanks, Lorenzo