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

[Blue Swirl]

On Sat, Sep 11, 2010 at 5:05 PM, 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.
> The underlying issue is that madvise() is not a POSIX function,
> therefore Solaris' _POSIX_C_SOURCE suppresses the prototype.
>
> Haiku doesn't implement madvise() at all.
> OpenBSD however doesn't implement posix_madvise().
>
> Check for posix_madvise() in configure and supply qemu_madvise() as wrapper.
> Use qemu_madvise() in arch_init.c's ram_load().
>
> http://www.opengroup.org/onlinepubs/9699919799/functions/posix_madvise.html
>
> Remaining madvise() users:
> exec.c: limited to __linux__ and/or MADV_MERGEABLE (no POSIX equivalent)
> kvm-all.c: limited to MADV_DONTFORK (no POSIX equivalent),
>           otherwise runtime error if !kvm_has_sync_mmu()
> hw/virtio-balloon.c: limited to __linux__

For consistency, I'd convert all users. If an OS doesn't support a
flag, we can return something like -ENOTSUP which may be checked by
the caller if it cares.

>
> 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>
> ---
>  arch_init.c |    2 +-
>  configure   |   11 +++++++++++
>  osdep.c     |   26 ++++++++++++++++++++++++++
>  osdep.h     |    4 ++++
>  vl.c        |    3 ---
>  5 files changed, 42 insertions(+), 4 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 4061cb7..5e6f3e1 100755
> --- a/configure
> +++ b/configure
> @@ -2069,6 +2069,17 @@ if compile_prog "" "" ; then
>  fi
>
>  ##########################################
> +# check if we have posix_madvise
> +
> +cat > $TMPC << EOF
> +#include <sys/mman.h>
> +int main(void) { posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); return 0; }
> +EOF
> +if compile_prog "" "" ; then
> +    QEMU_CFLAGS="-DCONFIG_POSIX_MADVISE ${QEMU_CFLAGS}"

Please take a look around configure:2444 how to add new config flags.
I'd also add a similar check for madvise() if posix_madvise() check
fails.

> +fi
> +
> +##########################################
>  # check if trace backend exists
>
>  sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 
> 2> /dev/null
> diff --git a/osdep.c b/osdep.c
> index 30426ff..8c09597 100644
> --- a/osdep.c
> +++ b/osdep.c
> @@ -28,6 +28,7 @@
>  #include <errno.h>
>  #include <unistd.h>
>  #include <fcntl.h>
> +#include <sys/mman.h>
>
>  /* Needed early for CONFIG_BSD etc. */
>  #include "config-host.h"
> @@ -139,6 +140,31 @@ void qemu_vfree(void *ptr)
>
>  #endif
>
> +int qemu_madvise(void *addr, size_t len, int advice)
> +{
> +#ifdef CONFIG_POSIX_MADVISE
> +    switch (advice) {
> +        case QEMU_MADV_DONTNEED:
> +            advice = POSIX_MADV_DONTNEED;
> +            break;
> +        default:
> +            fprintf(stderr, "Advice %d unhandled.\n", advice);
> +            abort();

This should be an assert, it's an internal error. It's also common to
both cases.

> +    }
> +    return posix_madvise(addr, len, advice);
> +#else

#elif defined(CONFIG_MADVISE)

> +    switch (advice) {
> +        case QEMU_MADV_DONTNEED:
> +            advice = MADV_DONTNEED;
> +            break;

case QEMU_MADV_DONTFORK:
#ifndef MADV_DONTFORK
return -ENOTSUP;
#else
advice = MADV_DONTFORK;
break;
#endif

Same goes for MADV_MERGEABLE.

> +        default:
> +            fprintf(stderr, "Advice %d unhandled.\n", advice);
> +            abort();
> +    }
> +    return madvise(addr, len, advice);

#else
return -ENOTSUP;