Re: [PATCH 6/9] mm/memory: split non-tlb flushing part from zap_page_range_single()

[SeongJae Park <sj@kernel.org>]

On Mon, 31 Mar 2025 21:45:40 -0400 "Liam R. Howlett" <Liam.Howlett@oracle.com> wrote:

> * SeongJae Park <sj@kernel.org> [250310 13:24]:
> > Some of zap_page_range_single() callers such as [process_]madvise() with
> > MADV_DONEED[_LOCKED] cannot batch tlb flushes because
> > zap_page_range_single() does tlb flushing for each invocation.  Split
> > out the body of zap_page_range_single() except mmu_gather object
> > initialization and gathered tlb entries flushing parts for such batched
> > tlb flushing usage.
> > 
> > Signed-off-by: SeongJae Park <sj@kernel.org>
> > ---
> >  mm/memory.c | 36 ++++++++++++++++++++++--------------
> >  1 file changed, 22 insertions(+), 14 deletions(-)
> > 
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 78c7ee62795e..88c478e2ed1a 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1995,38 +1995,46 @@ void unmap_vmas(struct mmu_gather *tlb, struct ma_state *mas,
> >  	mmu_notifier_invalidate_range_end(&range);
> >  }
> >  
> > -/**
> > - * zap_page_range_single - remove user pages in a given range
> > - * @vma: vm_area_struct holding the applicable pages
> > - * @address: starting address of pages to zap
> > - * @size: number of bytes to zap
> > - * @details: details of shared cache invalidation
> > - *
> > - * The range must fit into one VMA.
> > - */
> > -void zap_page_range_single(struct vm_area_struct *vma, unsigned long address,
> > +static void unmap_vma_single(struct mmu_gather *tlb,
> 
> I could not, for the life of me, figure out what was going on here until
> I realised that is is a new function name and not unmap_single_vma(),
> which is called below.

Agreed, definitely the name is confusing, especially given the existence of
unmap_single_vma().

> 
> Can we name this differently somehow?  notify_unmap_single_vma() or
> something better?

notify_unmap_single_vma() sounds good to me.  I'll use the name in the next
revision unless we find a better one.

> 
> Also, maybe add a description of the function to this patch vs the next
> patch?

That makes sense.  In the next revision, I will add the kernel-doc comment
here, but not as a valid kernel-doc comment (maybe wtarts with /* instead of
/**) since this function is a static function as of this patch.  On the next
patch that makes this non-static, I will make the comment a valid kernel-doc
comment with a minimum change.

I prefer not having a valid kernel-doc comment for static function, but that's
just a personal preferrence and I have no strong reason to object other way.
Please feel free to let me know if you prefer making it valid kernel doc
comment starting from this patch.

Thank you for nice suggestions, Liam.


Thanks,
SJ

[...]