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) {
