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

[Alexander Graf]

On 12.09.2010, at 00:39, Andreas Färber wrote:

> 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?

Hrm. I'm not sure. Do we have to fail madvise at all?

> 
>>> 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 ..."?

I'd remove the "Advice" part: "qemu_madvise: Flag %x unknown".

> 
>>> +            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.

Oh, so OpenSolaris and Haiku have posix_madvise? Nice.

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

Sure :).

> 
>> 
>>> +#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

This could work, though in general preprocessor magic is ... too magic. I was 
thinking:

#ifdef CONFIG_POSIX_MADVISE
#define QEMU_MADV_DONTNEED POSIX_MADV_DONTNEED
#else
#define QEMU_MADV_DONTNEED MADV_DONTNEED
#endif

and keep the qemu_madvise implementation in C. But then there's no need to 
convert between QEMU_MADV and real MADV bits.

> 
>>> +
>>> +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.

*shrug* I was just afraid this was there for a reason. But maybe the author of 
those lines simply didn't know about posix_madvise.

> Thanks for the review,

You're welcome :)


Alex