| Message ID | 20250310172318.653630-3-sj@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/madvise: batch tlb flushes for MADV_DONTNEED and MADV_FREE | expand |
On Mon, Mar 10, 2025 at 10:23:11AM -0700, 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@kernel.org> LGTM, so: Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > mm/madvise.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/mm/madvise.c b/mm/madvise.c > index c3ab1f283b18..611db868ae38 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -1640,6 +1640,17 @@ static bool is_valid_madvise(unsigned long start, size_t len_in, int behavior) > return true; > } > > +static bool is_madvise_populate(int behavior) > +{ > + switch (behavior) { > + case MADV_POPULATE_READ: > + case MADV_POPULATE_WRITE: > + return true; > + default: > + return false; > + } > +} > + > static int madvise_do_behavior(struct mm_struct *mm, > unsigned long start, size_t len_in, size_t len, int behavior) > { > @@ -1653,16 +1664,11 @@ static int madvise_do_behavior(struct mm_struct *mm, > end = start + len; > > blk_start_plug(&plug); > - switch (behavior) { > - case MADV_POPULATE_READ: > - case MADV_POPULATE_WRITE: > + if (is_madvise_populate(behavior)) > error = madvise_populate(mm, start, end, behavior); > - break; > - default: > + else > error = madvise_walk_vmas(mm, start, end, behavior, > madvise_vma_behavior); > - break; > - } > blk_finish_plug(&plug); > return error; > } > -- > 2.39.5
diff --git a/mm/madvise.c b/mm/madvise.c index c3ab1f283b18..611db868ae38 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -1640,6 +1640,17 @@ static bool is_valid_madvise(unsigned long start, size_t len_in, int behavior) return true; } +static bool is_madvise_populate(int behavior) +{ + switch (behavior) { + case MADV_POPULATE_READ: + case MADV_POPULATE_WRITE: + return true; + default: + return false; + } +} + static int madvise_do_behavior(struct mm_struct *mm, unsigned long start, size_t len_in, size_t len, int behavior) { @@ -1653,16 +1664,11 @@ static int madvise_do_behavior(struct mm_struct *mm, end = start + len; blk_start_plug(&plug); - switch (behavior) { - case MADV_POPULATE_READ: - case MADV_POPULATE_WRITE: + if (is_madvise_populate(behavior)) error = madvise_populate(mm, start, end, behavior); - break; - default: + else error = madvise_walk_vmas(mm, start, end, behavior, madvise_vma_behavior); - break; - } blk_finish_plug(&plug); return error; }
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@kernel.org> --- mm/madvise.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)