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@xxxxxxxxxx>
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@xxxxxxxxxx>
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
