Re: [PATCH v2 4/4] mm/madvise: batch tlb flushes for MADV_DONTNEED[_LOCKED]

[SeongJae Park <sj@kernel.org>]

On Tue, 8 Apr 2025 14:36:18 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

> On Fri, Apr 04, 2025 at 02:07:00PM -0700, SeongJae Park wrote:
> > Batch tlb flushes for MADV_DONTNEED[_LOCKED] for better efficiency, in a
> > way that very similar to the tlb flushes batching for MADV_FREE.
> 
> This seems like a rather succinct commit message under the circumstances :) can
> we put some meat on the bone?
> 
> Perhaps explain why one might want to do so, propagating some of your excellent
> cover letter contents here, etc.
> 
> Also you're doing more than this, you're also exporting the (soon to be renamed,
> ideally :) notify_unmap_single_vma() function, let's mention this here please,
> and also mention why.

Good points, thank you.  I will update the commit message in the next spin
following your suggestions.

> 
> >
> > Signed-off-by: SeongJae Park <sj@kernel.org>
> > ---
> >  mm/internal.h | 3 +++
> >  mm/madvise.c  | 9 ++++++---
> >  mm/memory.c   | 4 ++--
> >  3 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/internal.h b/mm/internal.h
> > index e9695baa5922..be0c46837e22 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -435,6 +435,9 @@ void unmap_page_range(struct mmu_gather *tlb,
> >  			     struct vm_area_struct *vma,
> >  			     unsigned long addr, unsigned long end,
> >  			     struct zap_details *details);
> > +void notify_unmap_single_vma(struct mmu_gather *tlb,
> > +		struct vm_area_struct *vma, unsigned long addr,
> > +		unsigned long size, struct zap_details *details);
> 
> Yeah I know I said in 3/4 but I really hate this name. We need to change it... :)

Yes, I will change the name in the next revision.

> 
> >  int folio_unmap_invalidate(struct address_space *mapping, struct folio *folio,
> >  			   gfp_t gfp);
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 564095e381b2..c7ac32b4a371 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -851,7 +851,8 @@ static int madvise_free_single_vma(
> >   * An interface that causes the system to free clean pages and flush
> >   * dirty pages is already available as msync(MS_INVALIDATE).
> >   */
> > -static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
> > +static long madvise_dontneed_single_vma(struct madvise_behavior *behavior,
> 
> Again, let's go with madv_behavior for now please. Otherwise we have a weird
> inconsistency that sometimes behavior = the int 'behavior' value and sometimes
> it's a pointer to the helper struct.

Ye, I will do so.

> 
> > +					struct vm_area_struct *vma,
> >  					unsigned long start, unsigned long end)
> >  {
> >  	struct zap_details details = {


Thanks,
SJ

[...]