Re: [PATCH v4 17/66] mmap: Change zeroing of maple tree in __vma_adjust

[Vlastimil Babka]

On 12/1/21 15:29, Liam Howlett wrote:
> Only write to the maple tree if we are not inserting or the insert isn't
> going to overwrite the area to clear.  This avoids spanning writes and
> node coealescing when unnecessary.
> 
> Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> ---
>  mm/mmap.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 93ed19b2c6ce..c5f92666d145 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -615,6 +615,7 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
>  	bool vma_changed = false;
>  	long adjust_next = 0;
>  	int remove_next = 0;
> +	unsigned long old_start;
>  
>  	if (next && !insert) {
>  		struct vm_area_struct *exporter = NULL, *importer = NULL;
> @@ -740,25 +741,29 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
>  			vma_interval_tree_remove(next, root);
>  	}
>  
> +	old_start = vma->vm_start;
>  	if (start != vma->vm_start) {
> -		if (vma->vm_start < start)
> -			vma_mt_szero(mm, vma->vm_start, start);
> -		else
> -			vma_changed = true;
> +		vma_changed = true;

This says vma_changed = true even if vma is shrinking, so below we might do
an unnecessary vma_store(), no?

>  		vma->vm_start = start;
>  	}
>  	if (end != vma->vm_end) {
> -		if (vma->vm_end > end)
> -			vma_mt_szero(mm, end, vma->vm_end);
> -		else
> +		if (vma->vm_end > end) {

In contrast to the above, here vma_changed is only set when expanding
('vma_expand' would be a more descriptive name maybe?). So why are the two
cases handled differently, am I missing something?

> +			if (!insert || (insert && (insert->vm_start != end)))

Note: thanks to lazy evaluation, "insert &&" should be unnecessary?
More importantly: is "insert->vm_start == end" a guarantee that insert
covers the whole interval from end to vma->vm_end? Probably yes, but a
VM_WARN_ON would be in order?

> +				vma_mt_szero(mm, end, vma->vm_end);

I guess it can't happen that insert would cover a later part of this
interval, so we could zero only between end vna insert->vm_start?

> +		} else
>  			vma_changed = true;

Same nit about { } block as previously.

>  		vma->vm_end = end;
>  		if (!next)
>  			mm->highest_vm_end = vm_end_gap(vma);
>  	}
>  
> -	if (vma_changed)
> +	if (vma_changed) {
>  		vma_store(mm, vma);
> +		if (old_start < start) {
> +			if (insert && (insert->vm_start != old_start))
> +				vma_mt_szero(mm, old_start, start);

This condition looks actively wrong, no zeroing at all if insert is NULL?

> +		}
> +	}
>  
>  	vma->vm_pgoff = pgoff;
>  	if (adjust_next) {