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 19:37:28 -0800 Shakeel Butt <shakeel.butt@linux.dev> wrote:

> On Wed, Mar 05, 2025 at 03:56:32PM -0800, SeongJae Park wrote:
> > On Wed, 5 Mar 2025 13:40:17 -0800 Shakeel Butt <shakeel.butt@linux.dev> wrote:
> > 
> > >  
> > > +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.
> > 
> 
> This is not a new pattern. You can find a lot of examples in kernel
> where a struct encapsulates multiple fields and its pointer is passed
> around rather than those fields (or subset of them). You can check out
> zap_details, vm_fault, fs_parameter. If some fields are mutually
> exclusive you can have union in the struct.

You're right.  But we do so for readability and/or efficiency, right?  And what
I'm saying is that I'm not very sure if this change is making code much easier
to read and/or efficient.

> Also I am not sure what do
> you mean by "not efficient" here. Inefficient in what sense? Unused
> memory or something else?

I was meaning unused memory in this context.  It is a negligible extent, of
course.  Nevertheless, what I wanted to say is not that the change will degrade
efficiency too much, but I'm not clearly seeing the efficiency benefit that
explains why the change is something that should be made.

> 
> > [...]
> > > @@ -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.
> 
> Here we can keep behavior as parameter to madvise_walk_vmas() and define
> struct madvise_walk_param inside it with the passed behavior. Anyways
> this is a very common pattern in kernel.
> 
> > 
> > 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?
> > 
> 
> Squashing patches 5 to 8 into one is the main request from me. My other
> suggestion you can ignore but let's see what other says.

Makes sense.  Thank you for your inputs :)


Thanks,
SJ