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

[Zi Yan <ziy@nvidia.com>]

On 19 Jun 2025, at 21:54, Zi Yan wrote:

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

Based on git history, it seems that no one paid attention to
madvise_inject_error() and the [start, start + len_in] has never been
changed since it was added back from 2009.

OK, it seems that Kirill (cc'd) moved start = untagged_addr(start); from
before madvise_inject_error() to after it at commit 428e106ae1ad
("mm: Introduce untagged_addr_remote()"). It changed code behavior.

So memory_failure should get the same range as others, meaning
madvise_do_behavior() can just take struct madvise_behavior
and the range can be set at the call sites.

--
Best Regards,
Yan, Zi