Re: [PATCH 3/5] mm/madvise: thread VMA range state through madvise_behavior

[Zi Yan <ziy@nvidia.com>]

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

> Rather than updating start and a confusing local parameter 'tmp' in
> madvise_walk_vmas(), instead store the current range being operated upon in
> the struct madvise_behavior helper object in a range pair and use this
> consistently in all operations.
>
> This makes it clearer what is going on and opens the door to further
> cleanup now we store state regarding what is currently being operated upon
> here.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  mm/madvise.c | 101 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 55 insertions(+), 46 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 47485653c2a1..6faa38b92111 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -58,17 +58,26 @@ enum madvise_lock_mode {
>  	MADVISE_VMA_READ_LOCK,
>  };
>
> +struct madvise_behavior_range {
> +	unsigned long start, end;
> +};
> +

Declare members separately?

<snip>

> @@ -1425,10 +1437,11 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
>  /*
>   * Error injection support for memory error handling.
>   */
> -static int madvise_inject_error(unsigned long start, unsigned long end,
> -		struct madvise_behavior *madv_behavior)
> +static int madvise_inject_error(struct madvise_behavior *madv_behavior)
>  {
>  	unsigned long size;
> +	unsigned long start = madv_behavior->range.start;
> +	unsigned long end = madv_behavior->range.end;
>
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> @@ -1482,8 +1495,7 @@ static bool is_memory_failure(struct madvise_behavior *madv_behavior)
>
>  #else
>
> -static int madvise_inject_error(unsigned long start, unsigned long end,
> -		struct madvise_behavior *madv_behavior)
> +static int madvise_inject_error(struct madvise_behavior *madv_behavior)
>  {
>  	return 0;
>  }

OK, now I get why you pass struct madvise_behavior to madvise_inject_error()
in Patch 2. The changes make sense to me now. Maybe delay that conversation
in this one.



> @@ -1565,20 +1577,20 @@ static bool process_madvise_remote_valid(int behavior)
>   * If a VMA read lock could not be acquired, we return NULL and expect caller to
>   * fallback to mmap lock behaviour.
>   */
> -static struct vm_area_struct *try_vma_read_lock(struct mm_struct *mm,
> -		struct madvise_behavior *madv_behavior,
> -		unsigned long start, unsigned long end)
> +static
> +struct vm_area_struct *try_vma_read_lock(struct madvise_behavior *madv_behavior)
>  {
> +	struct mm_struct *mm = madv_behavior->mm;

Is the struct mm_struct removal missed in Patch 2?


<snip>

> @@ -1846,22 +1854,23 @@ static int madvise_do_behavior(unsigned long start, size_t len_in,
>  		struct madvise_behavior *madv_behavior)
>  {
>  	struct blk_plug plug;
> -	unsigned long end;
>  	int error;
> +	struct madvise_behavior_range *range = &madv_behavior->range;
>
>  	if (is_memory_failure(madv_behavior)) {
> -		end = start + len_in;
> -		return madvise_inject_error(start, end, madv_behavior);
> +		range->start = start;
> +		range->end = start + len_in;
> +		return madvise_inject_error(madv_behavior);
>  	}
>
> -	start = get_untagged_addr(madv_behavior->mm, start);
> -	end = start + PAGE_ALIGN(len_in);
> +	range->start = get_untagged_addr(madv_behavior->mm, start);
> +	range->end = range->start + PAGE_ALIGN(len_in);
>
>  	blk_start_plug(&plug);
>  	if (is_madvise_populate(madv_behavior))
> -		error = madvise_populate(start, end, madv_behavior);
> +		error = madvise_populate(madv_behavior);
>  	else
> -		error = madvise_walk_vmas(start, end, madv_behavior);
> +		error = madvise_walk_vmas(madv_behavior);
>  	blk_finish_plug(&plug);
>  	return error;
>  }

We almost can pass just struct madvise_behavior to madvise_do_behavior().
I wonder why memory_failure behaves differently.

--
Best Regards,
Yan, Zi