Re: [PATCH v10 14/69] kernel/fork: use maple tree for dup_mmap() during forking

[Liam Howlett]

* David Hildenbrand <david@xxxxxxxxxx> [220623 14:42]:
> On 21.06.22 22:46, Liam Howlett wrote:
> > From: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
> > 
> > The maple tree was already tracking VMAs in this function by an earlier
> > commit, but the rbtree iterator was being used to iterate the list.
> > Change the iterator to use a maple tree native iterator and switch to the
> > maple tree advanced API to avoid multiple walks of the tree during insert
> > operations.  Unexport the now-unused vma_store() function.
> > 
> > For performance reasons we bulk allocate the maple tree nodes.  The node
> > calculations are done internally to the tree and use the VMA count and
> > assume the worst-case node requirements.  The VM_DONT_COPY flag does not
> > allow for the most efficient copy method of the tree and so a bulk loading
> > algorithm is used.
> 
> You could track the #VM_DONT_COPY per mm and optimize? Do you have that
> planned or maybe it already is part of this patch set? :)
> 
> > 
> > Link: https://lkml.kernel.org/r/20220504010716.661115-16-Liam.Howlett@xxxxxxxxxx
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> > Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Cc: David Howells <dhowells@xxxxxxxxxx>
> > Cc: SeongJae Park <sj@xxxxxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >  include/linux/mm.h |  2 --
> >  kernel/fork.c      | 12 +++++++++---
> >  2 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index f22c6f71a18c..27fa36a7c093 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2612,8 +2612,6 @@ extern bool arch_has_descending_max_zone_pfns(void);
> >  /* nommu.c */
> >  extern atomic_long_t mmap_pages_allocated;
> >  extern int nommu_shrink_inode_mappings(struct inode *, size_t, size_t);
> > -/* mmap.c */
> > -void vma_mas_store(struct vm_area_struct *vma, struct ma_state *mas);
> >  
> >  /* interval_tree.c */
> >  void vma_interval_tree_insert(struct vm_area_struct *node,
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 1840da0732f6..4ecca79556eb 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -583,9 +583,10 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> >  	struct vm_area_struct *mpnt, *tmp, *prev, **pprev;
> >  	struct rb_node **rb_link, *rb_parent;
> >  	int retval;
> > -	unsigned long charge;
> > -	LIST_HEAD(uf);
> > +	unsigned long charge = 0;
> 
> ^ why that change?

mas_for_each() may not be executed - according to the compiler.  We know
it will be but this makes the compiler happy.

> 
> > +	MA_STATE(old_mas, &oldmm->mm_mt, 0, 0);
> >  	MA_STATE(mas, &mm->mm_mt, 0, 0);
> > +	LIST_HEAD(uf);
> 
> ^ why that reshuffling?

I'll revert this.

> 
> >  
> >  	uprobe_start_dup_mmap();
> >  	if (mmap_write_lock_killable(oldmm)) {
> > @@ -620,7 +621,12 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> >  		goto out;
> >  
> >  	prev = NULL;
> > -	for (mpnt = oldmm->mmap; mpnt; mpnt = mpnt->vm_next) {
> > +
> > +	retval = mas_expected_entries(&mas, oldmm->map_count);
> > +	if (retval)
> > +		goto out;
> 
> mas_expected_entries is a fancy function :)
> 
> 
> > +
> > +	mas_for_each(&old_mas, mpnt, ULONG_MAX) {
> >  		struct file *file;
> >  
> >  		if (mpnt->vm_flags & VM_DONTCOPY) {
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb
>