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

["Vlastimil Babka (SUSE)" <vbabka@kernel.org>]

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