linux-kernel - Re: [PATCH v2 4/4] mm/madvise: batch tlb flushes for MADV_DONTNEED[

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20250408201629.63324-1-sj@kernel.org>
Date: Tue,  8 Apr 2025 13:16:29 -0700
From: SeongJae Park <sj@...nel.org>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: SeongJae Park <sj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Liam R.Howlett" <howlett@...il.com>,
	David Hildenbrand <david@...hat.com>,
	Rik van Riel <riel@...riel.com>,
	Shakeel Butt <shakeel.butt@...ux.dev>,
	Vlastimil Babka <vbabka@...e.cz>,
	kernel-team@...a.com,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH v2 4/4] mm/madvise: batch tlb flushes for MADV_DONTNEED[_LOCKED]

On Tue, 8 Apr 2025 14:36:18 +0100 Lorenzo Stoakes <lorenzo.stoakes@...cle.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@...nel.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

[...]