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

[Andreas Färber]

Am 12.09.2010 um 09:20 schrieb Blue Swirl:

> On Sat, Sep 11, 2010 at 5:05 PM, Andreas Färber <address@hidden 
> > 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.

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

Thanks,

Andreas