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
