Re: [PATCH] linux-user: Add partial support for MADV_DONTNEED

[Laurent Vivier <laurent@vivier.eu>]

Le 21/06/2022 à 00:49, Ilya Leoshkevich a écrit :
> Currently QEMU ignores madvise(MADV_DONTNEED), which break apps that
> rely on this for zeroing out memory [1]. Improve the situation by doing
> a passthrough when the range in question is a host-page-aligned
> anonymous mapping.
> 
> This is based on the patches from Simon Hausmann [2] and Chris Fallin
> [3]. The structure is taken from Simon's patch. The PAGE_MAP_ANONYMOUS
> bits are superseded by commit 26bab757d41b ("linux-user: Introduce
> PAGE_ANON"). In the end the patch acts like the one from Chris: we
> either pass-through the entire syscall, or do nothing, since doing this
> only partially would not help the affected applications much. Finally,
> add some extra checks to match the behavior of the Linux kernel [4].
> 
> [1] https://gitlab.com/qemu-project/qemu/-/issues/326
> [2] https://patchew.org/QEMU/20180827084037.25316-1-simon.hausmann@qt.io/
> [3] https://github.com/bytecodealliance/wasmtime/blob/v0.37.0/ci/qemu-madvise.patch
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/madvise.c?h=v5.19-rc3#n1368
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/mmap.c      | 68 ++++++++++++++++++++++++++++++++++++++++++
>   linux-user/syscall.c   |  6 +---
>   linux-user/user-mmap.h |  1 +
>   3 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 48e1373796..900df7b28c 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -835,3 +835,71 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
>       mmap_unlock();
>       return new_addr;
>   }
> +
> +static bool can_passthrough_madv_dontneed(abi_ulong start, abi_ulong end)
> +{
> +    ulong addr;
> +
> +    if ((start | end) & ~qemu_host_page_mask) {
> +        return false;
> +    }
> +
> +    for (addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
> +        if (!(page_get_flags(addr) & PAGE_ANON)) {
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
> +int target_madvise(abi_ulong start, abi_ulong len_in, int advice)
> +{
> +    abi_ulong len, end;
> +    int ret = 0;
> +
> +    if (start & ~TARGET_PAGE_MASK) {
> +        errno = EINVAL;
> +        return -1;

You should remove the "errno = EINVAL" and return -TARGET_EINVAL.

> +    }
> +    len = TARGET_PAGE_ALIGN(len_in);
> +
> +    if (len_in && !len) {
> +        errno = EINVAL;
> +        return -1;

return -TARGET_EINVAL

> +    }
> +
> +    end = start + len;
> +    if (end < start) {
> +        errno = EINVAL;
> +        return -1;

return -TARGET_EINVAL

> +    }
> +
> +    if (end == start) {
> +        return 0;
> +    }
> +
> +    if (!guest_range_valid_untagged(start, len)) {
> +        errno = EINVAL;
> +        return -1;

return -TARGET_EINVAL

> +    }
> +
> +    /*
> +     * A straight passthrough may not be safe because qemu sometimes turns
> +     * private file-backed mappings into anonymous mappings.
> +     *
> +     * This is a hint, so ignoring and returning success is ok.
> +     *
> +     * This breaks MADV_DONTNEED, completely implementing which is quite
> +     * complicated. However, there is one low-hanging fruit: host-page-aligned
> +     * anonymous mappings. In this case passthrough is safe, so do it.
> +     */
> +    mmap_lock();
> +    if ((advice & MADV_DONTNEED) &&
> +        can_passthrough_madv_dontneed(start, end)) {
> +        ret = madvise(g2h_untagged(start), len, MADV_DONTNEED);

ret = get_errno(madvise(g2h_untagged(start), len, MADV_DONTNEED));

> +    }
> +    mmap_unlock();
> +
> +    return ret;
> +}
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f55cdebee5..d25759b992 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11807,11 +11807,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>   
>   #ifdef TARGET_NR_madvise
>       case TARGET_NR_madvise:
> -        /* A straight passthrough may not be safe because qemu sometimes
> -           turns private file-backed mappings into anonymous mappings.
> -           This will break MADV_DONTNEED.
> -           This is a hint, so ignoring and returning success is ok.  */
> -        return 0;
> +        return get_errno(target_madvise(arg1, arg2, arg3));

return target_madvise(arg1, arg2, arg3);

>   #endif
>   #ifdef TARGET_NR_fcntl64
>       case TARGET_NR_fcntl64:
> diff --git a/linux-user/user-mmap.h b/linux-user/user-mmap.h
> index d1dec99c02..41cd358c7a 100644
> --- a/linux-user/user-mmap.h
> +++ b/linux-user/user-mmap.h
> @@ -25,6 +25,7 @@ int target_munmap(abi_ulong start, abi_ulong len);
>   abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
>                          abi_ulong new_size, unsigned long flags,
>                          abi_ulong new_addr);
> +int target_madvise(abi_ulong start, abi_ulong len_in, int advice);
>   extern unsigned long last_brk;
>   extern abi_ulong mmap_next_start;
>   abi_ulong mmap_find_vma(abi_ulong, abi_ulong, abi_ulong);

Except comments above, it looks good.

Reviewed-by: Laurent Vivier <laurent@vivier.eu>