On Tue, Aug 06, 2024 at 04:21:49PM GMT, Petr Tesařík wrote:
> 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".
At the risk of sounding a little rude, it'd be courteous to take the time
to read through and understand the function before reviewing, especially
when doing a drive-by.
We use other fields like start, end, pgoff in a similar way to reset things
if expansion fails.
>
> >
> > 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". ;-)
I don't find these kind of sarcastic comments hugely helpful.
>
> 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.
Yes, that's intentional, and a product of how anon_vma objects
function. This may be worth a comment actually.
By this point, is_mergeable_anon_vma() would be been checked between the
VMAs, so either they'd be identical, or (due to the intricacies of
anon_vma) it'd be permitted to overwrite the new VMA's anon_vma.
This is fiddly so I'll add a comment on respin.
>
> > > > +
> > > > + 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.
I think at some point on review when a bikeshed point is simply being
repeated a review-ee just has to say 'sorry no'.
So, sorry, no.
I don't mean to be rude, but I just don't think it's productive to go in a
loop.
>
> Just my two eurocents.
>
> Petr T
