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