On 2024/2/22 23:07, Liam R. Howlett wrote:
* Yajun Deng <yajun.deng@xxxxxxxxx> [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
