Re: [RFC PATCH 05/16] mm/madvise: define and use madvise_behavior struct for madvise_do_behavior()

[SeongJae Park <sj@kernel.org>]

On Wed, 5 Mar 2025 13:40:17 -0800 Shakeel Butt <shakeel.butt@linux.dev> wrote:

> On Wed, Mar 05, 2025 at 01:02:15PM -0800, Shakeel Butt wrote:
> > On Wed, Mar 05, 2025 at 10:16:00AM -0800, 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.  Define a struct for passing such
> > > information together with the behavior value without increasing number
> > > of parameters of all code paths towards the internal logic.
> > > 
> > > Signed-off-by: SeongJae Park <sj@kernel.org>
> > > ---
> > >  mm/madvise.c | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index c5e1a4d1df72..3346e593e07d 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -1665,9 +1665,15 @@ static bool is_memory_populate(int behavior)
> > >  	}
> > >  }
> > >  
> > > +struct madvise_behavior {
> > > +	int behavior;
> > > +};
> > 
> > I think the patch 5 to 8 are just causing churn and will be much better
> > to be a single patch.

I agree.  I will do so in the next version.

> > Also why not make the above struct a general
> > madvise visit param struct which can be used by both
> > madvise_vma_anon_name() and madvise_vma_behavior()?

I was also thinking we can further extend the struct for more clean and
efficient code, so agree to your fundamental point.  I ended up desiring to
focus on tlb flushes batching at the moment, though.

However, what you are suggesting is bit different from what I was thinking.  To
me, madvise_walk_vmas() is for general purposes and hence the visit functions
should be able to recieve an argument of arbitrary types.  It is true that
there are only two visit functions, but they seems doing very different purpose
works to me, so having a same type argument doesn't seem very straightforward
to understand its usage, nor efficient to my humble viewpoint.

> 
> Something like following:
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index c5e1a4d1df72..cbc3817366a6 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -890,11 +890,17 @@ static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
>  	return true;
>  }
>  
> +struct madvise_walk_param {
> +	int behavior;
> +	struct anon_vma_name *anon_name;
> +};

Only madvise_vma_behavior() will use 'behavior' field.  And only
madvise_vma_anon_name() will use 'anon_name' field.  But I will have to assume
both function _might_ use both fields when reading the madvise_walk_vmas()
function code.  That doesn't make my humble code reading that simple and
straightforward.

Also populating and passing a data structure that has data that would not
really be used seems not very efficient to me.

[...]
> @@ -1666,8 +1674,10 @@ static bool is_memory_populate(int behavior)
>  }
>  
>  static int madvise_do_behavior(struct mm_struct *mm,
> -		unsigned long start, size_t len_in, int behavior)
> +		unsigned long start, size_t len_in,
> +		struct madvise_walk_param *arg)
>  {
> +	int behavior = arg->behavior;
>  	struct blk_plug plug;
>  	unsigned long end;
>  	int error;
> @@ -1681,7 +1691,7 @@ static int madvise_do_behavior(struct mm_struct *mm,
>  	if (is_memory_populate(behavior))
>  		error = madvise_populate(mm, start, end, behavior);

'arg' is for madvise_walk_vmas() visit function, but we're using it as a
capsule for passing an information that will be used for madvise_do_behavior().
This also seems not very straightforward to my humble perspective.

I have no strong opinion and maybe my humble taste is too peculiar.  But, if
this is not a blocker for tlb flushes batcing, I'd like to suggest keep this
part as is for now, and revisit for more code cleanup later.  What do you
think, Shakeel?


Thanks,
SJ