Message-ID: <85146A1C-767D-4761-A37D-3F2248F0C7E7@nvidia.com>
Date: Thu, 19 Jun 2025 21:32:43 -0400
From: Zi Yan <ziy@...dia.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
 David Hildenbrand <david@...hat.com>,
 Baolin Wang <baolin.wang@...ux.alibaba.com>,
 "Liam R . Howlett" <Liam.Howlett@...cle.com>, Nico Pache <npache@...hat.com>,
 Ryan Roberts <ryan.roberts@....com>, Dev Jain <dev.jain@....com>,
 Barry Song <baohua@...nel.org>, Vlastimil Babka <vbabka@...e.cz>,
 Jann Horn <jannh@...gle.com>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, Lance Yang <ioworker0@...il.com>,
 SeongJae Park <sj@...nel.org>, Suren Baghdasaryan <surenb@...gle.com>
Subject: Re: [PATCH 1/5] mm/madvise: remove the visitor pattern and thread
 anon_vma state

On 19 Jun 2025, at 16:26, Lorenzo Stoakes wrote:

> Now we have the madvise_behavior helper struct we no longer need to mess
> around with void* pointers in order to propagate anon_vma_name, and this
> means we can get rid of the confusing and inconsistent visitor pattern
> implementation in madvise_vma_anon_name().
>
> This means we now have a single state object that threads through most of
> madvise()'s logic and a single code path which executes the majority of
> madvise() behaviour (we maintain separate logic for failure injection and
> memory population for the time being).
>
> Note that users cannot inadvertently cause this behaviour to occur, as
> madvise_behavior_valid() would reject it.
>
> Doing this results in a can_modify_vma_madv() check for anonymous VMA name
> changes, however this will cause no issues as this operation is not
> prohibited.
>
> We can also then reuse more code and drop the redundant
> madvise_vma_anon_name() function altogether.
>
> Additionally separate out behaviours that update VMAs from those that do
> not.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> ---
>  mm/madvise.c | 167 +++++++++++++++++++++++++++------------------------
>  1 file changed, 90 insertions(+), 77 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 070132f9842b..9dd935d64692 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -37,6 +37,9 @@
>  #include "internal.h"
>  #include "swap.h"
>
> +#define __MADV_SET_ANON_VMA_NAME (-1)
> +#define __MADV_CLEAR_ANON_VMA_NAME (-2)
> +

These are stored in madvise_behavior.behavior field and used
internally. At least you could add a comment in mman-common.h
about this use, in case someone uses these values for MADV_*.
Yes, these are large values that are very unlikely to be used,
but who knows whether one will use them. :)

>  /*
>   * Maximum number of attempts we make to install guard pages before we give up
>   * and return -ERESTARTNOINTR to have userspace try again.
> @@ -59,9 +62,13 @@ struct madvise_behavior {
>  	int behavior;
>  	struct mmu_gather *tlb;
>  	enum madvise_lock_mode lock_mode;
> +	struct anon_vma_name *anon_name;
>  };
>
>  #ifdef CONFIG_ANON_VMA_NAME
> +static int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> +		unsigned long end, struct madvise_behavior *madv_behavior);
> +
>  struct anon_vma_name *anon_vma_name_alloc(const char *name)
>  {
>  	struct anon_vma_name *anon_name;
> @@ -112,6 +119,48 @@ static int replace_anon_vma_name(struct vm_area_struct *vma,
>
>  	return 0;
>  }
> +
> +int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> +			  unsigned long len_in, struct anon_vma_name *anon_name)
> +{
> +	unsigned long end;
> +	unsigned long len;
> +	struct madvise_behavior madv_behavior = {
> +		.lock_mode = MADVISE_MMAP_WRITE_LOCK,
> +		.anon_name = anon_name,
> +	};
> +
> +	if (start & ~PAGE_MASK)
> +		return -EINVAL;
> +	len = (len_in + ~PAGE_MASK) & PAGE_MASK;
> +
> +	/* Check to see whether len was rounded up from small -ve to zero */
> +	if (len_in && !len)
> +		return -EINVAL;
> +
> +	end = start + len;
> +	if (end < start)
> +		return -EINVAL;
> +
> +	if (end == start)
> +		return 0;
> +
> +	madv_behavior.behavior =
> +		anon_name ? __MADV_SET_ANON_VMA_NAME : __MADV_CLEAR_ANON_VMA_NAME;

How are __MADV_SET_ANON_VMA_NAME and __MADV_CLEAR_ANON_VMA_NAME used?
It seems to me that madvise_vma_behavior() treats them the same.
Why does anon_name is NULL or not make a difference?

> +
> +	return madvise_walk_vmas(mm, start, end, &madv_behavior);
> +}
> +
> +static bool is_anon_vma_name(int behavior)

Maybe update_anon_vma_name()? Otherwise the function reads like
the behavior can be anon_vma_name.

> +{
> +	switch (behavior) {
> +	case __MADV_SET_ANON_VMA_NAME:
> +	case __MADV_CLEAR_ANON_VMA_NAME:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
>  #else /* CONFIG_ANON_VMA_NAME */
>  static int replace_anon_vma_name(struct vm_area_struct *vma,
>  				 struct anon_vma_name *anon_name)
> @@ -121,6 +170,11 @@ static int replace_anon_vma_name(struct vm_area_struct *vma,
>
>  	return 0;
>  }
> +
> +static bool is_anon_vma_name(int behavior)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_ANON_VMA_NAME */
>  /*
>   * Update the vm_flags on region of a vma, splitting it or merging it as
> @@ -1252,13 +1306,12 @@ static long madvise_guard_remove(struct vm_area_struct *vma,
>  static int madvise_vma_behavior(struct vm_area_struct *vma,
>  				struct vm_area_struct **prev,
>  				unsigned long start, unsigned long end,
> -				void *behavior_arg)
> +				struct madvise_behavior *madv_behavior)
>  {
> -	struct madvise_behavior *arg = behavior_arg;
> -	int behavior = arg->behavior;
> -	int error;
> -	struct anon_vma_name *anon_name;
> +	int behavior = madv_behavior->behavior;
> +	struct anon_vma_name *anon_name = madv_behavior->anon_name;
>  	vm_flags_t new_flags = vma->vm_flags;
> +	int error;
>
>  	if (unlikely(!can_modify_vma_madv(vma, behavior)))
>  		return -EPERM;
> @@ -1275,7 +1328,17 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
>  	case MADV_FREE:
>  	case MADV_DONTNEED:
>  	case MADV_DONTNEED_LOCKED:
> -		return madvise_dontneed_free(vma, prev, start, end, arg);
> +		return madvise_dontneed_free(vma, prev, start, end,
> +					     madv_behavior);
> +	case MADV_COLLAPSE:
> +		return madvise_collapse(vma, prev, start, end);
> +	case MADV_GUARD_INSTALL:
> +		return madvise_guard_install(vma, prev, start, end);
> +	case MADV_GUARD_REMOVE:
> +		return madvise_guard_remove(vma, prev, start, end);
> +
> +	/* The below behaviours update VMAs via madvise_update_vma(). */
> +

Great comment and code move!

>  	case MADV_NORMAL:
>  		new_flags = new_flags & ~VM_RAND_READ & ~VM_SEQ_READ;
>  		break;
> @@ -1325,21 +1388,25 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
>  		if (error)
>  			goto out;
>  		break;
> -	case MADV_COLLAPSE:
> -		return madvise_collapse(vma, prev, start, end);
> -	case MADV_GUARD_INSTALL:
> -		return madvise_guard_install(vma, prev, start, end);
> -	case MADV_GUARD_REMOVE:
> -		return madvise_guard_remove(vma, prev, start, end);
> +	case __MADV_SET_ANON_VMA_NAME:
> +	case __MADV_CLEAR_ANON_VMA_NAME:
> +		/* Only anonymous mappings can be named */
> +		if (vma->vm_file && !vma_is_anon_shmem(vma))
> +			return -EBADF;
> +		break;
>  	}

__MADV_SET_ANON_VMA_NAME and __MADV_CLEAR_ANON_VMA_NAME are
used here the code below. I do not see the functional difference
of them. I understand a NULL anon_name means clear the name,
but it is also just set the name to NULL.

>
>  	/* We cannot provide prev in this lock mode. */
> -	VM_WARN_ON_ONCE(arg->lock_mode == MADVISE_VMA_READ_LOCK);
> -	anon_name = anon_vma_name(vma);
> -	anon_vma_name_get(anon_name);
> +	VM_WARN_ON_ONCE(madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK);
> +
> +	if (!is_anon_vma_name(behavior)) {
> +		anon_name = anon_vma_name(vma);
> +		anon_vma_name_get(anon_name);
> +	}
>  	error = madvise_update_vma(vma, prev, start, end, new_flags,
>  				   anon_name);
> -	anon_vma_name_put(anon_name);
> +	if (!is_anon_vma_name(behavior))
> +		anon_vma_name_put(anon_name);

Otherwise, the rest looks good to me.

--
Best Regards,
Yan, Zi

Powered by blists - more mailing lists