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