linux-kernel - Re: [PATCH 5/9] mm/madvise: define and use madvise_behavior struct for madvise_do

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: <20250311205617.85289-1-sj@kernel.org>
Date: Tue, 11 Mar 2025 13:56:17 -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>,
	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 5/9] mm/madvise: define and use madvise_behavior struct for madvise_do_behavior()

On Tue, 11 Mar 2025 12:17:40 +0000 Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:

> On Mon, Mar 10, 2025 at 10:23:14AM -0700, SeongJae Park wrote:
> > To implement batched tlb flushes for MADV_DONTNEED[_LOCKED] and
> > MADV_FREE, an mmu_gather object in addition to the behavior integer need
> > to be passed to the internal logics.  Using a struct can make it easy
> > without increasing the number of parameters of all code paths towards
> > the internal logic.  Define a struct for the purpose and use it on the
> > code path that starts from madvise_do_behavior() and ends on
> > madvise_dontneed_free().
> 
> Oh a helper struct! I like these!
> 
> Nitty but...
> 
> I wonder if we should just add the the mmu_gather field immediately even if
> it isn't used yet?

I will do so in the next spin.

> 
> Also I feel like this patch and 6 should be swapped around, as you are
> laying the groundwork here for patch 7 but then doing something unrelated
> in 6, unless I'm missing something.

I actually introduced patch 6 before this one initially.  But I started
thinking this patch could help not only tlb flushes batching but potential
future madvise behavior extensions, and ended up chaging the order in current
way.  I have no strong opinion and your suggestion also sounds nice to me.  I
will swap those in the next version unless someone makes voice.

> 
> Also maybe add a bit in commit msg about changing the madvise_walk_vmas()
> visitor type signature

I will do so, in the next version.

> (I wonder if that'd be better as a typedef tbh?)

Something like below?

    typedef void *madvise_walk_arg;

I think that could make the code easier to read.  But I feel the void pointer
is also not very bad for the current simple static functions use case, so I'd
like keep this as is if you don't mind.

Please let me know if I'm missing your point.

> 
> However, this change looks fine aside from nits (and you know, helper
> struct and I'm sold obviously ;) so:
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

Thank you! :)

> 
> >
> > Signed-off-by: SeongJae Park <sj@...nel.org>
> > ---
> >  mm/madvise.c | 36 ++++++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 469c25690a0e..ba2a78795207 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -890,11 +890,16 @@ static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
> >  	return true;
> >  }
> >
> > +struct madvise_behavior {
> > +	int behavior;
> > +};
> > +
> >  static long madvise_dontneed_free(struct vm_area_struct *vma,
> >  				  struct vm_area_struct **prev,
> >  				  unsigned long start, unsigned long end,
> > -				  int behavior)
> > +				  struct madvise_behavior *madv_behavior)
> 
> Nitty, but not sure about the need for 'madv_' here. I think keeping this as
> 'behavior' is fine, as the type is very clear.

Agreed.  I wanted to reduce the name conflict causing diff lines, but I think
your suggestion is the right thing to do for long term.


Thanks,
SJ

[...]