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

[Andreas Färber]

Am 11.09.2010 um 23:37 schrieb Alexander Graf:

> On 11.09.2010, at 19:05, Andreas Färber wrote:
>
> > 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__

> > 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);
>
> Is this the only occurence in the whole code base? This patch should  
> convert all users, right?

It's the only relevant occurrence, cf. description above.

We could in theory create QEMU_MADV_MERGEABLE and QEMU_MADV_DONTFORK  
and convert the callers, too, but what's the point when only madvise  
supports it?
Using the current #ifdef mechanism we can detect/handle it at compile- 
time; inside qemu_madvise it would be deferred to runtime. Or do you  
have a suggestion how to handle that scenario other than returning an  
error?

> > diff --git a/osdep.c b/osdep.c
> > index 30426ff..8c09597 100644
> > --- a/osdep.c
> > +++ b/osdep.c
> > @@ -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);
>
> Advise :)

I'd advise against that advice. ;) (*hint hint*)

> It should probably also be 'madvise' here. Plus, I'd recommend %x as  
> that makes the bits slightly more obvious.

You mean, "qemu_madvise: Advice %x ..."? Or "Advice %x for  
posix_madvise ..."?

> > +            abort();
> > +    }
> > +    return posix_madvise(addr, len, advice);
> > +#else
> > +    switch (advice) {
> > +        case QEMU_MADV_DONTNEED:
> > +            advice = MADV_DONTNEED;
> > +            break;
> > +        default:
> > +            fprintf(stderr, "Advice %d unhandled.\n", advice);
> > +            abort();
> > +    }
> > +    return madvise(addr, len, advice);
>
> So what do you do on haiku where there's no madvise?

It should detect posix_madvise and not run into this code path, just  
like OpenSolaris.
I was hoping for Michael Lotz to resurface though and haven't retried  
myself yet.

Platforms that have neither posix_madvise nor madvise are equally  
broken before and after.

> > +#endif
> > +}
> > +
> > int qemu_create_pidfile(const char *filename)
> > {
> >    char buffer[128];
> > diff --git a/osdep.h b/osdep.h
> > index 1cdc7e2..9f0da46 100644
> > --- a/osdep.h
> > +++ b/osdep.h
> > @@ -90,6 +90,10 @@ void *qemu_memalign(size_t alignment, size_t  
> > size);
> > void *qemu_vmalloc(size_t size);
> > void qemu_vfree(void *ptr);
> >
> > +#define QEMU_MADV_DONTNEED 1
>
> (1 << 0)
>
> There are probably more to follow. I'm also not sure if it wouldn't  
> make sense to just directly map qemu_madvise and real madvise bits.  
> Something like
>
> #ifdef MADV_DONTNEED
> #define QEMU_MADV_DONTNEED   (1 << 0)
> #else
> #define QEMU_MADV_DONTNEED   0
> #endif
>
> Unless of course a flag is mandatory - but I don't think any of the  
> madvise bits are.

I don't follow. Something like the following?

#ifdef CONFIG_POSIX_MADVISE
#define QEMU_MADV_DONTNEED POSIX_MADV_DONTNEED
#define qemu_madvise posix_madvise
#else
#ifdef MADV_DONTNEED
#define QEMU_MADV_DONTNEED MADV_DONTNEED
#endif
#define qemu_madvise madvise
#endif

> > +
> > +int qemu_madvise(void *addr, size_t len, int advice);
> > +
> > int qemu_create_pidfile(const char *filename);
> >
> > #ifdef _WIN32
> > diff --git a/vl.c b/vl.c
> > index 3f45aa9..d352d18 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -80,9 +80,6 @@
> > #include <net/if.h>
> > #include <syslog.h>
> > #include <stropts.h>
> > -/* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for
> > -   discussion about Solaris header problems */
> > -extern int madvise(caddr_t, size_t, int);
>
> Does Solaris have posix_madvise? Otherwise it would still require  
> this header hack, no?

I tested on OpenSolaris 2009.06. Haven't tested on Solaris 10 yet,  
don't have access to older ones.
We could wrap it in #ifndef CONFIG_POSIX_MADVISE to be fool-safe.

Thanks for the review,

Andreas