Re: [PATCH 02/10] mm: introduce vma_merge_struct and abstract merge parameters

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

V Tue, 6 Aug 2024 15:20:49 +0100
Lorenzo Stoakes <lorenzo.stoakes@oracle.com> napsáno:

> On Tue, Aug 06, 2024 at 04:06:50PM GMT, Petr Tesařík wrote:
> > On Tue, 6 Aug 2024 14:43:48 +0100
> > Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> >  
> > > On Tue, Aug 06, 2024 at 02:47:54PM GMT, Petr Tesařík wrote:  
> > > > Hi Lorenzo!
> > > >
> > > > On Mon,  5 Aug 2024 13:13:49 +0100
> > > > Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> > > >  
> > > > > Rather than passing around huge numbers of parameters to numerous helper
> > > > > functions, abstract them into a single struct that we thread through the
> > > > > operation.
> > > > >
> > > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > > > > ---
> > > > >  mm/mmap.c |  76 ++++++++------
> > > > >  mm/vma.c  | 297 ++++++++++++++++++++++++++++++++++++++----------------
> > > > >  mm/vma.h  |  92 ++++++++---------
> > > > >  3 files changed, 294 insertions(+), 171 deletions(-)
> > > > >
> > > > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > > > index 4a9c2329b09a..f931000c561f 100644
> > > > > --- a/mm/mmap.c
> > > > > +++ b/mm/mmap.c
> > > > > @@ -1369,9 +1369,16 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > > > >  	unsigned long end = addr + len;
> > > > >  	unsigned long merge_start = addr, merge_end = end;
> > > > >  	bool writable_file_mapping = false;
> > > > > -	pgoff_t vm_pgoff;
> > > > >  	int error;
> > > > >  	VMA_ITERATOR(vmi, mm, addr);
> > > > > +	struct vma_merge_struct vmg = {
> > > > > +		.vmi = &vmi,
> > > > > +		.start = addr,
> > > > > +		.end = end,
> > > > > +		.flags = vm_flags,
> > > > > +		.pgoff = pgoff,
> > > > > +		.file = file,
> > > > > +	};
> > > > >
> > > > >  	/* Check against address space limit. */
> > > > >  	if (!may_expand_vm(mm, vm_flags, len >> PAGE_SHIFT)) {
> > > > > @@ -1405,8 +1412,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > > > >  		vm_flags |= VM_ACCOUNT;
> > > > >  	}
> > > > >
> > > > > -	next = vma_next(&vmi);
> > > > > -	prev = vma_prev(&vmi);
> > > > > +	next = vmg.next = vma_next(&vmi);
> > > > > +	prev = vmg.prev = vma_prev(&vmi);  
> > > >
> > > > So, next is now a shortcut for vmg.next, and prev is a shortcut for
> > > > vmg.prev. ATM there is only one assignment, so no big deal, but I
> > > > wonder if next and prev could be removed instead, same as you replaced
> > > > vm_pgoff with vmg.pgoff.  
> > >
> > > It's simply to avoid repeatedly referencing vmg.xxx / at least reduce
> > > _some_ churn. Also this will get moved shortly, so it's worth looking at in
> > > final form.  
> >
> > I'm not a MM maintainer, so my comments may not be relevant, but my
> > experience shows that pointer aliases have a potential to introduce all
> > kinds of subtle bugs. That's the reason I generally try to avoid them.  
> 
> Right, I understand, I don't want to get too deep into a distracting bike
> shed when this series is doing something quite major.
> 
> If you feel this is absolutely critical, I can adjust this code that I
> later delete, if not I suggest leaving it as it is.

Fair enough. I missed that _both_ occurences of the pointer aliases are
deleted later.

Then you're right, it's fine as is. No more bike shedding.

Petr T