Re: [PATCH 07/13] mm/madvise: convert madvise_vma_behavior() to use vma_flags_t

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/5/26 22:30, Lorenzo Stoakes (ARM) wrote:
> Replace use of the legacy vm_flags_t flags with vma_flags_t values
> throughout the madvise logic.
> 
> In order to do so, update ksm_madvise() and hugepage_madvise() to take a
> vma_flags_t pointer, and update their other callers in mremap and the
> powerpc KVM ultravisor code accordingly.
> 
> In the latter case we replace vm_flags_reset() with vma_overwrite_flags().
> The VMA is write locked at this point, so the atomic flags preserved by
> vma_overwrite_flags() cannot differ from those copied, and the behaviour
> is unchanged.
> 
> No functional change intended.
> 
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> ---
>  arch/powerpc/kvm/book3s_hv_uvmem.c | 10 ++++----
>  include/linux/huge_mm.h            |  4 ++--
>  include/linux/ksm.h                |  4 ++--
>  mm/khugepaged.c                    | 10 ++++----
>  mm/ksm.c                           | 11 +++++----
>  mm/madvise.c                       | 47 ++++++++++++++++++++------------------
>  mm/mremap.c                        |  2 +-
>  7 files changed, 46 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 463aef870c4e..0fa50bee2ffc 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -393,7 +393,7 @@ static int kvmppc_memslot_page_merge(struct kvm *kvm,
>  {
>  	unsigned long gfn = memslot->base_gfn;
>  	unsigned long end, start = gfn_to_hva(kvm, gfn);
> -	vm_flags_t vm_flags;
> +	vma_flags_t vma_flags;
>  	int ret = 0;
>  	struct vm_area_struct *vma;
>  	int merge_flag = (merge) ? MADV_MERGEABLE : MADV_UNMERGEABLE;
> @@ -411,15 +411,15 @@ static int kvmppc_memslot_page_merge(struct kvm *kvm,
>  			break;
>  		}
>  		vma_start_write(vma);
> -		/* Copy vm_flags to avoid partial modifications in ksm_madvise */
> -		vm_flags = vma->vm_flags;
> +		/* Copy flags to avoid partial modifications in ksm_madvise */
> +		vma_flags = vma->flags;
>  		ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
> -			  merge_flag, &vm_flags);
> +			  merge_flag, &vma_flags);

Unusual indentation (before your change).

>  		if (ret) {
>  			ret = H_STATE;
>  			break;
>  		}
> -		vm_flags_reset(vma, vm_flags);
> +		vma_overwrite_flags(vma, vma_flags);
>  		start = vma->vm_end;
>  	} while (end > vma->vm_end);
>  
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index c745f7ad2298..8b6f4101eabf 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -508,7 +508,7 @@ change_huge_pud(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  			__split_huge_pud(__vma, __pud, __address);	\
>  	}  while (0)
>  
> -int hugepage_madvise(struct vm_area_struct *vma, vm_flags_t *vm_flags,
> +int hugepage_madvise(struct vm_area_struct *vma, vma_flags_t *vma_flags,
>  		     int advice);
>  int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
>  		     unsigned long end, bool *lock_dropped);
> @@ -710,7 +710,7 @@ static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
>  	do { } while (0)
>  
>  static inline int hugepage_madvise(struct vm_area_struct *vma,
> -				   vm_flags_t *vm_flags, int advice)
> +				   vma_flags_t *vma_flags, int advice)

Could do two tabs while at it.

>  {
>  	return -EINVAL;
>  }
> diff --git a/include/linux/ksm.h b/include/linux/ksm.h
> index d39d0d5483a2..1ffc69a72a1a 100644
> --- a/include/linux/ksm.h
> +++ b/include/linux/ksm.h
> @@ -16,7 +16,7 @@
>  
>  #ifdef CONFIG_KSM
>  int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
> -		unsigned long end, int advice, vm_flags_t *vm_flags);
> +		unsigned long end, int advice, vma_flags_t *vma_flags);
>  vma_flags_t ksm_vma_flags(struct mm_struct *mm, const struct file *file,
>  			  vma_flags_t vma_flags);
>  int ksm_enable_merge_any(struct mm_struct *mm);
> @@ -139,7 +139,7 @@ static inline void collect_procs_ksm(const struct folio *folio,
>  
>  #ifdef CONFIG_MMU
>  static inline int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
> -		unsigned long end, int advice, vm_flags_t *vm_flags)
> +		unsigned long end, int advice, vma_flags_t *vma_flags)
>  {
>  	return 0;
>  }
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index a2c7c6a0768d..be1f779c8da5 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -363,16 +363,16 @@ static unsigned int collapse_max_ptes_swap(struct collapse_control *cc,
>  }
>  
>  int hugepage_madvise(struct vm_area_struct *vma,
> -		     vm_flags_t *vm_flags, int advice)
> +		     vma_flags_t *vma_flags, int advice)
>  {
>  	switch (advice) {
>  	case MADV_HUGEPAGE:
> -		*vm_flags &= ~VM_NOHUGEPAGE;
> -		*vm_flags |= VM_HUGEPAGE;
> +		vma_flags_clear(vma_flags, VMA_NOHUGEPAGE_BIT);
> +		vma_flags_set(vma_flags, VMA_HUGEPAGE_BIT);
>  		break;
>  	case MADV_NOHUGEPAGE:
> -		*vm_flags &= ~VM_HUGEPAGE;
> -		*vm_flags |= VM_NOHUGEPAGE;
> +		vma_flags_clear(vma_flags, VMA_HUGEPAGE_BIT);
> +		vma_flags_set(vma_flags, VMA_NOHUGEPAGE_BIT);
>  		/*
>  		 * Setting VM_NOHUGEPAGE will prevent khugepaged from scanning
>  		 * this vma even if we leave the mm registered in khugepaged if
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 61b772bba229..3eb9c1ec2f87 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -779,7 +779,8 @@ static struct vm_area_struct *find_mergeable_vma(struct mm_struct *mm,
>  	if (ksm_test_exit(mm))
>  		return NULL;
>  	vma = vma_lookup(mm, addr);
> -	if (!vma || !vma_test(vma, VMA_MERGEABLE_BIT) || !vma_has_anon_rmap(vma))
> +	if (!vma || !vma_test(vma, VMA_MERGEABLE_BIT) ||
> +	    !vma_has_anon_rmap(vma))
>  		return NULL;
>  	return vma;
>  }
> @@ -3021,7 +3022,7 @@ int ksm_disable(struct mm_struct *mm)
>  }
>  
>  int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
> -		unsigned long end, int advice, vm_flags_t *vm_flags)
> +		unsigned long end, int advice, vma_flags_t *vma_flags)
>  {
>  	struct mm_struct *mm = vma->vm_mm;
>  	int err;
> @@ -3039,11 +3040,11 @@ int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
>  				return err;
>  		}
>  
> -		*vm_flags |= VM_MERGEABLE;
> +		vma_flags_set(vma_flags, VMA_MERGEABLE_BIT);
>  		break;
>  
>  	case MADV_UNMERGEABLE:
> -		if (!(*vm_flags & VM_MERGEABLE))
> +		if (!vma_flags_test(vma_flags, VMA_MERGEABLE_BIT))
>  			return 0;		/* just ignore the advice */
>  
>  		if (vma_has_anon_rmap(vma)) {
> @@ -3052,7 +3053,7 @@ int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
>  				return err;
>  		}
>  
> -		*vm_flags &= ~VM_MERGEABLE;
> +		vma_flags_clear(vma_flags, VMA_MERGEABLE_BIT);
>  		break;
>  	}
>  
> diff --git a/mm/madvise.c b/mm/madvise.c
> index e9fa4f71efea..1b539e67b00d 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -148,11 +148,10 @@ static int replace_anon_vma_name(struct vm_area_struct *vma,
>   * Update the vm_flags or anon_name on region of a vma, splitting it or merging
>   * it as necessary. Must be called with mmap_lock held for writing.
>   */
> -static int madvise_update_vma(vm_flags_t new_flags,
> +static int madvise_update_vma(vma_flags_t new_vma_flags,
>  		struct madvise_behavior *madv_behavior)
>  {
>  	struct vm_area_struct *vma = madv_behavior->vma;
> -	vma_flags_t new_vma_flags = legacy_to_vma_flags(new_flags);
>  	struct madvise_behavior_range *range = &madv_behavior->range;
>  	struct anon_vma_name *anon_name = madv_behavior->anon_name;
>  	bool set_new_anon_name = madv_behavior->behavior == __MADV_SET_ANON_VMA_NAME;
> @@ -601,7 +600,8 @@ static void madvise_cold_page_range(struct mmu_gather *tlb,
>  
>  static inline bool can_madv_lru_vma(struct vm_area_struct *vma)
>  {
> -	return !(vma->vm_flags & (VM_LOCKED|VM_PFNMAP|VM_HUGETLB));
> +	return !vma_test_any(vma, VMA_LOCKED_BIT, VMA_PFNMAP_BIT,
> +			     VMA_HUGETLB_BIT);
>  }
>  
>  static long madvise_cold(struct madvise_behavior *madv_behavior)
> @@ -650,7 +650,7 @@ static long madvise_pageout(struct madvise_behavior *madv_behavior)
>  	 * further to pageout dirty anon pages.
>  	 */
>  	if (!vma_is_anonymous(vma) && (!can_do_file_pageout(vma) &&
> -				(vma->vm_flags & VM_MAYSHARE)))
> +				vma_test(vma, VMA_MAYSHARE_BIT)))

Another weird indentation. Likely because the spare set of () that are not
necessary?


Acked-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David