On Tue, 6 Aug 2024 15:08:33 +0100
Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> 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@xxxxxxxxxx> 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
