Re: [PATCH] linux-user: Passthrough MADV_DONTNEED for certain file mappings

[Laurent Vivier]

Le 01/07/2022 à 15:52, Ilya Leoshkevich a écrit :
> This is a follow-up for commit 892a4f6a750a ("linux-user: Add partial
> support for MADV_DONTNEED"), which added passthrough for anonymous
> mappings. File mappings can be handled in a similar manner.
>
> In order to do that, mark pages, for which mmap() was passed through,
> with PAGE_PASSTHROUGH, and then allow madvise() passthrough for these
> pages as well.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   include/exec/cpu-all.h |  6 ++++++
>   linux-user/mmap.c      | 25 +++++++++++++++++++++----
>   2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index f5bda2c3ca..fbdbc0fdec 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -262,6 +262,12 @@ extern const TargetPageBits target_page;
>   #define PAGE_TARGET_1  0x0200
>   #define PAGE_TARGET_2  0x0400
>   
> +/*
> + * For linux-user, indicates that the page is mapped with the same semantics
> + * in both guest and host.
> + */
> +#define PAGE_PASSTHROUGH 0x0080

0x0080 is already PAGE_ANON, do you mean 0x0800?

> +
>   #if defined(CONFIG_USER_ONLY)
>   void page_dump(FILE *f);
>   
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 4e7a6be6ee..58622a0c15 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -424,7 +424,8 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, 
> abi_ulong align)
>   abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>                        int flags, int fd, abi_ulong offset)
>   {
> -    abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len;
> +    abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len,
> +              passthrough_start = -1, passthrough_end = -1;
>       int page_flags, host_prot;
>   
>       mmap_lock();
> @@ -537,6 +538,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int 
> target_prot,
>               host_start += offset - host_offset;
>           }
>           start = h2g(host_start);
> +        passthrough_start = start;
> +        passthrough_end = start + len;
>       } else {
>           if (start & ~TARGET_PAGE_MASK) {
>               errno = EINVAL;
> @@ -619,6 +622,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int 
> target_prot,
>                        host_prot, flags, fd, offset1);
>               if (p == MAP_FAILED)
>                   goto fail;
> +            passthrough_start = real_start;
> +            passthrough_end = real_end;
>           }
>       }
>    the_end1:
> @@ -626,7 +631,18 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int 
> target_prot,
>           page_flags |= PAGE_ANON;
>       }
>       page_flags |= PAGE_RESET;
> -    page_set_flags(start, start + len, page_flags);
> +    if (passthrough_start == passthrough_end) {
> +        page_set_flags(start, start + len, page_flags);
> +    } else {
> +        if (start != passthrough_start) {

it would be consistent to use "check start < passthrough_start"

> +            page_set_flags(start, passthrough_start, page_flags);
> +        }
> +        page_set_flags(passthrough_start, passthrough_end,
> +                       page_flags | PAGE_PASSTHROUGH);
> +        if (passthrough_end != start + len) {

passthrough_end < start + len

> +            page_set_flags(passthrough_end, start + len, page_flags);
> +        }
> +    }
>    the_end:
>       trace_target_mmap_complete(start);
>       if (qemu_loglevel_mask(CPU_LOG_PAGE)) {
> @@ -845,7 +861,7 @@ static bool can_passthrough_madv_dontneed(abi_ulong start, 
> abi_ulong end)
>       }
>   
>       for (addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
> -        if (!(page_get_flags(addr) & PAGE_ANON)) {
> +        if (!(page_get_flags(addr) & (PAGE_ANON | PAGE_PASSTHROUGH))) {
>               return false;
>           }
>       }
> @@ -888,7 +904,8 @@ abi_long target_madvise(abi_ulong start, abi_ulong len_in, 
> int advice)
>        *
>        * 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.
> +     * anonymous mappings or mappings that are known to have the same semantics
> +     * in the host and the guest. In this case passthrough is safe, so do it.
>        */
>       mmap_lock();
>       if ((advice & MADV_DONTNEED) &&

Thanks,
Laurent