Re: [PATCH 10/10] mm: rework vm_ops->close() handling on VMA merge

["Petr Tesařík" <petr@tesarici.cz>]

On Tue, 6 Aug 2024 15:08:33 +0100
Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

> On Tue, Aug 06, 2024 at 03:55:55PM GMT, Petr Tesařík wrote:
> > On Mon,  5 Aug 2024 13:13:57 +0100
> > Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
>[...]
> > > @@ -886,6 +894,8 @@ struct vm_area_struct *vma_merge_new_vma(struct vma_merge_struct *vmg)
> > >  	unsigned long end = vmg->end;
> > >  	pgoff_t pgoff = vmg->pgoff;
> > >  	pgoff_t pglen = PHYS_PFN(end - start);
> > > +	bool merge_next = false;
> > > +	struct anon_vma *anon_vma = vmg->anon_vma;  
> >
> > Calling this "anon_vma" feels a bit too generic. IIUC you want to save
> > the original vmg->anon_vma in case the VMA turns out to be ummergeable
> > with the next VMA after vmg->anon_vma has already been modified.
> >
> > What about calling it "orig_anon_vma"?  
> 
> I disagree, that'd be unnecessary noise (and this is applicable to _all_
> the fields).

I'm afraid I don't understand what you mean with _all_ fields. FWIW my
comment concerns a local variable called "anon_vma", not a struct
member called "anon_vma".

> 
> Again we come to some trade-off between readability and inherent
> complexity. I am not a fan of making variable names unnecessarily
> overwrought.

Then call it "a". ;-)

See additional comments below:

> 
> In this case it's just a short-hand, as the only instance where we'd retry
> the operation anon_vma would be NULL (from mmap_region()), so we reset that
> to NULL, however strictly we should reset to anon_vma.
> 
> I'll change that on the next respin just to be strict.
> 
> >
> > Petr T
> >  
> > >
> > >  	VM_WARN_ON(vmg->vma);
> > >
> > > @@ -916,8 +926,9 @@ struct vm_area_struct *vma_merge_new_vma(struct vma_merge_struct *vmg)
> > >  		vmg->end = next->vm_end;
> > >  		vmg->vma = next;
> > >  		vmg->pgoff = next->vm_pgoff - pglen;
> > > -
> > >  		vmg->anon_vma = next->anon_vma;

Here, vmg->anon_vma is modified. Original value is lost.

> > > +
> > > +		merge_next = true;
> > >  	}
> > >
> > >  	/* If we can merge with the previous VMA, adjust vmg accordingly. */
> > > @@ -925,6 +936,16 @@ struct vm_area_struct *vma_merge_new_vma(struct vma_merge_struct *vmg)
> > >  		vmg->start = prev->vm_start;
> > >  		vmg->vma = prev;
> > >  		vmg->pgoff = prev->vm_pgoff;
> > > +
> > > +		/*
> > > +		 * If this merge would result in removal of the next VMA but we
> > > +		 * are not permitted to do so, reduce the operation to merging
> > > +		 * prev and vma.
> > > +		 */
> > > +		if (merge_next && !can_merge_remove_vma(next)) {
> > > +			vmg->end = end;
> > > +			vmg->anon_vma = anon_vma;

But here you need to restore the original value of vmg->anon_vma.

Isn't this why you introduced the local variable "anon_vma"? I believe
it would be easier to understand its purpose if it includes the "orig_"
prefix.

Just my two eurocents.

Petr T