| Message ID | 20250310172318.653630-4-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:12AM -0700, SeongJae Park wrote: > The logic for checking if a given madvise() request for a single memory > range can skip real work, namely madvise_do_behavior(), is duplicated in > do_madvise() and vector_madvise(). Split out the logic to a function > and resue it. NIT: typo :) > > Signed-off-by: SeongJae Park <sj@kernel.org> madvise_set_anon_name() seems to do something similar, but somewhat differently... not sure if you address this in a later commit but worth looking at too! Anyway this seems sane, so: Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Note nits! > --- > mm/madvise.c | 53 +++++++++++++++++++++++++++++----------------------- > 1 file changed, 30 insertions(+), 23 deletions(-) > > diff --git a/mm/madvise.c b/mm/madvise.c > index 611db868ae38..764ec1f2475b 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -1640,6 +1640,27 @@ static bool is_valid_madvise(unsigned long start, size_t len_in, int behavior) > return true; > } > > +/* > + * madvise_should_skip() - Return if an madivse request can skip real works. NIT: 'real works' sounds strange. I'd say something like 'if the specified behaviour is invalid or nothing would occur, we skip this operation. In the former case we return an error.' > + * @start: Start address of madvise-requested address range. > + * @len_in: Length of madvise-requested address range. > + * @behavior: Requested madvise behavor. > + * @err: Pointer to store an error code from the check. > + */ > +static bool madvise_should_skip(unsigned long start, size_t len_in, > + int behavior, int *err) > +{ > + if (!is_valid_madvise(start, len_in, behavior)) { > + *err = -EINVAL; > + return true; > + } > + if (start + PAGE_ALIGN(len_in) == start) { > + *err = 0; > + return true; > + } > + return false; > +} > + > static bool is_madvise_populate(int behavior) > { > switch (behavior) { > @@ -1747,23 +1768,15 @@ static int madvise_do_behavior(struct mm_struct *mm, > */ > int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior) > { > - unsigned long end; > int error; > - size_t len; > - > - if (!is_valid_madvise(start, len_in, behavior)) > - return -EINVAL; > - > - len = PAGE_ALIGN(len_in); > - end = start + len; > - > - if (end == start) > - return 0; > > + if (madvise_should_skip(start, len_in, behavior, &error)) > + return error; > error = madvise_lock(mm, behavior); > if (error) > return error; > - error = madvise_do_behavior(mm, start, len_in, len, behavior); > + error = madvise_do_behavior(mm, start, len_in, PAGE_ALIGN(len_in), > + behavior); > madvise_unlock(mm, behavior); > > return error; > @@ -1790,19 +1803,13 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter, > while (iov_iter_count(iter)) { > unsigned long start = (unsigned long)iter_iov_addr(iter); > size_t len_in = iter_iov_len(iter); > - size_t len; > - > - if (!is_valid_madvise(start, len_in, behavior)) { > - ret = -EINVAL; > - break; > - } > + int error; > > - len = PAGE_ALIGN(len_in); > - if (start + len == start) > - ret = 0; > + if (madvise_should_skip(start, len_in, behavior, &error)) > + ret = error; > else > - ret = madvise_do_behavior(mm, start, len_in, len, > - behavior); > + ret = madvise_do_behavior(mm, start, len_in, > + PAGE_ALIGN(len_in), behavior); > /* > * An madvise operation is attempting to restart the syscall, > * but we cannot proceed as it would not be correct to repeat > -- > 2.39.5
diff --git a/mm/madvise.c b/mm/madvise.c index 611db868ae38..764ec1f2475b 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -1640,6 +1640,27 @@ static bool is_valid_madvise(unsigned long start, size_t len_in, int behavior) return true; } +/* + * madvise_should_skip() - Return if an madivse request can skip real works. + * @start: Start address of madvise-requested address range. + * @len_in: Length of madvise-requested address range. + * @behavior: Requested madvise behavor. + * @err: Pointer to store an error code from the check. + */ +static bool madvise_should_skip(unsigned long start, size_t len_in, + int behavior, int *err) +{ + if (!is_valid_madvise(start, len_in, behavior)) { + *err = -EINVAL; + return true; + } + if (start + PAGE_ALIGN(len_in) == start) { + *err = 0; + return true; + } + return false; +} + static bool is_madvise_populate(int behavior) { switch (behavior) { @@ -1747,23 +1768,15 @@ static int madvise_do_behavior(struct mm_struct *mm, */ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior) { - unsigned long end; int error; - size_t len; - - if (!is_valid_madvise(start, len_in, behavior)) - return -EINVAL; - - len = PAGE_ALIGN(len_in); - end = start + len; - - if (end == start) - return 0; + if (madvise_should_skip(start, len_in, behavior, &error)) + return error; error = madvise_lock(mm, behavior); if (error) return error; - error = madvise_do_behavior(mm, start, len_in, len, behavior); + error = madvise_do_behavior(mm, start, len_in, PAGE_ALIGN(len_in), + behavior); madvise_unlock(mm, behavior); return error; @@ -1790,19 +1803,13 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter, while (iov_iter_count(iter)) { unsigned long start = (unsigned long)iter_iov_addr(iter); size_t len_in = iter_iov_len(iter); - size_t len; - - if (!is_valid_madvise(start, len_in, behavior)) { - ret = -EINVAL; - break; - } + int error; - len = PAGE_ALIGN(len_in); - if (start + len == start) - ret = 0; + if (madvise_should_skip(start, len_in, behavior, &error)) + ret = error; else - ret = madvise_do_behavior(mm, start, len_in, len, - behavior); + ret = madvise_do_behavior(mm, start, len_in, + PAGE_ALIGN(len_in), behavior); /* * An madvise operation is attempting to restart the syscall, * but we cannot proceed as it would not be correct to repeat
The logic for checking if a given madvise() request for a single memory range can skip real work, namely madvise_do_behavior(), is duplicated in do_madvise() and vector_madvise(). Split out the logic to a function and resue it. Signed-off-by: SeongJae Park <sj@kernel.org> --- mm/madvise.c | 53 +++++++++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 23 deletions(-)