Re: [PATCH] mm/mmap: convert all mas except mas_detach to vma iterator

[Yajun Deng <yajun.deng@linux.dev>]

On 2024/2/22 23:07, Liam R. Howlett wrote:
> * Yajun Deng <yajun.deng@linux.dev> [240222 03:56]:
> ...
>
>>>>>>> @@ -1959,11 +1958,12 @@ static int expand_upwards(struct vm_area_struct *vma, unsigned long address)
>>>>>>>      	struct vm_area_struct *next;
>>>>>>>      	unsigned long gap_addr;
>>>>>>>      	int error = 0;
>>>>>>> -	MA_STATE(mas, &mm->mm_mt, vma->vm_start, address);
>>>>>>> +	VMA_ITERATOR(vmi, mm, 0);
>>>>>>>      	if (!(vma->vm_flags & VM_GROWSUP))
>>>>>>>      		return -EFAULT;
>>>>>>> +	vma_iter_config(&vmi, vma->vm_start, address);
>>>>> This is confusing.  I think you are doing this so that the vma iterator
>>>>> is set up the same as the maple state, and not what is logically
>>>>> necessary?
>>>> Yes, VMA_ITERATOR can only pass one address.
>>>>
>>>>>>>      	/* Guard against exceeding limits of the address space. */
>>>>>>>      	address &= PAGE_MASK;
>>>>>>>      	if (address >= (TASK_SIZE & PAGE_MASK))
>>>>>>> @@ -1985,15 +1985,15 @@ static int expand_upwards(struct vm_area_struct *vma, unsigned long address)
>>>>>>>      	}
>>>>>>>      	if (next)
>>>>>>> -		mas_prev_range(&mas, address);
>>>>>>> +		mas_prev_range(&vmi.mas, address);
>>>>> This isn't really hiding the maple state.
>>>> Okay,  I will create a new helper function for this in the mm/internal.h.
>>>>
>>>>>>> -	__mas_set_range(&mas, vma->vm_start, address - 1);
>>>>>>> -	if (mas_preallocate(&mas, vma, GFP_KERNEL))
>>>>>>> +	vma_iter_config(&vmi, vma->vm_start, address);
>>>>> The above maple state changes is to get the maple state to point to the
>>>>> correct area for the preallocation call below.  This seems unnecessary
>>>>> to me.
>>>>>
>>>>> We really should just set it up correctly.  Unfortunately, with the VMA
>>>>> iterator, that's not really possible on initialization.
>>>>>
>>>>> What we can do is use the vma->vm_start for the initialization, then use
>>>>> vma_iter_config() here.  That will not reset any state - but that's fine
>>>>> because the preallocation is the first call that actually uses it
>>>>> anyways.
>>>>>
>>>>> So we can initialize with vma->vm_start, don't call vma_iter_config
>>>>> until here, and also drop the if (next) part.
>>>>>
>>>>> This is possible here because it's not optimised like the
>>>>> expand_upwards() case, which uses the state to check prev and avoids an
>>>>> extra walk.
>>>>>
>>>>> Please make sure to test with the ltp tests on the stack combining, etc
>>>>> on a platform that expands down.
>>
>> It seems something wrong about this description. This change is in
>> expand_upwards(), but not in
>>
>> expand_downwards(). So we should test it on a platform that expands up.
> Oh, yes.  Test on the platform that expands upwards would be best.
> Sorry about the mix up.


I didn't have a platform that expands up, so I can't test the 
expand_upwards().

>> And
>> drop the if (next) part
>>
>> is unnecessary. Did I get that right?
> Yes, I think the if (next) part is unnecessary because the maple
> state/vma iterator has not actually moved - we use
> find_vma_intersection() to locate next and not the iterator.  This is
> different than what we do in the expand_downwards.

Yes.

Since I can't test the expand_upwards(), I think it's safer to keep the 
if (next) part.

> Note that, in the even that we reach the limit and cannot return a
> usable address, these functions will call the counterpart and search in
> the opposite direction.
>
>>>> Okay, I will test it.
>>> Testing this can be tricky.  Thanks for looking at it.
>>>
> ...
>
>
> Thanks,
> Liam
>