lkml.org 
[lkml]   [2023]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH v2 06/44] mm/mmap: convert brk to use vma iterator
Date
* Vernon Yang <vernon2gm@gmail.com> [230109 10:19]:
> Hello Liam,
>
> On Thu, Jan 05, 2023 at 07:15:54PM +0000, Liam Howlett wrote:
> > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
> >
> > Use the vma iterator API for the brk() system call. This will provide
> > type safety at compile time.
> >
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > ---
> > mm/mmap.c | 47 +++++++++++++++++++++++------------------------
> > 1 file changed, 23 insertions(+), 24 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 9318f2ac8a6e..4a6f42ab3560 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c

...

> > @@ -2998,7 +2998,7 @@ static int do_brk_munmap(struct ma_state *mas, struct vm_area_struct *vma,
> > * do not match then create a new anonymous VMA. Eventually we may be able to
> > * do some brk-specific accounting here.
> > */
> > -static int do_brk_flags(struct ma_state *mas, struct vm_area_struct *vma,
> > +static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > unsigned long addr, unsigned long len, unsigned long flags)
> > {
> > struct mm_struct *mm = current->mm;
> > @@ -3025,8 +3025,7 @@ static int do_brk_flags(struct ma_state *mas, struct vm_area_struct *vma,
> > if (vma && vma->vm_end == addr && !vma_policy(vma) &&
> > can_vma_merge_after(vma, flags, NULL, NULL,
> > addr >> PAGE_SHIFT, NULL_VM_UFFD_CTX, NULL)) {
> > - mas_set_range(mas, vma->vm_start, addr + len - 1);
>
> Why was mas_set_range() removed here, but below [1] it’s left?

Set range does two things: 1. it sets the range, and 2. it resets the
maple state so that the store will occur in the correct location. This
is so that you do not use an invalid maple state for a store.

We don't need to move the maple state in this case since we are already
pointing at the vma we are merging with.

Furthermore, the API for the vma iterator below "vma_iter_prealloc()"
also ensures that the state is okay.

>
> > - if (mas_preallocate(mas, vma, GFP_KERNEL))
> > + if (vma_iter_prealloc(vmi, vma))
> > goto unacct_fail;
> >
> > vma_adjust_trans_huge(vma, vma->vm_start, addr + len, 0);
> > @@ -3036,7 +3035,7 @@ static int do_brk_flags(struct ma_state *mas, struct vm_area_struct *vma,
> > }
> > vma->vm_end = addr + len;
> > vma->vm_flags |= VM_SOFTDIRTY;
> > - mas_store_prealloc(mas, vma);
> > + vma_iter_store(vmi, vma);
> >
> > if (vma->anon_vma) {
> > anon_vma_interval_tree_post_update_vma(vma);
> > @@ -3057,8 +3056,8 @@ static int do_brk_flags(struct ma_state *mas, struct vm_area_struct *vma,
> > vma->vm_pgoff = addr >> PAGE_SHIFT;
> > vma->vm_flags = flags;
> > vma->vm_page_prot = vm_get_page_prot(flags);
> > - mas_set_range(mas, vma->vm_start, addr + len - 1);
> > - if (mas_store_gfp(mas, vma, GFP_KERNEL))
> > + mas_set_range(&vmi->mas, vma->vm_start, vma->vm_end - 1);
>
> [1]. the mas_set_range() here has been reserved.

The "vma_iter_store_gfp()" API call does not check the state and the
state could be invalid.

>
> > + if (vma_iter_store_gfp(vmi, vma, GFP_KERNEL))
> > goto mas_store_fail;
> >
> > mm->map_count++;
...

This situation happened because I added the sanity check to the state
later in the development cycle.

I will fix this inconsistency and remove the mas_set_range(). Thanks
for catching this.

\
 
 \ /
  Last update: 2023-03-26 23:33    [W:0.097 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site