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

[Liam R. Howlett]

* 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