| Message ID | 20250305181611.54484-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 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@kernel.org> > --- > 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()? > +{ > + 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) > { > @@ -1646,16 +1657,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_memory_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
On Wed, 5 Mar 2025 12:32:52 -0800 Shakeel Butt <shakeel.butt@linux.dev> 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@kernel.org> > > --- > > 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 [...]
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) +{ + 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) { @@ -1646,16 +1657,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_memory_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(-)