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

[Alexander Graf <agraf@suse.de>]

Blue Swirl wrote:
> On Sun, Sep 12, 2010 at 12:55 PM, Andreas Färber <andreas.faerber@web.de> wrote:
>   
>> From: Andreas Färber <afaerber@opensolaris.org>
>>
>> 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_*. No functional change
>> except for arch_init.c:ram_load() now potentially falling back to posix_madvise()
>> or no-op in lack of both.
>>
>> 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.
>>
>> v1 -> v2:
>> * Don't rely on posix_madvise() availability, add qemu_madvise().
>>  Suggested by Blue Swirl.
>>
>> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> Cc: Alexander Graf <agraf@suse.de>
>> ---
>>  arch_init.c         |    2 +-
>>  configure           |   33 +++++++++++++++++++++++++++++++++
>>  exec.c              |    8 ++++----
>>  hw/virtio-balloon.c |    4 ++--
>>  kvm-all.c           |    6 +++---
>>  osdep.c             |   15 +++++++++++++++
>>  osdep.h             |   25 +++++++++++++++++++++++++
>>  vl.c                |    3 ---
>>  8 files changed, 83 insertions(+), 13 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..86558eb 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2069,6 +2069,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
>> @@ -2226,6 +2251,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"
>> @@ -2466,6 +2493,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..b1fe3e9 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2841,8 +2841,8 @@ 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);
>> +#ifdef QEMU_MADV_MERGEABLE
>>     
>
> I'd like to avoid these #ifdefs. How about always #defining
> QEMU_MADV_MERGEABLE? QEMU_MADV_* values could be synthetic and not
> rely on MADV_*.
>   

Hrm. Something like

#ifdef MADV_MEREABLE
#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
#else
#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
#endif

would work, right? Only need to find an unused bit in madv ...


Alex