Re: [PATCH v3 17/23] mm: convert do_brk_flags() to use vma_flags_t

["Lorenzo Stoakes (Oracle)" <ljs@kernel.org>]

On Fri, Mar 20, 2026 at 04:06:15PM +0100, Vlastimil Babka (SUSE) wrote:
> 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);

Right yeah sorry :)

That is clearer but still pretty verbose. Will think about how to do it but I
think as a follow up still makes more sense.

>
> (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.

Cheers!

>
> >>
> >> > +}
> >> >
> >> >  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
>