[Qemu-devel] Re: [PATCH v7] Introduce qemu_madvise()

[Blue Swirl]

On Sun, Sep 19, 2010 at 10:11 AM, Andreas Färber <address@hidden> wrote:
> From: Andreas Färber <address@hidden>
>
> vl.c has a Sun-specific hack to supply a prototype for madvise(),
> but the call site has apparently moved to arch_init.c.
>
> Haiku doesn't implement madvise() in favor of posix_madvise().
> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>
> Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
> as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
> Convert all callers to use qemu_madvise() and QEMU_MADV_*.
>
> Note that on Solaris the warning is fixed by moving the madvise() prototype,
> not by qemu_madvise() itself. It helps with porting though, and it simplifies
> most call sites.
>
> v6 -> v7:
> * Adopt madvise() rather than posix_madvise() semantics for returning errors.
> * Use EINVAL in place of ENOTSUP.
>
> v5 -> v6:
> * Replace two leftover instances of POSIX_MADV_NORMAL with QEMU_MADV_INVALID.
>  Spotted by Blue Swirl.
>
> v4 -> v5:
> * Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
>  Note that this relies on -1 not being a valid advice value.
>
> v3 -> v4:
> * Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
>  This will currently break the check in kvm-all.c by calling madvise() with
>  a supported flag, which will not fail. Ideas/patches welcome.
>
> v2 -> v3:
> * Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
> * Add configure check for madvise(), too.
>  Add defines to Makefile, not QEMU_CFLAGS.
>  Convert all callers, untested. Suggested by Blue Swirl.
> * Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
> * Display configure check results.
>
> v1 -> v2:
> * Don't rely on posix_madvise() availability, add qemu_madvise().
>  Suggested by Blue Swirl.
>
> Signed-off-by: Andreas Färber <address@hidden>
> Cc: Blue Swirl <address@hidden>
> Cc: Alexander Graf <address@hidden>
> Cc: Andrea Arcangeli <address@hidden>
> ---
>  arch_init.c         |    2 +-
>  configure           |   33 +++++++++++++++++++++++++++++++++
>  exec.c              |    8 ++------
>  hw/virtio-balloon.c |    4 ++--
>  kvm-all.c           |   12 ++++--------
>  osdep.c             |   20 ++++++++++++++++++++
>  osdep.h             |   35 +++++++++++++++++++++++++++++++++++
>  vl.c                |    3 ---
>  8 files changed, 97 insertions(+), 20 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index e468c0c..a910033 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -396,7 +396,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
>  #ifndef _WIN32
>             if (ch == 0 &&
>                 (!kvm_enabled() || kvm_has_sync_mmu())) {
> -                madvise(host, TARGET_PAGE_SIZE, MADV_DONTNEED);
> +                qemu_madvise(host, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
>             }
>  #endif
>         } else if (flags & RAM_SAVE_FLAG_PAGE) {
> diff --git a/configure b/configure
> index 3bfc5e9..6a21bf2 100755
> --- a/configure
> +++ b/configure
> @@ -2072,6 +2072,31 @@ if compile_prog "" "" ; then
>  fi
>
>  ##########################################
> +# check if we have madvise
> +
> +madvise=no
> +cat > $TMPC << EOF
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    madvise=yes
> +fi
> +
> +##########################################
> +# check if we have posix_madvise
> +
> +posix_madvise=no
> +cat > $TMPC << EOF
> +#include <sys/mman.h>
> +int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    posix_madvise=yes
> +fi
> +
> +##########################################
>  # check if trace backend exists
>
>  sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 
> 2> /dev/null
> @@ -2238,6 +2263,8 @@ echo "KVM support       $kvm"
>  echo "fdt support       $fdt"
>  echo "preadv support    $preadv"
>  echo "fdatasync         $fdatasync"
> +echo "madvise           $madvise"
> +echo "posix_madvise     $posix_madvise"
>  echo "uuid support      $uuid"
>  echo "vhost-net support $vhost_net"
>  echo "Trace backend     $trace_backend"
> @@ -2478,6 +2505,12 @@ fi
>  if test "$fdatasync" = "yes" ; then
>   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
>  fi
> +if test "$madvise" = "yes" ; then
> +  echo "CONFIG_MADVISE=y" >> $config_host_mak
> +fi
> +if test "$posix_madvise" = "yes" ; then
> +  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
> +fi
>
>  # XXX: suppress that
>  if [ "$bsd" = "yes" ] ; then
> diff --git a/exec.c b/exec.c
> index 380dab5..9b5464f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2841,9 +2841,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, 
> const char *name,
>             new_block->host = file_ram_alloc(new_block, size, mem_path);
>             if (!new_block->host) {
>                 new_block->host = qemu_vmalloc(size);
> -#ifdef MADV_MERGEABLE
> -                madvise(new_block->host, size, MADV_MERGEABLE);
> -#endif
> +                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>             }
>  #else
>             fprintf(stderr, "-mem-path option unsupported\n");
> @@ -2858,9 +2856,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, 
> const char *name,
>  #else
>             new_block->host = qemu_vmalloc(size);
>  #endif
> -#ifdef MADV_MERGEABLE
> -            madvise(new_block->host, size, MADV_MERGEABLE);
> -#endif
> +            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>         }
>     }
>
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 9fe3886..1e74674 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
>  {
>  #if defined(__linux__)
>     if (!kvm_enabled() || kvm_has_sync_mmu())
> -        madvise(addr, TARGET_PAGE_SIZE,
> -                deflate ? MADV_WILLNEED : MADV_DONTNEED);
> +        qemu_madvise(addr, TARGET_PAGE_SIZE,
> +                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
>  #endif
>  }
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 58b0404..1cc696f 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1031,18 +1031,14 @@ int kvm_has_xcrs(void)
>  void kvm_setup_guest_memory(void *start, size_t size)
>  {
>     if (!kvm_has_sync_mmu()) {
> -#ifdef MADV_DONTFORK
> -        int ret = madvise(start, size, MADV_DONTFORK);
> +        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
>
>         if (ret) {
> -            perror("madvice");
> +            perror("qemu_madvise");
> +            fprintf(stderr,
> +                    "Need MADV_DONTFORK in absence of synchronous KVM 
> MMU\n");
>             exit(1);
>         }
> -#else
> -        fprintf(stderr,
> -                "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
> -        exit(1);
> -#endif
>     }
>  }
>
> diff --git a/osdep.c b/osdep.c
> index 30426ff..56c6944 100644
> --- a/osdep.c
> +++ b/osdep.c
> @@ -28,6 +28,7 @@
>  #include <errno.h>
>  #include <unistd.h>
>  #include <fcntl.h>
> +#include <sys/mman.h>

With the patch applied, I get a warning here with mingw from Debian stable:
  CC    osdep.o
/src/qemu/osdep.c:31:22: error: sys/mman.h: No such file or directory

For some reason, it doesn't happen with newer mingw from Debian testing.