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
[...]
