On Sun, Sep 12, 2010 at 8:50 AM, Andreas Färber <andreas.faer...@web.de> wrote: > Am 12.09.2010 um 09:20 schrieb Blue Swirl: > >> On Sat, Sep 11, 2010 at 5:05 PM, Andreas Färber <andreas.faer...@web.de> >> wrote: >>> >>> 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. > > Will do. > >>> 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 did. It's just not obvious that they find their way from the Makefile to a > C header, unlike autoconf. > >> I'd also add a similar check for madvise() if posix_madvise() check >> fails. > > Fine with me. > >>> 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. > > So you disagree with or didn't yet read Alex' suggestion of eliminating this > mapping code in qemu_madvise() altogether? > Doing that in a sensible way would allow code to do: > > #ifdef QEMU_MADV_MERGEABLE > ... > > as before at compile-time. The only qemu_madvise() error condition would > then be the #else below.
That's one way too, if nobody cares about madvise() return values for MADV_MERGEABLE. In any case I'd like to eliminate the #ifdefs in other places than osdep.[ch].
