Re: [RFC PATCH 02/16] mm/madvise: split out populate behavior check logic

[SeongJae Park]

On Wed, 5 Mar 2025 12:32:52 -0800 Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:

> On Wed, Mar 05, 2025 at 10:15:57AM -0800, SeongJae Park wrote:
> > madvise_do_behavior() has a long open-coded 'behavior' check for
> > MADV_POPULATE_{READ,WRITE}.  It adds multiple layers[1] and make the
> > code arguably take longer time to read.  Like is_memory_failure(), split
> > out the check to a separate function.  This is not technically removing
> > the additional layer but discourage further extending the switch-case.
> > Also it makes madvise_do_behavior() code shorter and therefore easier to
> > read.
> > 
> > [1] https://lore.kernel.org/bd6d0bf1-c79e-46bd-a810-9791efb9ad73@lucifer.local
> > 
> > Signed-off-by: SeongJae Park <sj@xxxxxxxxxx>
> > ---
> >  mm/madvise.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index dbc8fec05cc6..4a91590656dc 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -1633,6 +1633,17 @@ static bool is_valid_madvise(unsigned long start, size_t len_in, int behavior)
> >  	return true;
> >  }
> >  
> > +static bool is_memory_populate(int behavior)
> 
> No strong opinion on this patch but if you want to keep it, the above
> name feels weird. How about either is_madvise_populate() or
> is_populate_memory()?

I wanted to make this reads consistent with other similar purpose ones like
is_memory_failure(behavior).  I have no strong opinions, either, though.
Unless someone makes a voice here, I will rename this to is_madvise_populate()
in the next version.

> 
> > +{
> > +	switch (behavior) {
> > +	case MADV_POPULATE_READ:
> > +	case MADV_POPULATE_WRITE:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}

Thanks,
SJ

[...]