[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240222150703.u55kpmdlog5hrld3@revolver>
Date: Thu, 22 Feb 2024 10:07:03 -0500
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Yajun Deng <yajun.deng@...ux.dev>
Cc: akpm@...ux-foundation.org, vbabka@...e.cz, lstoakes@...il.com,
surenb@...gle.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/mmap: convert all mas except mas_detach to vma
iterator
* Yajun Deng <yajun.deng@...ux.dev> [240222 03:56]:
..
> > > > > > @@ -1959,11 +1958,12 @@ static int expand_upwards(struct vm_area_struct *vma, unsigned long address)
> > > > > > struct vm_area_struct *next;
> > > > > > unsigned long gap_addr;
> > > > > > int error = 0;
> > > > > > - MA_STATE(mas, &mm->mm_mt, vma->vm_start, address);
> > > > > > + VMA_ITERATOR(vmi, mm, 0);
> > > > > > if (!(vma->vm_flags & VM_GROWSUP))
> > > > > > return -EFAULT;
> > > > > > + vma_iter_config(&vmi, vma->vm_start, address);
> > > > This is confusing. I think you are doing this so that the vma iterator
> > > > is set up the same as the maple state, and not what is logically
> > > > necessary?
> > >
> > > Yes, VMA_ITERATOR can only pass one address.
> > >
> > > > > > /* Guard against exceeding limits of the address space. */
> > > > > > address &= PAGE_MASK;
> > > > > > if (address >= (TASK_SIZE & PAGE_MASK))
> > > > > > @@ -1985,15 +1985,15 @@ static int expand_upwards(struct vm_area_struct *vma, unsigned long address)
> > > > > > }
> > > > > > if (next)
> > > > > > - mas_prev_range(&mas, address);
> > > > > > + mas_prev_range(&vmi.mas, address);
> > > > This isn't really hiding the maple state.
> > >
> > > Okay, I will create a new helper function for this in the mm/internal.h.
> > >
> > > >
> > > > > > - __mas_set_range(&mas, vma->vm_start, address - 1);
> > > > > > - if (mas_preallocate(&mas, vma, GFP_KERNEL))
> > > > > > + vma_iter_config(&vmi, vma->vm_start, address);
> > > > The above maple state changes is to get the maple state to point to the
> > > > correct area for the preallocation call below. This seems unnecessary
> > > > to me.
> > > >
> > > > We really should just set it up correctly. Unfortunately, with the VMA
> > > > iterator, that's not really possible on initialization.
> > > >
> > > > What we can do is use the vma->vm_start for the initialization, then use
> > > > vma_iter_config() here. That will not reset any state - but that's fine
> > > > because the preallocation is the first call that actually uses it
> > > > anyways.
> > > >
> > > > So we can initialize with vma->vm_start, don't call vma_iter_config
> > > > until here, and also drop the if (next) part.
> > > >
> > > > This is possible here because it's not optimised like the
> > > > expand_upwards() case, which uses the state to check prev and avoids an
> > > > extra walk.
> > > >
> > > > Please make sure to test with the ltp tests on the stack combining, etc
> > > > on a platform that expands down.
>
>
> It seems something wrong about this description. This change is in
> expand_upwards(), but not in
>
> expand_downwards(). So we should test it on a platform that expands up.
Oh, yes. Test on the platform that expands upwards would be best.
Sorry about the mix up.
>And
> drop the if (next) part
>
> is unnecessary. Did I get that right?
Yes, I think the if (next) part is unnecessary because the maple
state/vma iterator has not actually moved - we use
find_vma_intersection() to locate next and not the iterator. This is
different than what we do in the expand_downwards.
Note that, in the even that we reach the limit and cannot return a
usable address, these functions will call the counterpart and search in
the opposite direction.
>
> > >
> > > Okay, I will test it.
> > Testing this can be tricky. Thanks for looking at it.
> >
..
Thanks,
Liam
Powered by blists - more mailing lists