Re: [PATCH 5/9] mm/madvise: define and use madvise_behavior struct for madvise_do_behavior()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Mar 11, 2025 at 01:56:17PM -0700, SeongJae Park wrote:
> On Tue, 11 Mar 2025 12:17:40 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.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.

No to be clear I meant the:

int (*visit)(struct vm_area_struct *vma,
				   struct vm_area_struct **prev, unsigned long start,
				   unsigned long end, unsigned long arg)

Function pointer.

But this is not a big deal and let's leave it as-is for now, we can address
this later potentially! :)

>
> >
> > 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@oracle.com>
>
> Thank you! :)
>
> >
> > >
> > > Signed-off-by: SeongJae Park <sj@kernel.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
>
> [...]

Thanks for being so flexible on the feedback! Appreciated :>)