All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] Introduce qemu_madvise()
@ 2010-09-11 17:05 Andreas Färber
  2010-09-11 21:37 ` Alexander Graf
  2010-09-12  7:20 ` [Qemu-devel] " Blue Swirl
  0 siblings, 2 replies; 28+ messages in thread
From: Andreas Färber @ 2010-09-11 17:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: Blue Swirl, Andreas Färber

From: Andreas Färber <afaerber@opensolaris.org>

vl.c has a Sun-specific hack to supply a prototype for madvise(),
but the call site has apparently moved to arch_init.c.
The underlying issue is that madvise() is not a POSIX function,
therefore Solaris' _POSIX_C_SOURCE suppresses the prototype.

Haiku doesn't implement madvise() at all.
OpenBSD however doesn't implement posix_madvise().

Check for posix_madvise() in configure and supply qemu_madvise() as wrapper.
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__

v1 -> v2:
* Don't rely on posix_madvise() availability, add qemu_madvise().
  Suggested by Blue Swirl.

Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 arch_init.c |    2 +-
 configure   |   11 +++++++++++
 osdep.c     |   26 ++++++++++++++++++++++++++
 osdep.h     |    4 ++++
 vl.c        |    3 ---
 5 files changed, 42 insertions(+), 4 deletions(-)

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);
             }
 #endif
         } else if (flags & RAM_SAVE_FLAG_PAGE) {
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}"
+fi
+
+##########################################
 # check if trace backend exists
 
 sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
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();
+    }
+    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);
+#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
+
+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);
 #endif
 #endif
 #endif
-- 
1.7.2.2

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH] Introduce qemu_madvise()
  2010-09-11 17:05 [Qemu-devel] [PATCH] Introduce qemu_madvise() Andreas Färber
@ 2010-09-11 21:37 ` Alexander Graf
  2010-09-11 22:39   ` Andreas Färber
  2010-09-12  7:20 ` [Qemu-devel] " Blue Swirl
  1 sibling, 1 reply; 28+ messages in thread
From: Alexander Graf @ 2010-09-11 21:37 UTC (permalink / raw)
  To: Andreas Färber; +Cc: Blue Swirl, qemu-devel, Andreas Färber


On 11.09.2010, at 19:05, Andreas Färber wrote:

> From: Andreas Färber <afaerber@opensolaris.org>
> 
> vl.c has a Sun-specific hack to supply a prototype for madvise(),
> but the call site has apparently moved to arch_init.c.
> The underlying issue is that madvise() is not a POSIX function,
> therefore Solaris' _POSIX_C_SOURCE suppresses the prototype.
> 
> Haiku doesn't implement madvise() at all.
> OpenBSD however doesn't implement posix_madvise().
> 
> Check for posix_madvise() in configure and supply qemu_madvise() as wrapper.
> 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__
> 
> v1 -> v2:
> * Don't rely on posix_madvise() availability, add qemu_madvise().
>  Suggested by Blue Swirl.
> 
> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> ---
> arch_init.c |    2 +-
> configure   |   11 +++++++++++
> osdep.c     |   26 ++++++++++++++++++++++++++
> osdep.h     |    4 ++++
> vl.c        |    3 ---
> 5 files changed, 42 insertions(+), 4 deletions(-)
> 
> 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?

>             }
> #endif
>         } else if (flags & RAM_SAVE_FLAG_PAGE) {
> 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}"
> +fi
> +
> +##########################################
> # check if trace backend exists
> 
> sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
> 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);

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

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

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

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


Alex

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH] Introduce qemu_madvise()
  2010-09-11 21:37 ` Alexander Graf
@ 2010-09-11 22:39   ` Andreas Färber
  2010-09-11 22:47     ` Alexander Graf
  0 siblings, 1 reply; 28+ messages in thread
From: Andreas Färber @ 2010-09-11 22:39 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Blue Swirl, QEMU Developers

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] [PATCH] Introduce qemu_madvise()
  2010-09-11 22:39   ` Andreas Färber
@ 2010-09-11 22:47     ` Alexander Graf
  0 siblings, 0 replies; 28+ messages in thread
From: Alexander Graf @ 2010-09-11 22:47 UTC (permalink / raw)
  To: Andreas Färber; +Cc: Blue Swirl, QEMU Developers


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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH] Introduce qemu_madvise()
  2010-09-11 17:05 [Qemu-devel] [PATCH] Introduce qemu_madvise() Andreas Färber
  2010-09-11 21:37 ` Alexander Graf
@ 2010-09-12  7:20 ` Blue Swirl
  2010-09-12  8:50   ` Andreas Färber
  1 sibling, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2010-09-12  7:20 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-devel, Andreas Färber

On Sat, Sep 11, 2010 at 5:05 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> From: Andreas Färber <afaerber@opensolaris.org>
>
> vl.c has a Sun-specific hack to supply a prototype for madvise(),
> but the call site has apparently moved to arch_init.c.
> The underlying issue is that madvise() is not a POSIX function,
> therefore Solaris' _POSIX_C_SOURCE suppresses the prototype.
>
> Haiku doesn't implement madvise() at all.
> OpenBSD however doesn't implement posix_madvise().
>
> Check for posix_madvise() in configure and supply qemu_madvise() as wrapper.
> 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.

>
> v1 -> v2:
> * Don't rely on posix_madvise() availability, add qemu_madvise().
>  Suggested by Blue Swirl.
>
> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> ---
>  arch_init.c |    2 +-
>  configure   |   11 +++++++++++
>  osdep.c     |   26 ++++++++++++++++++++++++++
>  osdep.h     |    4 ++++
>  vl.c        |    3 ---
>  5 files changed, 42 insertions(+), 4 deletions(-)
>
> 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);
>             }
>  #endif
>         } else if (flags & RAM_SAVE_FLAG_PAGE) {
> 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'd also add a similar check for madvise() if posix_madvise() check
fails.

> +fi
> +
> +##########################################
>  # check if trace backend exists
>
>  sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
> 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.

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

#else
return -ENOTSUP;

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH] Introduce qemu_madvise()
  2010-09-12  7:20 ` [Qemu-devel] " Blue Swirl
@ 2010-09-12  8:50   ` Andreas Färber
  2010-09-12  9:02     ` Blue Swirl
  2010-09-12 12:55     ` [Qemu-devel] [PATCH v3] " Andreas Färber
  0 siblings, 2 replies; 28+ messages in thread
From: Andreas Färber @ 2010-09-12  8:50 UTC (permalink / raw)
  To: Blue Swirl; +Cc: Alexander Graf, QEMU Developers, 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 <andreas.faerber@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.

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

Thanks,

Andreas

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH] Introduce qemu_madvise()
  2010-09-12  8:50   ` Andreas Färber
@ 2010-09-12  9:02     ` Blue Swirl
  2010-09-12 12:55     ` [Qemu-devel] [PATCH v3] " Andreas Färber
  1 sibling, 0 replies; 28+ messages in thread
From: Blue Swirl @ 2010-09-12  9:02 UTC (permalink / raw)
  To: Andreas Färber; +Cc: Alexander Graf, QEMU Developers, Andreas Färber

On Sun, Sep 12, 2010 at 8:50 AM, Andreas Färber <andreas.faerber@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.faerber@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].

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] [PATCH v3] Introduce qemu_madvise()
  2010-09-12  8:50   ` Andreas Färber
  2010-09-12  9:02     ` Blue Swirl
@ 2010-09-12 12:55     ` Andreas Färber
  2010-09-12 17:29       ` [Qemu-devel] " Blue Swirl
  1 sibling, 1 reply; 28+ messages in thread
From: Andreas Färber @ 2010-09-12 12:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: Blue Swirl, Alexander Graf

From: Andreas Färber <afaerber@opensolaris.org>

vl.c has a Sun-specific hack to supply a prototype for madvise(),
but the call site has apparently moved to arch_init.c.

Haiku doesn't implement madvise() in favor of posix_madvise().
OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().

Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
Convert all callers to use qemu_madvise() and QEMU_MADV_*. No functional change
except for arch_init.c:ram_load() now potentially falling back to posix_madvise()
or no-op in lack of both.

v2 -> v3:
* Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
* Add configure check for madvise(), too.
  Add defines to Makefile, not QEMU_CFLAGS.
  Convert all callers, untested. Suggested by Blue Swirl.
* Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.

v1 -> v2:
* Don't rely on posix_madvise() availability, add qemu_madvise().
  Suggested by Blue Swirl.

Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
---
 arch_init.c         |    2 +-
 configure           |   33 +++++++++++++++++++++++++++++++++
 exec.c              |    8 ++++----
 hw/virtio-balloon.c |    4 ++--
 kvm-all.c           |    6 +++---
 osdep.c             |   15 +++++++++++++++
 osdep.h             |   25 +++++++++++++++++++++++++
 vl.c                |    3 ---
 8 files changed, 83 insertions(+), 13 deletions(-)

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);
             }
 #endif
         } else if (flags & RAM_SAVE_FLAG_PAGE) {
diff --git a/configure b/configure
index 4061cb7..86558eb 100755
--- a/configure
+++ b/configure
@@ -2069,6 +2069,31 @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
+# check if we have madvise
+
+madvise=no
+cat > $TMPC << EOF
+#include <sys/types.h>
+#include <sys/mman.h>
+int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    madvise=yes
+fi
+
+##########################################
+# check if we have posix_madvise
+
+posix_madvise=no
+cat > $TMPC << EOF
+#include <sys/mman.h>
+int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    posix_madvise=yes
+fi
+
+##########################################
 # check if trace backend exists
 
 sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
@@ -2226,6 +2251,8 @@ echo "KVM support       $kvm"
 echo "fdt support       $fdt"
 echo "preadv support    $preadv"
 echo "fdatasync         $fdatasync"
+echo "madvise           $madvise"
+echo "posix_madvise     $posix_madvise"
 echo "uuid support      $uuid"
 echo "vhost-net support $vhost_net"
 echo "Trace backend     $trace_backend"
@@ -2466,6 +2493,12 @@ fi
 if test "$fdatasync" = "yes" ; then
   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
 fi
+if test "$madvise" = "yes" ; then
+  echo "CONFIG_MADVISE=y" >> $config_host_mak
+fi
+if test "$posix_madvise" = "yes" ; then
+  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
+fi
 
 # XXX: suppress that
 if [ "$bsd" = "yes" ] ; then
diff --git a/exec.c b/exec.c
index 380dab5..b1fe3e9 100644
--- a/exec.c
+++ b/exec.c
@@ -2841,8 +2841,8 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
             new_block->host = file_ram_alloc(new_block, size, mem_path);
             if (!new_block->host) {
                 new_block->host = qemu_vmalloc(size);
-#ifdef MADV_MERGEABLE
-                madvise(new_block->host, size, MADV_MERGEABLE);
+#ifdef QEMU_MADV_MERGEABLE
+                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
 #endif
             }
 #else
@@ -2858,8 +2858,8 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
 #else
             new_block->host = qemu_vmalloc(size);
 #endif
-#ifdef MADV_MERGEABLE
-            madvise(new_block->host, size, MADV_MERGEABLE);
+#ifdef QEMU_MADV_MERGEABLE
+            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
 #endif
         }
     }
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 9fe3886..1e74674 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
 {
 #if defined(__linux__)
     if (!kvm_enabled() || kvm_has_sync_mmu())
-        madvise(addr, TARGET_PAGE_SIZE,
-                deflate ? MADV_WILLNEED : MADV_DONTNEED);
+        qemu_madvise(addr, TARGET_PAGE_SIZE,
+                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
 #endif
 }
 
diff --git a/kvm-all.c b/kvm-all.c
index 58b0404..9393419 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1031,11 +1031,11 @@ int kvm_has_xcrs(void)
 void kvm_setup_guest_memory(void *start, size_t size)
 {
     if (!kvm_has_sync_mmu()) {
-#ifdef MADV_DONTFORK
-        int ret = madvise(start, size, MADV_DONTFORK);
+#ifdef QEMU_MADV_DONTFORK
+        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
 
         if (ret) {
-            perror("madvice");
+            perror("qemu_madvise");
             exit(1);
         }
 #else
diff --git a/osdep.c b/osdep.c
index 30426ff..b5e006f 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"
@@ -35,6 +36,9 @@
 #ifdef CONFIG_SOLARIS
 #include <sys/types.h>
 #include <sys/statvfs.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);
 #endif
 
 #ifdef CONFIG_EVENTFD
@@ -139,6 +143,17 @@ void qemu_vfree(void *ptr)
 
 #endif
 
+int qemu_madvise(void *addr, size_t len, int advice)
+{
+#if defined(CONFIG_MADVISE)
+    return madvise(addr, len, advice);
+#elif defined(CONFIG_POSIX_MADVISE)
+    return posix_madvise(addr, len, advice);
+#else
+    return -ENOTSUP;
+#endif
+}
+
 int qemu_create_pidfile(const char *filename)
 {
     char buffer[128];
diff --git a/osdep.h b/osdep.h
index 1cdc7e2..552e453 100644
--- a/osdep.h
+++ b/osdep.h
@@ -90,6 +90,31 @@ void *qemu_memalign(size_t alignment, size_t size);
 void *qemu_vmalloc(size_t size);
 void qemu_vfree(void *ptr);
 
+#if defined(CONFIG_MADVISE)
+
+#define QEMU_MADV_WILLNEED MADV_WILLNEED
+#define QEMU_MADV_DONTNEED MADV_DONTNEED
+#ifdef MADV_DONTFORK
+#define QEMU_MADV_DONTFORK MADV_DONTFORK
+#endif
+#ifdef MADV_MERGEABLE
+#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
+#endif
+
+#elif defined(CONFIG_POSIX_MADVISE)
+
+#define QEMU_MADV_WILLNEED POSIX_MADV_WILLNEED
+#define QEMU_MADV_DONTNEED POSIX_MADV_DONTNEED
+
+#else /* no-op */
+
+#define QEMU_MADV_WILLNEED (1 << 0)
+#define QEMU_MADV_DONTNEED (2 << 0)
+
+#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);
 #endif
 #endif
 #endif
-- 
1.7.2.2

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH v3] Introduce qemu_madvise()
  2010-09-12 12:55     ` [Qemu-devel] [PATCH v3] " Andreas Färber
@ 2010-09-12 17:29       ` Blue Swirl
  2010-09-13 12:02         ` Alexander Graf
  2010-09-13 21:26         ` [Qemu-devel] [RFC v4] " Andreas Färber
  0 siblings, 2 replies; 28+ messages in thread
From: Blue Swirl @ 2010-09-12 17:29 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-devel, Alexander Graf

On Sun, Sep 12, 2010 at 12:55 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> From: Andreas Färber <afaerber@opensolaris.org>
>
> vl.c has a Sun-specific hack to supply a prototype for madvise(),
> but the call site has apparently moved to arch_init.c.
>
> Haiku doesn't implement madvise() in favor of posix_madvise().
> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>
> Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
> as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
> Convert all callers to use qemu_madvise() and QEMU_MADV_*. No functional change
> except for arch_init.c:ram_load() now potentially falling back to posix_madvise()
> or no-op in lack of both.
>
> v2 -> v3:
> * Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
> * Add configure check for madvise(), too.
>  Add defines to Makefile, not QEMU_CFLAGS.
>  Convert all callers, untested. Suggested by Blue Swirl.
> * Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
>
> v1 -> v2:
> * Don't rely on posix_madvise() availability, add qemu_madvise().
>  Suggested by Blue Swirl.
>
> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> ---
>  arch_init.c         |    2 +-
>  configure           |   33 +++++++++++++++++++++++++++++++++
>  exec.c              |    8 ++++----
>  hw/virtio-balloon.c |    4 ++--
>  kvm-all.c           |    6 +++---
>  osdep.c             |   15 +++++++++++++++
>  osdep.h             |   25 +++++++++++++++++++++++++
>  vl.c                |    3 ---
>  8 files changed, 83 insertions(+), 13 deletions(-)
>
> 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);
>             }
>  #endif
>         } else if (flags & RAM_SAVE_FLAG_PAGE) {
> diff --git a/configure b/configure
> index 4061cb7..86558eb 100755
> --- a/configure
> +++ b/configure
> @@ -2069,6 +2069,31 @@ if compile_prog "" "" ; then
>  fi
>
>  ##########################################
> +# check if we have madvise
> +
> +madvise=no
> +cat > $TMPC << EOF
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    madvise=yes
> +fi
> +
> +##########################################
> +# check if we have posix_madvise
> +
> +posix_madvise=no
> +cat > $TMPC << EOF
> +#include <sys/mman.h>
> +int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    posix_madvise=yes
> +fi
> +
> +##########################################
>  # check if trace backend exists
>
>  sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
> @@ -2226,6 +2251,8 @@ echo "KVM support       $kvm"
>  echo "fdt support       $fdt"
>  echo "preadv support    $preadv"
>  echo "fdatasync         $fdatasync"
> +echo "madvise           $madvise"
> +echo "posix_madvise     $posix_madvise"
>  echo "uuid support      $uuid"
>  echo "vhost-net support $vhost_net"
>  echo "Trace backend     $trace_backend"
> @@ -2466,6 +2493,12 @@ fi
>  if test "$fdatasync" = "yes" ; then
>   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
>  fi
> +if test "$madvise" = "yes" ; then
> +  echo "CONFIG_MADVISE=y" >> $config_host_mak
> +fi
> +if test "$posix_madvise" = "yes" ; then
> +  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
> +fi
>
>  # XXX: suppress that
>  if [ "$bsd" = "yes" ] ; then
> diff --git a/exec.c b/exec.c
> index 380dab5..b1fe3e9 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2841,8 +2841,8 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>             new_block->host = file_ram_alloc(new_block, size, mem_path);
>             if (!new_block->host) {
>                 new_block->host = qemu_vmalloc(size);
> -#ifdef MADV_MERGEABLE
> -                madvise(new_block->host, size, MADV_MERGEABLE);
> +#ifdef QEMU_MADV_MERGEABLE

I'd like to avoid these #ifdefs. How about always #defining
QEMU_MADV_MERGEABLE? QEMU_MADV_* values could be synthetic and not
rely on MADV_*.

> +                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>  #endif
>             }
>  #else
> @@ -2858,8 +2858,8 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>  #else
>             new_block->host = qemu_vmalloc(size);
>  #endif
> -#ifdef MADV_MERGEABLE
> -            madvise(new_block->host, size, MADV_MERGEABLE);
> +#ifdef QEMU_MADV_MERGEABLE
> +            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>  #endif
>         }
>     }
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 9fe3886..1e74674 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
>  {
>  #if defined(__linux__)
>     if (!kvm_enabled() || kvm_has_sync_mmu())
> -        madvise(addr, TARGET_PAGE_SIZE,
> -                deflate ? MADV_WILLNEED : MADV_DONTNEED);
> +        qemu_madvise(addr, TARGET_PAGE_SIZE,
> +                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
>  #endif
>  }
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 58b0404..9393419 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1031,11 +1031,11 @@ int kvm_has_xcrs(void)
>  void kvm_setup_guest_memory(void *start, size_t size)
>  {
>     if (!kvm_has_sync_mmu()) {
> -#ifdef MADV_DONTFORK
> -        int ret = madvise(start, size, MADV_DONTFORK);
> +#ifdef QEMU_MADV_DONTFORK
> +        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
>
>         if (ret) {
> -            perror("madvice");
> +            perror("qemu_madvise");
>             exit(1);
>         }
>  #else

Here the approach could be that if the return value is -ENOTSUP, we
don't print the error message. Then the #ifdefs could be dropped.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH v3] Introduce qemu_madvise()
  2010-09-12 17:29       ` [Qemu-devel] " Blue Swirl
@ 2010-09-13 12:02         ` Alexander Graf
  2010-09-13 21:26         ` [Qemu-devel] [RFC v4] " Andreas Färber
  1 sibling, 0 replies; 28+ messages in thread
From: Alexander Graf @ 2010-09-13 12:02 UTC (permalink / raw)
  To: Blue Swirl; +Cc: Andreas Färber, qemu-devel

Blue Swirl wrote:
> On Sun, Sep 12, 2010 at 12:55 PM, Andreas Färber <andreas.faerber@web.de> wrote:
>   
>> From: Andreas Färber <afaerber@opensolaris.org>
>>
>> vl.c has a Sun-specific hack to supply a prototype for madvise(),
>> but the call site has apparently moved to arch_init.c.
>>
>> Haiku doesn't implement madvise() in favor of posix_madvise().
>> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>>
>> Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
>> as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
>> Convert all callers to use qemu_madvise() and QEMU_MADV_*. No functional change
>> except for arch_init.c:ram_load() now potentially falling back to posix_madvise()
>> or no-op in lack of both.
>>
>> v2 -> v3:
>> * Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
>> * Add configure check for madvise(), too.
>>  Add defines to Makefile, not QEMU_CFLAGS.
>>  Convert all callers, untested. Suggested by Blue Swirl.
>> * Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
>>
>> v1 -> v2:
>> * Don't rely on posix_madvise() availability, add qemu_madvise().
>>  Suggested by Blue Swirl.
>>
>> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> Cc: Alexander Graf <agraf@suse.de>
>> ---
>>  arch_init.c         |    2 +-
>>  configure           |   33 +++++++++++++++++++++++++++++++++
>>  exec.c              |    8 ++++----
>>  hw/virtio-balloon.c |    4 ++--
>>  kvm-all.c           |    6 +++---
>>  osdep.c             |   15 +++++++++++++++
>>  osdep.h             |   25 +++++++++++++++++++++++++
>>  vl.c                |    3 ---
>>  8 files changed, 83 insertions(+), 13 deletions(-)
>>
>> 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);
>>             }
>>  #endif
>>         } else if (flags & RAM_SAVE_FLAG_PAGE) {
>> diff --git a/configure b/configure
>> index 4061cb7..86558eb 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2069,6 +2069,31 @@ if compile_prog "" "" ; then
>>  fi
>>
>>  ##########################################
>> +# check if we have madvise
>> +
>> +madvise=no
>> +cat > $TMPC << EOF
>> +#include <sys/types.h>
>> +#include <sys/mman.h>
>> +int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
>> +EOF
>> +if compile_prog "" "" ; then
>> +    madvise=yes
>> +fi
>> +
>> +##########################################
>> +# check if we have posix_madvise
>> +
>> +posix_madvise=no
>> +cat > $TMPC << EOF
>> +#include <sys/mman.h>
>> +int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
>> +EOF
>> +if compile_prog "" "" ; then
>> +    posix_madvise=yes
>> +fi
>> +
>> +##########################################
>>  # check if trace backend exists
>>
>>  sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
>> @@ -2226,6 +2251,8 @@ echo "KVM support       $kvm"
>>  echo "fdt support       $fdt"
>>  echo "preadv support    $preadv"
>>  echo "fdatasync         $fdatasync"
>> +echo "madvise           $madvise"
>> +echo "posix_madvise     $posix_madvise"
>>  echo "uuid support      $uuid"
>>  echo "vhost-net support $vhost_net"
>>  echo "Trace backend     $trace_backend"
>> @@ -2466,6 +2493,12 @@ fi
>>  if test "$fdatasync" = "yes" ; then
>>   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
>>  fi
>> +if test "$madvise" = "yes" ; then
>> +  echo "CONFIG_MADVISE=y" >> $config_host_mak
>> +fi
>> +if test "$posix_madvise" = "yes" ; then
>> +  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
>> +fi
>>
>>  # XXX: suppress that
>>  if [ "$bsd" = "yes" ] ; then
>> diff --git a/exec.c b/exec.c
>> index 380dab5..b1fe3e9 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2841,8 +2841,8 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>>             new_block->host = file_ram_alloc(new_block, size, mem_path);
>>             if (!new_block->host) {
>>                 new_block->host = qemu_vmalloc(size);
>> -#ifdef MADV_MERGEABLE
>> -                madvise(new_block->host, size, MADV_MERGEABLE);
>> +#ifdef QEMU_MADV_MERGEABLE
>>     
>
> I'd like to avoid these #ifdefs. How about always #defining
> QEMU_MADV_MERGEABLE? QEMU_MADV_* values could be synthetic and not
> rely on MADV_*.
>   

Hrm. Something like

#ifdef MADV_MEREABLE
#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
#else
#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
#endif

would work, right? Only need to find an unused bit in madv ...


Alex

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] [RFC v4] Introduce qemu_madvise()
  2010-09-12 17:29       ` [Qemu-devel] " Blue Swirl
  2010-09-13 12:02         ` Alexander Graf
@ 2010-09-13 21:26         ` Andreas Färber
  2010-09-14 16:31           ` [Qemu-devel] " Blue Swirl
  1 sibling, 1 reply; 28+ messages in thread
From: Andreas Färber @ 2010-09-13 21:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: Blue Swirl, Alexander Graf

From: Andreas Färber <afaerber@opensolaris.org>

vl.c has a Sun-specific hack to supply a prototype for madvise(),
but the call site has apparently moved to arch_init.c.

Haiku doesn't implement madvise() in favor of posix_madvise().
OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().

Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
Convert all callers to use qemu_madvise() and QEMU_MADV_*.

Note that on Solaris the warning is fixed by moving the madvise() prototype,
not by qemu_madvise() itself. It will help with future porting though.

v3 -> v4:
* Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
  This will currently break the check in kvm-all.c by calling madvise() with
  a supported flag, which will not fail. Ideas/patches welcome.

v2 -> v3:
* Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
* Add configure check for madvise(), too.
  Add defines to Makefile, not QEMU_CFLAGS.
  Convert all callers, untested. Suggested by Blue Swirl.
* Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
* Display configure check results.

v1 -> v2:
* Don't rely on posix_madvise() availability, add qemu_madvise().
  Suggested by Blue Swirl.

Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
---
 arch_init.c         |    2 +-
 configure           |   33 +++++++++++++++++++++++++++++++++
 exec.c              |    8 ++------
 hw/virtio-balloon.c |    4 ++--
 kvm-all.c           |   15 +++++++--------
 osdep.c             |   15 +++++++++++++++
 osdep.h             |   33 +++++++++++++++++++++++++++++++++
 vl.c                |    3 ---
 8 files changed, 93 insertions(+), 20 deletions(-)

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);
             }
 #endif
         } else if (flags & RAM_SAVE_FLAG_PAGE) {
diff --git a/configure b/configure
index 4061cb7..86558eb 100755
--- a/configure
+++ b/configure
@@ -2069,6 +2069,31 @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
+# check if we have madvise
+
+madvise=no
+cat > $TMPC << EOF
+#include <sys/types.h>
+#include <sys/mman.h>
+int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    madvise=yes
+fi
+
+##########################################
+# check if we have posix_madvise
+
+posix_madvise=no
+cat > $TMPC << EOF
+#include <sys/mman.h>
+int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    posix_madvise=yes
+fi
+
+##########################################
 # check if trace backend exists
 
 sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
@@ -2226,6 +2251,8 @@ echo "KVM support       $kvm"
 echo "fdt support       $fdt"
 echo "preadv support    $preadv"
 echo "fdatasync         $fdatasync"
+echo "madvise           $madvise"
+echo "posix_madvise     $posix_madvise"
 echo "uuid support      $uuid"
 echo "vhost-net support $vhost_net"
 echo "Trace backend     $trace_backend"
@@ -2466,6 +2493,12 @@ fi
 if test "$fdatasync" = "yes" ; then
   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
 fi
+if test "$madvise" = "yes" ; then
+  echo "CONFIG_MADVISE=y" >> $config_host_mak
+fi
+if test "$posix_madvise" = "yes" ; then
+  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
+fi
 
 # XXX: suppress that
 if [ "$bsd" = "yes" ] ; then
diff --git a/exec.c b/exec.c
index 380dab5..9b5464f 100644
--- a/exec.c
+++ b/exec.c
@@ -2841,9 +2841,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
             new_block->host = file_ram_alloc(new_block, size, mem_path);
             if (!new_block->host) {
                 new_block->host = qemu_vmalloc(size);
-#ifdef MADV_MERGEABLE
-                madvise(new_block->host, size, MADV_MERGEABLE);
-#endif
+                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
             }
 #else
             fprintf(stderr, "-mem-path option unsupported\n");
@@ -2858,9 +2856,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
 #else
             new_block->host = qemu_vmalloc(size);
 #endif
-#ifdef MADV_MERGEABLE
-            madvise(new_block->host, size, MADV_MERGEABLE);
-#endif
+            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
         }
     }
 
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 9fe3886..1e74674 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
 {
 #if defined(__linux__)
     if (!kvm_enabled() || kvm_has_sync_mmu())
-        madvise(addr, TARGET_PAGE_SIZE,
-                deflate ? MADV_WILLNEED : MADV_DONTNEED);
+        qemu_madvise(addr, TARGET_PAGE_SIZE,
+                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
 #endif
 }
 
diff --git a/kvm-all.c b/kvm-all.c
index 58b0404..f7c1d12 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1031,18 +1031,17 @@ int kvm_has_xcrs(void)
 void kvm_setup_guest_memory(void *start, size_t size)
 {
     if (!kvm_has_sync_mmu()) {
-#ifdef MADV_DONTFORK
-        int ret = madvise(start, size, MADV_DONTFORK);
+        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
 
+        if (ret == -ENOTSUP) {
+            fprintf(stderr,
+                    "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
+            exit(1);
+        }
         if (ret) {
-            perror("madvice");
+            perror("qemu_madvise");
             exit(1);
         }
-#else
-        fprintf(stderr,
-                "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
-        exit(1);
-#endif
     }
 }
 
diff --git a/osdep.c b/osdep.c
index 30426ff..b5e006f 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"
@@ -35,6 +36,9 @@
 #ifdef CONFIG_SOLARIS
 #include <sys/types.h>
 #include <sys/statvfs.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);
 #endif
 
 #ifdef CONFIG_EVENTFD
@@ -139,6 +143,17 @@ void qemu_vfree(void *ptr)
 
 #endif
 
+int qemu_madvise(void *addr, size_t len, int advice)
+{
+#if defined(CONFIG_MADVISE)
+    return madvise(addr, len, advice);
+#elif defined(CONFIG_POSIX_MADVISE)
+    return posix_madvise(addr, len, advice);
+#else
+    return -ENOTSUP;
+#endif
+}
+
 int qemu_create_pidfile(const char *filename)
 {
     char buffer[128];
diff --git a/osdep.h b/osdep.h
index 1cdc7e2..2ba7f71 100644
--- a/osdep.h
+++ b/osdep.h
@@ -90,6 +90,39 @@ void *qemu_memalign(size_t alignment, size_t size);
 void *qemu_vmalloc(size_t size);
 void qemu_vfree(void *ptr);
 
+#if defined(CONFIG_MADVISE)
+
+#define QEMU_MADV_WILLNEED  MADV_WILLNEED
+#define QEMU_MADV_DONTNEED  MADV_DONTNEED
+#ifdef MADV_DONTFORK
+#define QEMU_MADV_DONTFORK  MADV_DONTFORK
+#else
+#define QEMU_MADV_DONTFORK  MADV_NORMAL
+#endif
+#ifdef MADV_MERGEABLE
+#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
+#else
+#define QEMU_MADV_MERGEABLE MADV_NORMAL
+#endif
+
+#elif defined(CONFIG_POSIX_MADVISE)
+
+#define QEMU_MADV_WILLNEED  POSIX_MADV_WILLNEED
+#define QEMU_MADV_DONTNEED  POSIX_MADV_DONTNEED
+#define QEMU_MADV_DONTFORK  POSIX_MADV_NORMAL
+#define QEMU_MADV_MERGEABLE POSIX_MADV_NORMAL
+
+#else /* no-op */
+
+#define QEMU_MADV_WILLNEED  0
+#define QEMU_MADV_DONTNEED  0
+#define QEMU_MADV_DONTFORK  0
+#define QEMU_MADV_MERGEABLE 0
+
+#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);
 #endif
 #endif
 #endif
-- 
1.7.2.2

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [RFC v4] Introduce qemu_madvise()
  2010-09-13 21:26         ` [Qemu-devel] [RFC v4] " Andreas Färber
@ 2010-09-14 16:31           ` Blue Swirl
  2010-09-14 16:34             ` Alexander Graf
  0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2010-09-14 16:31 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-devel, Alexander Graf

On Mon, Sep 13, 2010 at 9:26 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> From: Andreas Färber <afaerber@opensolaris.org>
>
> vl.c has a Sun-specific hack to supply a prototype for madvise(),
> but the call site has apparently moved to arch_init.c.
>
> Haiku doesn't implement madvise() in favor of posix_madvise().
> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>
> Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
> as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
> Convert all callers to use qemu_madvise() and QEMU_MADV_*.
>
> Note that on Solaris the warning is fixed by moving the madvise() prototype,
> not by qemu_madvise() itself. It will help with future porting though.
>
> v3 -> v4:
> * Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
>  This will currently break the check in kvm-all.c by calling madvise() with
>  a supported flag, which will not fail. Ideas/patches welcome.

Your original switch with synthetic (1<<0, 1<<1 etc) values for
QEMU_MADV_* for all hosts (never reusing MADV_* defines).

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [RFC v4] Introduce qemu_madvise()
  2010-09-14 16:31           ` [Qemu-devel] " Blue Swirl
@ 2010-09-14 16:34             ` Alexander Graf
  2010-09-14 17:10               ` Blue Swirl
  0 siblings, 1 reply; 28+ messages in thread
From: Alexander Graf @ 2010-09-14 16:34 UTC (permalink / raw)
  To: Blue Swirl; +Cc: Andreas Färber, qemu-devel


Am 14.09.2010 um 18:31 schrieb Blue Swirl <blauwirbel@gmail.com>:

> On Mon, Sep 13, 2010 at 9:26 PM, Andreas Färber <andreas.faerber@web.de> wrote:
>> From: Andreas Färber <afaerber@opensolaris.org>
>> 
>> vl.c has a Sun-specific hack to supply a prototype for madvise(),
>> but the call site has apparently moved to arch_init.c.
>> 
>> Haiku doesn't implement madvise() in favor of posix_madvise().
>> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>> 
>> Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
>> as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
>> Convert all callers to use qemu_madvise() and QEMU_MADV_*.
>> 
>> Note that on Solaris the warning is fixed by moving the madvise() prototype,
>> not by qemu_madvise() itself. It will help with future porting though.
>> 
>> v3 -> v4:
>> * Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
>>  This will currently break the check in kvm-all.c by calling madvise() with
>>  a supported flag, which will not fail. Ideas/patches welcome.
> 
> Your original switch with synthetic (1<<0, 1<<1 etc) values for
> QEMU_MADV_* for all hosts (never reusing MADV_* defines).

Ugh. Back to square one? How about

#define QEMU_MADV_INVALID -1

Then you can define unknown balues against that. Should never occur otherwise.


Alex

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [RFC v4] Introduce qemu_madvise()
  2010-09-14 16:34             ` Alexander Graf
@ 2010-09-14 17:10               ` Blue Swirl
  2010-09-14 20:28                 ` [Qemu-devel] [PATCH v5] " Andreas Färber
  0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2010-09-14 17:10 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Andreas Färber, qemu-devel

On Tue, Sep 14, 2010 at 4:34 PM, Alexander Graf <agraf@suse.de> wrote:
>
> Am 14.09.2010 um 18:31 schrieb Blue Swirl <blauwirbel@gmail.com>:
>
>> On Mon, Sep 13, 2010 at 9:26 PM, Andreas Färber <andreas.faerber@web.de> wrote:
>>> From: Andreas Färber <afaerber@opensolaris.org>
>>>
>>> vl.c has a Sun-specific hack to supply a prototype for madvise(),
>>> but the call site has apparently moved to arch_init.c.
>>>
>>> Haiku doesn't implement madvise() in favor of posix_madvise().
>>> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>>>
>>> Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
>>> as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
>>> Convert all callers to use qemu_madvise() and QEMU_MADV_*.
>>>
>>> Note that on Solaris the warning is fixed by moving the madvise() prototype,
>>> not by qemu_madvise() itself. It will help with future porting though.
>>>
>>> v3 -> v4:
>>> * Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
>>>  This will currently break the check in kvm-all.c by calling madvise() with
>>>  a supported flag, which will not fail. Ideas/patches welcome.
>>
>> Your original switch with synthetic (1<<0, 1<<1 etc) values for
>> QEMU_MADV_* for all hosts (never reusing MADV_* defines).
>
> Ugh. Back to square one? How about
>
> #define QEMU_MADV_INVALID -1
>
> Then you can define unknown balues against that. Should never occur otherwise.

Yeah, then the generic code could check for QEMU_MADV_INVALID. That's
simpler than the switch.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] [PATCH v5] Introduce qemu_madvise()
  2010-09-14 17:10               ` Blue Swirl
@ 2010-09-14 20:28                 ` Andreas Färber
  2010-09-14 20:36                   ` [Qemu-devel] " Blue Swirl
  0 siblings, 1 reply; 28+ messages in thread
From: Andreas Färber @ 2010-09-14 20:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: Blue Swirl, Alexander Graf

From: Andreas Färber <afaerber@opensolaris.org>

vl.c has a Sun-specific hack to supply a prototype for madvise(),
but the call site has apparently moved to arch_init.c.

Haiku doesn't implement madvise() in favor of posix_madvise().
OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().

Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
Convert all callers to use qemu_madvise() and QEMU_MADV_*.

Note that on Solaris the warning is fixed by moving the madvise() prototype,
not by qemu_madvise() itself. It will help with future porting though, and
it simplifies most call sites.

v4 -> v5:
* Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
  Note that this relies on -1 not being a valid advice value.

v3 -> v4:
* Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
  This will currently break the check in kvm-all.c by calling madvise() with
  a supported flag, which will not fail. Ideas/patches welcome.

v2 -> v3:
* Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
* Add configure check for madvise(), too.
  Add defines to Makefile, not QEMU_CFLAGS.
  Convert all callers, untested. Suggested by Blue Swirl.
* Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
* Display configure check results.

v1 -> v2:
* Don't rely on posix_madvise() availability, add qemu_madvise().
  Suggested by Blue Swirl.

Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
---
 arch_init.c         |    2 +-
 configure           |   33 +++++++++++++++++++++++++++++++++
 exec.c              |    8 ++------
 hw/virtio-balloon.c |    4 ++--
 kvm-all.c           |   15 +++++++--------
 osdep.c             |   18 ++++++++++++++++++
 osdep.h             |   35 +++++++++++++++++++++++++++++++++++
 vl.c                |    3 ---
 8 files changed, 98 insertions(+), 20 deletions(-)

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);
             }
 #endif
         } else if (flags & RAM_SAVE_FLAG_PAGE) {
diff --git a/configure b/configure
index 4061cb7..86558eb 100755
--- a/configure
+++ b/configure
@@ -2069,6 +2069,31 @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
+# check if we have madvise
+
+madvise=no
+cat > $TMPC << EOF
+#include <sys/types.h>
+#include <sys/mman.h>
+int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    madvise=yes
+fi
+
+##########################################
+# check if we have posix_madvise
+
+posix_madvise=no
+cat > $TMPC << EOF
+#include <sys/mman.h>
+int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    posix_madvise=yes
+fi
+
+##########################################
 # check if trace backend exists
 
 sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
@@ -2226,6 +2251,8 @@ echo "KVM support       $kvm"
 echo "fdt support       $fdt"
 echo "preadv support    $preadv"
 echo "fdatasync         $fdatasync"
+echo "madvise           $madvise"
+echo "posix_madvise     $posix_madvise"
 echo "uuid support      $uuid"
 echo "vhost-net support $vhost_net"
 echo "Trace backend     $trace_backend"
@@ -2466,6 +2493,12 @@ fi
 if test "$fdatasync" = "yes" ; then
   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
 fi
+if test "$madvise" = "yes" ; then
+  echo "CONFIG_MADVISE=y" >> $config_host_mak
+fi
+if test "$posix_madvise" = "yes" ; then
+  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
+fi
 
 # XXX: suppress that
 if [ "$bsd" = "yes" ] ; then
diff --git a/exec.c b/exec.c
index 380dab5..9b5464f 100644
--- a/exec.c
+++ b/exec.c
@@ -2841,9 +2841,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
             new_block->host = file_ram_alloc(new_block, size, mem_path);
             if (!new_block->host) {
                 new_block->host = qemu_vmalloc(size);
-#ifdef MADV_MERGEABLE
-                madvise(new_block->host, size, MADV_MERGEABLE);
-#endif
+                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
             }
 #else
             fprintf(stderr, "-mem-path option unsupported\n");
@@ -2858,9 +2856,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
 #else
             new_block->host = qemu_vmalloc(size);
 #endif
-#ifdef MADV_MERGEABLE
-            madvise(new_block->host, size, MADV_MERGEABLE);
-#endif
+            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
         }
     }
 
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 9fe3886..1e74674 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
 {
 #if defined(__linux__)
     if (!kvm_enabled() || kvm_has_sync_mmu())
-        madvise(addr, TARGET_PAGE_SIZE,
-                deflate ? MADV_WILLNEED : MADV_DONTNEED);
+        qemu_madvise(addr, TARGET_PAGE_SIZE,
+                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
 #endif
 }
 
diff --git a/kvm-all.c b/kvm-all.c
index 58b0404..f7c1d12 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1031,18 +1031,17 @@ int kvm_has_xcrs(void)
 void kvm_setup_guest_memory(void *start, size_t size)
 {
     if (!kvm_has_sync_mmu()) {
-#ifdef MADV_DONTFORK
-        int ret = madvise(start, size, MADV_DONTFORK);
+        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
 
+        if (ret == -ENOTSUP) {
+            fprintf(stderr,
+                    "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
+            exit(1);
+        }
         if (ret) {
-            perror("madvice");
+            perror("qemu_madvise");
             exit(1);
         }
-#else
-        fprintf(stderr,
-                "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
-        exit(1);
-#endif
     }
 }
 
diff --git a/osdep.c b/osdep.c
index 30426ff..1724a58 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"
@@ -35,6 +36,9 @@
 #ifdef CONFIG_SOLARIS
 #include <sys/types.h>
 #include <sys/statvfs.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);
 #endif
 
 #ifdef CONFIG_EVENTFD
@@ -139,6 +143,20 @@ void qemu_vfree(void *ptr)
 
 #endif
 
+int qemu_madvise(void *addr, size_t len, int advice)
+{
+    if (advice == QEMU_MADV_INVALID) {
+        return -ENOTSUP;
+    }
+#if defined(CONFIG_MADVISE)
+    return madvise(addr, len, advice);
+#elif defined(CONFIG_POSIX_MADVISE)
+    return posix_madvise(addr, len, advice);
+#else
+    return -ENOTSUP;
+#endif
+}
+
 int qemu_create_pidfile(const char *filename)
 {
     char buffer[128];
diff --git a/osdep.h b/osdep.h
index 1cdc7e2..6fb4ff3 100644
--- a/osdep.h
+++ b/osdep.h
@@ -90,6 +90,41 @@ void *qemu_memalign(size_t alignment, size_t size);
 void *qemu_vmalloc(size_t size);
 void qemu_vfree(void *ptr);
 
+#define QEMU_MADV_INVALID -1
+
+#if defined(CONFIG_MADVISE)
+
+#define QEMU_MADV_WILLNEED  MADV_WILLNEED
+#define QEMU_MADV_DONTNEED  MADV_DONTNEED
+#ifdef MADV_DONTFORK
+#define QEMU_MADV_DONTFORK  MADV_DONTFORK
+#else
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#endif
+#ifdef MADV_MERGEABLE
+#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
+#else
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+#endif
+
+#elif defined(CONFIG_POSIX_MADVISE)
+
+#define QEMU_MADV_WILLNEED  POSIX_MADV_WILLNEED
+#define QEMU_MADV_DONTNEED  POSIX_MADV_DONTNEED
+#define QEMU_MADV_DONTFORK  POSIX_MADV_NORMAL
+#define QEMU_MADV_MERGEABLE POSIX_MADV_NORMAL
+
+#else /* no-op */
+
+#define QEMU_MADV_WILLNEED  QEMU_MADV_INVALID
+#define QEMU_MADV_DONTNEED  QEMU_MADV_INVALID
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+
+#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);
 #endif
 #endif
 #endif
-- 
1.7.2.2

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH v5] Introduce qemu_madvise()
  2010-09-14 20:28                 ` [Qemu-devel] [PATCH v5] " Andreas Färber
@ 2010-09-14 20:36                   ` Blue Swirl
  2010-09-14 20:39                     ` Andreas Färber
  0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2010-09-14 20:36 UTC (permalink / raw)
  To: Andreas Färber; +Cc: qemu-devel, Alexander Graf

On Tue, Sep 14, 2010 at 8:28 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> From: Andreas Färber <afaerber@opensolaris.org>
>
> vl.c has a Sun-specific hack to supply a prototype for madvise(),
> but the call site has apparently moved to arch_init.c.
>
> Haiku doesn't implement madvise() in favor of posix_madvise().
> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>
> Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
> as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
> Convert all callers to use qemu_madvise() and QEMU_MADV_*.
>
> Note that on Solaris the warning is fixed by moving the madvise() prototype,
> not by qemu_madvise() itself. It will help with future porting though, and
> it simplifies most call sites.
>
> v4 -> v5:
> * Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
>  Note that this relies on -1 not being a valid advice value.
>
> v3 -> v4:
> * Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
>  This will currently break the check in kvm-all.c by calling madvise() with
>  a supported flag, which will not fail. Ideas/patches welcome.
>
> v2 -> v3:
> * Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
> * Add configure check for madvise(), too.
>  Add defines to Makefile, not QEMU_CFLAGS.
>  Convert all callers, untested. Suggested by Blue Swirl.
> * Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
> * Display configure check results.
>
> v1 -> v2:
> * Don't rely on posix_madvise() availability, add qemu_madvise().
>  Suggested by Blue Swirl.
>
> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> ---
>  arch_init.c         |    2 +-
>  configure           |   33 +++++++++++++++++++++++++++++++++
>  exec.c              |    8 ++------
>  hw/virtio-balloon.c |    4 ++--
>  kvm-all.c           |   15 +++++++--------
>  osdep.c             |   18 ++++++++++++++++++
>  osdep.h             |   35 +++++++++++++++++++++++++++++++++++
>  vl.c                |    3 ---
>  8 files changed, 98 insertions(+), 20 deletions(-)
>
> 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);
>             }
>  #endif
>         } else if (flags & RAM_SAVE_FLAG_PAGE) {
> diff --git a/configure b/configure
> index 4061cb7..86558eb 100755
> --- a/configure
> +++ b/configure
> @@ -2069,6 +2069,31 @@ if compile_prog "" "" ; then
>  fi
>
>  ##########################################
> +# check if we have madvise
> +
> +madvise=no
> +cat > $TMPC << EOF
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    madvise=yes
> +fi
> +
> +##########################################
> +# check if we have posix_madvise
> +
> +posix_madvise=no
> +cat > $TMPC << EOF
> +#include <sys/mman.h>
> +int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    posix_madvise=yes
> +fi
> +
> +##########################################
>  # check if trace backend exists
>
>  sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
> @@ -2226,6 +2251,8 @@ echo "KVM support       $kvm"
>  echo "fdt support       $fdt"
>  echo "preadv support    $preadv"
>  echo "fdatasync         $fdatasync"
> +echo "madvise           $madvise"
> +echo "posix_madvise     $posix_madvise"
>  echo "uuid support      $uuid"
>  echo "vhost-net support $vhost_net"
>  echo "Trace backend     $trace_backend"
> @@ -2466,6 +2493,12 @@ fi
>  if test "$fdatasync" = "yes" ; then
>   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
>  fi
> +if test "$madvise" = "yes" ; then
> +  echo "CONFIG_MADVISE=y" >> $config_host_mak
> +fi
> +if test "$posix_madvise" = "yes" ; then
> +  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
> +fi
>
>  # XXX: suppress that
>  if [ "$bsd" = "yes" ] ; then
> diff --git a/exec.c b/exec.c
> index 380dab5..9b5464f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2841,9 +2841,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>             new_block->host = file_ram_alloc(new_block, size, mem_path);
>             if (!new_block->host) {
>                 new_block->host = qemu_vmalloc(size);
> -#ifdef MADV_MERGEABLE
> -                madvise(new_block->host, size, MADV_MERGEABLE);
> -#endif
> +                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>             }
>  #else
>             fprintf(stderr, "-mem-path option unsupported\n");
> @@ -2858,9 +2856,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>  #else
>             new_block->host = qemu_vmalloc(size);
>  #endif
> -#ifdef MADV_MERGEABLE
> -            madvise(new_block->host, size, MADV_MERGEABLE);
> -#endif
> +            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>         }
>     }
>
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 9fe3886..1e74674 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
>  {
>  #if defined(__linux__)
>     if (!kvm_enabled() || kvm_has_sync_mmu())
> -        madvise(addr, TARGET_PAGE_SIZE,
> -                deflate ? MADV_WILLNEED : MADV_DONTNEED);
> +        qemu_madvise(addr, TARGET_PAGE_SIZE,
> +                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
>  #endif
>  }
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 58b0404..f7c1d12 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1031,18 +1031,17 @@ int kvm_has_xcrs(void)
>  void kvm_setup_guest_memory(void *start, size_t size)
>  {
>     if (!kvm_has_sync_mmu()) {
> -#ifdef MADV_DONTFORK
> -        int ret = madvise(start, size, MADV_DONTFORK);
> +        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
>
> +        if (ret == -ENOTSUP) {
> +            fprintf(stderr,
> +                    "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
> +            exit(1);
> +        }
>         if (ret) {
> -            perror("madvice");
> +            perror("qemu_madvise");
>             exit(1);
>         }
> -#else
> -        fprintf(stderr,
> -                "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
> -        exit(1);
> -#endif
>     }
>  }
>
> diff --git a/osdep.c b/osdep.c
> index 30426ff..1724a58 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"
> @@ -35,6 +36,9 @@
>  #ifdef CONFIG_SOLARIS
>  #include <sys/types.h>
>  #include <sys/statvfs.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);
>  #endif
>
>  #ifdef CONFIG_EVENTFD
> @@ -139,6 +143,20 @@ void qemu_vfree(void *ptr)
>
>  #endif
>
> +int qemu_madvise(void *addr, size_t len, int advice)
> +{
> +    if (advice == QEMU_MADV_INVALID) {
> +        return -ENOTSUP;
> +    }
> +#if defined(CONFIG_MADVISE)
> +    return madvise(addr, len, advice);
> +#elif defined(CONFIG_POSIX_MADVISE)
> +    return posix_madvise(addr, len, advice);
> +#else
> +    return -ENOTSUP;
> +#endif
> +}
> +
>  int qemu_create_pidfile(const char *filename)
>  {
>     char buffer[128];
> diff --git a/osdep.h b/osdep.h
> index 1cdc7e2..6fb4ff3 100644
> --- a/osdep.h
> +++ b/osdep.h
> @@ -90,6 +90,41 @@ void *qemu_memalign(size_t alignment, size_t size);
>  void *qemu_vmalloc(size_t size);
>  void qemu_vfree(void *ptr);
>
> +#define QEMU_MADV_INVALID -1
> +
> +#if defined(CONFIG_MADVISE)
> +
> +#define QEMU_MADV_WILLNEED  MADV_WILLNEED
> +#define QEMU_MADV_DONTNEED  MADV_DONTNEED
> +#ifdef MADV_DONTFORK
> +#define QEMU_MADV_DONTFORK  MADV_DONTFORK
> +#else
> +#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
> +#endif
> +#ifdef MADV_MERGEABLE
> +#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
> +#else
> +#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
> +#endif
> +
> +#elif defined(CONFIG_POSIX_MADVISE)
> +
> +#define QEMU_MADV_WILLNEED  POSIX_MADV_WILLNEED
> +#define QEMU_MADV_DONTNEED  POSIX_MADV_DONTNEED
> +#define QEMU_MADV_DONTFORK  POSIX_MADV_NORMAL
> +#define QEMU_MADV_MERGEABLE POSIX_MADV_NORMAL

Shouldn't these two (or at least QEMU_MADV_DONTFORK) be defined to
QEMU_MADV_INVALID, so KVM check will fail as it should?

Otherwise I'm happy with the patch.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH v5] Introduce qemu_madvise()
  2010-09-14 20:36                   ` [Qemu-devel] " Blue Swirl
@ 2010-09-14 20:39                     ` Andreas Färber
  2010-09-15 18:09                       ` [Qemu-devel] [PATCH v6] " Andreas Färber
  0 siblings, 1 reply; 28+ messages in thread
From: Andreas Färber @ 2010-09-14 20:39 UTC (permalink / raw)
  To: Blue Swirl; +Cc: qemu-devel, Alexander Graf

Am 14.09.2010 um 22:36 schrieb Blue Swirl:

> On Tue, Sep 14, 2010 at 8:28 PM, Andreas Färber <andreas.faerber@web.de 
> > wrote:
>> diff --git a/osdep.h b/osdep.h
>> index 1cdc7e2..6fb4ff3 100644
>> --- a/osdep.h
>> +++ b/osdep.h
>> @@ -90,6 +90,41 @@ void *qemu_memalign(size_t alignment, size_t  
>> size);
>>  void *qemu_vmalloc(size_t size);
>>  void qemu_vfree(void *ptr);
>>
>> +#define QEMU_MADV_INVALID -1
>> +
>> +#if defined(CONFIG_MADVISE)
>> +
>> +#define QEMU_MADV_WILLNEED  MADV_WILLNEED
>> +#define QEMU_MADV_DONTNEED  MADV_DONTNEED
>> +#ifdef MADV_DONTFORK
>> +#define QEMU_MADV_DONTFORK  MADV_DONTFORK
>> +#else
>> +#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
>> +#endif
>> +#ifdef MADV_MERGEABLE
>> +#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
>> +#else
>> +#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
>> +#endif
>> +
>> +#elif defined(CONFIG_POSIX_MADVISE)
>> +
>> +#define QEMU_MADV_WILLNEED  POSIX_MADV_WILLNEED
>> +#define QEMU_MADV_DONTNEED  POSIX_MADV_DONTNEED
>> +#define QEMU_MADV_DONTFORK  POSIX_MADV_NORMAL
>> +#define QEMU_MADV_MERGEABLE POSIX_MADV_NORMAL
>
> Shouldn't these two (or at least QEMU_MADV_DONTFORK) be defined to
> QEMU_MADV_INVALID, so KVM check will fail as it should?

Oops, missed these two. Will fix tomorrow.

Thanks,
Andreas

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] [PATCH v6] Introduce qemu_madvise()
  2010-09-14 20:39                     ` Andreas Färber
@ 2010-09-15 18:09                       ` Andreas Färber
  2010-09-15 19:00                         ` [Qemu-devel] " Blue Swirl
  0 siblings, 1 reply; 28+ messages in thread
From: Andreas Färber @ 2010-09-15 18:09 UTC (permalink / raw)
  To: qemu-devel; +Cc: Blue Swirl, Andrea Arcangeli, Alexander Graf

From: Andreas Färber <afaerber@opensolaris.org>

vl.c has a Sun-specific hack to supply a prototype for madvise(),
but the call site has apparently moved to arch_init.c.

Haiku doesn't implement madvise() in favor of posix_madvise().
OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().

Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
Convert all callers to use qemu_madvise() and QEMU_MADV_*.

Note that on Solaris the warning is fixed by moving the madvise() prototype,
not by qemu_madvise() itself. It will help with future porting though, and
it simplifies most call sites.

v5 -> v6:
* Replace two leftover instances of POSIX_MADV_NORMAL with QEMU_MADV_INVALID.
  Spotted by Blue Swirl.

v4 -> v5:
* Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
  Note that this relies on -1 not being a valid advice value.

v3 -> v4:
* Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
  This will currently break the check in kvm-all.c by calling madvise() with
  a supported flag, which will not fail. Ideas/patches welcome.

v2 -> v3:
* Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
* Add configure check for madvise(), too.
  Add defines to Makefile, not QEMU_CFLAGS.
  Convert all callers, untested. Suggested by Blue Swirl.
* Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
* Display configure check results.

v1 -> v2:
* Don't rely on posix_madvise() availability, add qemu_madvise().
  Suggested by Blue Swirl.

Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Andrea Arcangeli <aarcange@redhat.com>
---
 arch_init.c         |    2 +-
 configure           |   33 +++++++++++++++++++++++++++++++++
 exec.c              |    8 ++------
 hw/virtio-balloon.c |    4 ++--
 kvm-all.c           |   15 +++++++--------
 osdep.c             |   18 ++++++++++++++++++
 osdep.h             |   35 +++++++++++++++++++++++++++++++++++
 vl.c                |    3 ---
 8 files changed, 98 insertions(+), 20 deletions(-)

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);
             }
 #endif
         } else if (flags & RAM_SAVE_FLAG_PAGE) {
diff --git a/configure b/configure
index 4061cb7..86558eb 100755
--- a/configure
+++ b/configure
@@ -2069,6 +2069,31 @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
+# check if we have madvise
+
+madvise=no
+cat > $TMPC << EOF
+#include <sys/types.h>
+#include <sys/mman.h>
+int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    madvise=yes
+fi
+
+##########################################
+# check if we have posix_madvise
+
+posix_madvise=no
+cat > $TMPC << EOF
+#include <sys/mman.h>
+int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    posix_madvise=yes
+fi
+
+##########################################
 # check if trace backend exists
 
 sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
@@ -2226,6 +2251,8 @@ echo "KVM support       $kvm"
 echo "fdt support       $fdt"
 echo "preadv support    $preadv"
 echo "fdatasync         $fdatasync"
+echo "madvise           $madvise"
+echo "posix_madvise     $posix_madvise"
 echo "uuid support      $uuid"
 echo "vhost-net support $vhost_net"
 echo "Trace backend     $trace_backend"
@@ -2466,6 +2493,12 @@ fi
 if test "$fdatasync" = "yes" ; then
   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
 fi
+if test "$madvise" = "yes" ; then
+  echo "CONFIG_MADVISE=y" >> $config_host_mak
+fi
+if test "$posix_madvise" = "yes" ; then
+  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
+fi
 
 # XXX: suppress that
 if [ "$bsd" = "yes" ] ; then
diff --git a/exec.c b/exec.c
index 380dab5..9b5464f 100644
--- a/exec.c
+++ b/exec.c
@@ -2841,9 +2841,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
             new_block->host = file_ram_alloc(new_block, size, mem_path);
             if (!new_block->host) {
                 new_block->host = qemu_vmalloc(size);
-#ifdef MADV_MERGEABLE
-                madvise(new_block->host, size, MADV_MERGEABLE);
-#endif
+                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
             }
 #else
             fprintf(stderr, "-mem-path option unsupported\n");
@@ -2858,9 +2856,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
 #else
             new_block->host = qemu_vmalloc(size);
 #endif
-#ifdef MADV_MERGEABLE
-            madvise(new_block->host, size, MADV_MERGEABLE);
-#endif
+            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
         }
     }
 
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 9fe3886..1e74674 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
 {
 #if defined(__linux__)
     if (!kvm_enabled() || kvm_has_sync_mmu())
-        madvise(addr, TARGET_PAGE_SIZE,
-                deflate ? MADV_WILLNEED : MADV_DONTNEED);
+        qemu_madvise(addr, TARGET_PAGE_SIZE,
+                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
 #endif
 }
 
diff --git a/kvm-all.c b/kvm-all.c
index 58b0404..f7c1d12 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1031,18 +1031,17 @@ int kvm_has_xcrs(void)
 void kvm_setup_guest_memory(void *start, size_t size)
 {
     if (!kvm_has_sync_mmu()) {
-#ifdef MADV_DONTFORK
-        int ret = madvise(start, size, MADV_DONTFORK);
+        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
 
+        if (ret == -ENOTSUP) {
+            fprintf(stderr,
+                    "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
+            exit(1);
+        }
         if (ret) {
-            perror("madvice");
+            perror("qemu_madvise");
             exit(1);
         }
-#else
-        fprintf(stderr,
-                "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
-        exit(1);
-#endif
     }
 }
 
diff --git a/osdep.c b/osdep.c
index 30426ff..1724a58 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"
@@ -35,6 +36,9 @@
 #ifdef CONFIG_SOLARIS
 #include <sys/types.h>
 #include <sys/statvfs.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);
 #endif
 
 #ifdef CONFIG_EVENTFD
@@ -139,6 +143,20 @@ void qemu_vfree(void *ptr)
 
 #endif
 
+int qemu_madvise(void *addr, size_t len, int advice)
+{
+    if (advice == QEMU_MADV_INVALID) {
+        return -ENOTSUP;
+    }
+#if defined(CONFIG_MADVISE)
+    return madvise(addr, len, advice);
+#elif defined(CONFIG_POSIX_MADVISE)
+    return posix_madvise(addr, len, advice);
+#else
+    return -ENOTSUP;
+#endif
+}
+
 int qemu_create_pidfile(const char *filename)
 {
     char buffer[128];
diff --git a/osdep.h b/osdep.h
index 1cdc7e2..6716281 100644
--- a/osdep.h
+++ b/osdep.h
@@ -90,6 +90,41 @@ void *qemu_memalign(size_t alignment, size_t size);
 void *qemu_vmalloc(size_t size);
 void qemu_vfree(void *ptr);
 
+#define QEMU_MADV_INVALID -1
+
+#if defined(CONFIG_MADVISE)
+
+#define QEMU_MADV_WILLNEED  MADV_WILLNEED
+#define QEMU_MADV_DONTNEED  MADV_DONTNEED
+#ifdef MADV_DONTFORK
+#define QEMU_MADV_DONTFORK  MADV_DONTFORK
+#else
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#endif
+#ifdef MADV_MERGEABLE
+#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
+#else
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+#endif
+
+#elif defined(CONFIG_POSIX_MADVISE)
+
+#define QEMU_MADV_WILLNEED  POSIX_MADV_WILLNEED
+#define QEMU_MADV_DONTNEED  POSIX_MADV_DONTNEED
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+
+#else /* no-op */
+
+#define QEMU_MADV_WILLNEED  QEMU_MADV_INVALID
+#define QEMU_MADV_DONTNEED  QEMU_MADV_INVALID
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+
+#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);
 #endif
 #endif
 #endif
-- 
1.7.2.2

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH v6] Introduce qemu_madvise()
  2010-09-15 18:09                       ` [Qemu-devel] [PATCH v6] " Andreas Färber
@ 2010-09-15 19:00                         ` Blue Swirl
  2010-09-15 19:35                           ` Andreas Färber
  0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2010-09-15 19:00 UTC (permalink / raw)
  To: Andreas Färber; +Cc: Andrea Arcangeli, qemu-devel, Alexander Graf

On Wed, Sep 15, 2010 at 6:09 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> From: Andreas Färber <afaerber@opensolaris.org>
>
> vl.c has a Sun-specific hack to supply a prototype for madvise(),
> but the call site has apparently moved to arch_init.c.
>
> Haiku doesn't implement madvise() in favor of posix_madvise().
> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>
> Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
> as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
> Convert all callers to use qemu_madvise() and QEMU_MADV_*.
>
> Note that on Solaris the warning is fixed by moving the madvise() prototype,
> not by qemu_madvise() itself. It will help with future porting though, and
> it simplifies most call sites.
>
> v5 -> v6:
> * Replace two leftover instances of POSIX_MADV_NORMAL with QEMU_MADV_INVALID.
>  Spotted by Blue Swirl.
>
> v4 -> v5:
> * Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
>  Note that this relies on -1 not being a valid advice value.
>
> v3 -> v4:
> * Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
>  This will currently break the check in kvm-all.c by calling madvise() with
>  a supported flag, which will not fail. Ideas/patches welcome.
>
> v2 -> v3:
> * Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
> * Add configure check for madvise(), too.
>  Add defines to Makefile, not QEMU_CFLAGS.
>  Convert all callers, untested. Suggested by Blue Swirl.
> * Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
> * Display configure check results.
>
> v1 -> v2:
> * Don't rely on posix_madvise() availability, add qemu_madvise().
>  Suggested by Blue Swirl.
>
> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  arch_init.c         |    2 +-
>  configure           |   33 +++++++++++++++++++++++++++++++++
>  exec.c              |    8 ++------
>  hw/virtio-balloon.c |    4 ++--
>  kvm-all.c           |   15 +++++++--------
>  osdep.c             |   18 ++++++++++++++++++
>  osdep.h             |   35 +++++++++++++++++++++++++++++++++++
>  vl.c                |    3 ---
>  8 files changed, 98 insertions(+), 20 deletions(-)
>
> 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);
>             }
>  #endif
>         } else if (flags & RAM_SAVE_FLAG_PAGE) {
> diff --git a/configure b/configure
> index 4061cb7..86558eb 100755
> --- a/configure
> +++ b/configure
> @@ -2069,6 +2069,31 @@ if compile_prog "" "" ; then
>  fi
>
>  ##########################################
> +# check if we have madvise
> +
> +madvise=no
> +cat > $TMPC << EOF
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    madvise=yes
> +fi
> +
> +##########################################
> +# check if we have posix_madvise
> +
> +posix_madvise=no
> +cat > $TMPC << EOF
> +#include <sys/mman.h>
> +int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    posix_madvise=yes
> +fi
> +
> +##########################################
>  # check if trace backend exists
>
>  sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
> @@ -2226,6 +2251,8 @@ echo "KVM support       $kvm"
>  echo "fdt support       $fdt"
>  echo "preadv support    $preadv"
>  echo "fdatasync         $fdatasync"
> +echo "madvise           $madvise"
> +echo "posix_madvise     $posix_madvise"
>  echo "uuid support      $uuid"
>  echo "vhost-net support $vhost_net"
>  echo "Trace backend     $trace_backend"
> @@ -2466,6 +2493,12 @@ fi
>  if test "$fdatasync" = "yes" ; then
>   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
>  fi
> +if test "$madvise" = "yes" ; then
> +  echo "CONFIG_MADVISE=y" >> $config_host_mak
> +fi
> +if test "$posix_madvise" = "yes" ; then
> +  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
> +fi
>
>  # XXX: suppress that
>  if [ "$bsd" = "yes" ] ; then
> diff --git a/exec.c b/exec.c
> index 380dab5..9b5464f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2841,9 +2841,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>             new_block->host = file_ram_alloc(new_block, size, mem_path);
>             if (!new_block->host) {
>                 new_block->host = qemu_vmalloc(size);
> -#ifdef MADV_MERGEABLE
> -                madvise(new_block->host, size, MADV_MERGEABLE);
> -#endif
> +                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>             }
>  #else
>             fprintf(stderr, "-mem-path option unsupported\n");
> @@ -2858,9 +2856,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>  #else
>             new_block->host = qemu_vmalloc(size);
>  #endif
> -#ifdef MADV_MERGEABLE
> -            madvise(new_block->host, size, MADV_MERGEABLE);
> -#endif
> +            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>         }
>     }
>
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 9fe3886..1e74674 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
>  {
>  #if defined(__linux__)
>     if (!kvm_enabled() || kvm_has_sync_mmu())
> -        madvise(addr, TARGET_PAGE_SIZE,
> -                deflate ? MADV_WILLNEED : MADV_DONTNEED);
> +        qemu_madvise(addr, TARGET_PAGE_SIZE,
> +                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
>  #endif
>  }
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 58b0404..f7c1d12 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1031,18 +1031,17 @@ int kvm_has_xcrs(void)
>  void kvm_setup_guest_memory(void *start, size_t size)
>  {
>     if (!kvm_has_sync_mmu()) {
> -#ifdef MADV_DONTFORK
> -        int ret = madvise(start, size, MADV_DONTFORK);
> +        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
>
> +        if (ret == -ENOTSUP) {

Sorry, I should have noticed this earlier, but madvise() actually
returns 0 or -1 with error code in errno. Should we try to match that?

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH v6] Introduce qemu_madvise()
  2010-09-15 19:00                         ` [Qemu-devel] " Blue Swirl
@ 2010-09-15 19:35                           ` Andreas Färber
  2010-09-15 19:50                             ` Blue Swirl
  0 siblings, 1 reply; 28+ messages in thread
From: Andreas Färber @ 2010-09-15 19:35 UTC (permalink / raw)
  To: Blue Swirl; +Cc: Andrea Arcangeli, qemu-devel, Alexander Graf

Am 15.09.2010 um 21:00 schrieb Blue Swirl:

> madvise() actually
> returns 0 or -1 with error code in errno. Should we try to match that?

posix_madvise() doesn't seem to... "otherwise, an error number shall  
be returned to indicate the error"

It documents EINVAL for invalid advice, and so does madvise. Are we  
using ENOTSUP knowingly?

Maybe return -1 and set errno to the posix_madvise() return code? (but  
is it errno = -retval or errno = retval then?)

Andreas

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH v6] Introduce qemu_madvise()
  2010-09-15 19:35                           ` Andreas Färber
@ 2010-09-15 19:50                             ` Blue Swirl
  2010-09-15 20:07                               ` Andreas Färber
  2010-09-19 10:11                               ` [Qemu-devel] [PATCH v7] " Andreas Färber
  0 siblings, 2 replies; 28+ messages in thread
From: Blue Swirl @ 2010-09-15 19:50 UTC (permalink / raw)
  To: Andreas Färber; +Cc: Andrea Arcangeli, qemu-devel, Alexander Graf

On Wed, Sep 15, 2010 at 7:35 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 15.09.2010 um 21:00 schrieb Blue Swirl:
>
>> madvise() actually
>> returns 0 or -1 with error code in errno. Should we try to match that?
>
> posix_madvise() doesn't seem to... "otherwise, an error number shall be
> returned to indicate the error"

If we match posix_madvise(), then the wrapper function probably should
become qemu_posix_madvise() and we should prefer posix_madvise() over
madvise().

> It documents EINVAL for invalid advice, and so does madvise. Are we using
> ENOTSUP knowingly?

Let's use EINVAL.

> Maybe return -1 and set errno to the posix_madvise() return code? (but is it
> errno = -retval or errno = retval then?)

An advantage of posix_madvise() semantics is that no mucking with
errno is needed. Though existing code should be checked if they rely
on madvise() semantics instead.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH v6] Introduce qemu_madvise()
  2010-09-15 19:50                             ` Blue Swirl
@ 2010-09-15 20:07                               ` Andreas Färber
  2010-09-19 10:11                               ` [Qemu-devel] [PATCH v7] " Andreas Färber
  1 sibling, 0 replies; 28+ messages in thread
From: Andreas Färber @ 2010-09-15 20:07 UTC (permalink / raw)
  To: Blue Swirl; +Cc: Andrea Arcangeli, qemu-devel, Alexander Graf


Am 15.09.2010 um 21:50 schrieb Blue Swirl:

> On Wed, Sep 15, 2010 at 7:35 PM, Andreas Färber <andreas.faerber@web.de 
> > wrote:
>> Maybe return -1 and set errno to the posix_madvise() return code?  
>> (but is it
>> errno = -retval or errno = retval then?)
>
> An advantage of posix_madvise() semantics is that no mucking with
> errno is needed. Though existing code should be checked if they rely
> on madvise() semantics instead.

perror() after the MADV_DONTFORK failure does, right?

Andreas

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] [PATCH v7] Introduce qemu_madvise()
  2010-09-15 19:50                             ` Blue Swirl
  2010-09-15 20:07                               ` Andreas Färber
@ 2010-09-19 10:11                               ` Andreas Färber
  2010-09-20 20:33                                 ` [Qemu-devel] " Blue Swirl
  1 sibling, 1 reply; 28+ messages in thread
From: Andreas Färber @ 2010-09-19 10:11 UTC (permalink / raw)
  To: qemu-devel; +Cc: Blue Swirl, Andrea Arcangeli, Alexander Graf

From: Andreas Färber <afaerber@opensolaris.org>

vl.c has a Sun-specific hack to supply a prototype for madvise(),
but the call site has apparently moved to arch_init.c.

Haiku doesn't implement madvise() in favor of posix_madvise().
OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().

Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
Convert all callers to use qemu_madvise() and QEMU_MADV_*.

Note that on Solaris the warning is fixed by moving the madvise() prototype,
not by qemu_madvise() itself. It helps with porting though, and it simplifies
most call sites.

v6 -> v7:
* Adopt madvise() rather than posix_madvise() semantics for returning errors.
* Use EINVAL in place of ENOTSUP.

v5 -> v6:
* Replace two leftover instances of POSIX_MADV_NORMAL with QEMU_MADV_INVALID.
  Spotted by Blue Swirl.

v4 -> v5:
* Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
  Note that this relies on -1 not being a valid advice value.

v3 -> v4:
* Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
  This will currently break the check in kvm-all.c by calling madvise() with
  a supported flag, which will not fail. Ideas/patches welcome.

v2 -> v3:
* Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
* Add configure check for madvise(), too.
  Add defines to Makefile, not QEMU_CFLAGS.
  Convert all callers, untested. Suggested by Blue Swirl.
* Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
* Display configure check results.

v1 -> v2:
* Don't rely on posix_madvise() availability, add qemu_madvise().
  Suggested by Blue Swirl.

Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Andrea Arcangeli <aarcange@redhat.com>
---
 arch_init.c         |    2 +-
 configure           |   33 +++++++++++++++++++++++++++++++++
 exec.c              |    8 ++------
 hw/virtio-balloon.c |    4 ++--
 kvm-all.c           |   12 ++++--------
 osdep.c             |   20 ++++++++++++++++++++
 osdep.h             |   35 +++++++++++++++++++++++++++++++++++
 vl.c                |    3 ---
 8 files changed, 97 insertions(+), 20 deletions(-)

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);
             }
 #endif
         } else if (flags & RAM_SAVE_FLAG_PAGE) {
diff --git a/configure b/configure
index 3bfc5e9..6a21bf2 100755
--- a/configure
+++ b/configure
@@ -2072,6 +2072,31 @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
+# check if we have madvise
+
+madvise=no
+cat > $TMPC << EOF
+#include <sys/types.h>
+#include <sys/mman.h>
+int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    madvise=yes
+fi
+
+##########################################
+# check if we have posix_madvise
+
+posix_madvise=no
+cat > $TMPC << EOF
+#include <sys/mman.h>
+int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    posix_madvise=yes
+fi
+
+##########################################
 # check if trace backend exists
 
 sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
@@ -2238,6 +2263,8 @@ echo "KVM support       $kvm"
 echo "fdt support       $fdt"
 echo "preadv support    $preadv"
 echo "fdatasync         $fdatasync"
+echo "madvise           $madvise"
+echo "posix_madvise     $posix_madvise"
 echo "uuid support      $uuid"
 echo "vhost-net support $vhost_net"
 echo "Trace backend     $trace_backend"
@@ -2478,6 +2505,12 @@ fi
 if test "$fdatasync" = "yes" ; then
   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
 fi
+if test "$madvise" = "yes" ; then
+  echo "CONFIG_MADVISE=y" >> $config_host_mak
+fi
+if test "$posix_madvise" = "yes" ; then
+  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
+fi
 
 # XXX: suppress that
 if [ "$bsd" = "yes" ] ; then
diff --git a/exec.c b/exec.c
index 380dab5..9b5464f 100644
--- a/exec.c
+++ b/exec.c
@@ -2841,9 +2841,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
             new_block->host = file_ram_alloc(new_block, size, mem_path);
             if (!new_block->host) {
                 new_block->host = qemu_vmalloc(size);
-#ifdef MADV_MERGEABLE
-                madvise(new_block->host, size, MADV_MERGEABLE);
-#endif
+                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
             }
 #else
             fprintf(stderr, "-mem-path option unsupported\n");
@@ -2858,9 +2856,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
 #else
             new_block->host = qemu_vmalloc(size);
 #endif
-#ifdef MADV_MERGEABLE
-            madvise(new_block->host, size, MADV_MERGEABLE);
-#endif
+            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
         }
     }
 
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 9fe3886..1e74674 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
 {
 #if defined(__linux__)
     if (!kvm_enabled() || kvm_has_sync_mmu())
-        madvise(addr, TARGET_PAGE_SIZE,
-                deflate ? MADV_WILLNEED : MADV_DONTNEED);
+        qemu_madvise(addr, TARGET_PAGE_SIZE,
+                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
 #endif
 }
 
diff --git a/kvm-all.c b/kvm-all.c
index 58b0404..1cc696f 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1031,18 +1031,14 @@ int kvm_has_xcrs(void)
 void kvm_setup_guest_memory(void *start, size_t size)
 {
     if (!kvm_has_sync_mmu()) {
-#ifdef MADV_DONTFORK
-        int ret = madvise(start, size, MADV_DONTFORK);
+        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
 
         if (ret) {
-            perror("madvice");
+            perror("qemu_madvise");
+            fprintf(stderr,
+                    "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
             exit(1);
         }
-#else
-        fprintf(stderr,
-                "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
-        exit(1);
-#endif
     }
 }
 
diff --git a/osdep.c b/osdep.c
index 30426ff..56c6944 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"
@@ -35,6 +36,9 @@
 #ifdef CONFIG_SOLARIS
 #include <sys/types.h>
 #include <sys/statvfs.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);
 #endif
 
 #ifdef CONFIG_EVENTFD
@@ -139,6 +143,22 @@ void qemu_vfree(void *ptr)
 
 #endif
 
+int qemu_madvise(void *addr, size_t len, int advice)
+{
+    if (advice == QEMU_MADV_INVALID) {
+        errno = EINVAL;
+        return -1;
+    }
+#if defined(CONFIG_MADVISE)
+    return madvise(addr, len, advice);
+#elif defined(CONFIG_POSIX_MADVISE)
+    return posix_madvise(addr, len, advice);
+#else
+    errno = EINVAL;
+    return -1;
+#endif
+}
+
 int qemu_create_pidfile(const char *filename)
 {
     char buffer[128];
diff --git a/osdep.h b/osdep.h
index 1cdc7e2..6716281 100644
--- a/osdep.h
+++ b/osdep.h
@@ -90,6 +90,41 @@ void *qemu_memalign(size_t alignment, size_t size);
 void *qemu_vmalloc(size_t size);
 void qemu_vfree(void *ptr);
 
+#define QEMU_MADV_INVALID -1
+
+#if defined(CONFIG_MADVISE)
+
+#define QEMU_MADV_WILLNEED  MADV_WILLNEED
+#define QEMU_MADV_DONTNEED  MADV_DONTNEED
+#ifdef MADV_DONTFORK
+#define QEMU_MADV_DONTFORK  MADV_DONTFORK
+#else
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#endif
+#ifdef MADV_MERGEABLE
+#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
+#else
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+#endif
+
+#elif defined(CONFIG_POSIX_MADVISE)
+
+#define QEMU_MADV_WILLNEED  POSIX_MADV_WILLNEED
+#define QEMU_MADV_DONTNEED  POSIX_MADV_DONTNEED
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+
+#else /* no-op */
+
+#define QEMU_MADV_WILLNEED  QEMU_MADV_INVALID
+#define QEMU_MADV_DONTNEED  QEMU_MADV_INVALID
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+
+#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);
 #endif
 #endif
 #endif
-- 
1.7.2.2

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH v7] Introduce qemu_madvise()
  2010-09-19 10:11                               ` [Qemu-devel] [PATCH v7] " Andreas Färber
@ 2010-09-20 20:33                                 ` Blue Swirl
  2010-09-24 18:08                                   ` Andreas Färber
  0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2010-09-20 20:33 UTC (permalink / raw)
  To: Andreas Färber; +Cc: Andrea Arcangeli, qemu-devel, Alexander Graf

On Sun, Sep 19, 2010 at 10:11 AM, Andreas Färber <andreas.faerber@web.de> wrote:
> From: Andreas Färber <afaerber@opensolaris.org>
>
> vl.c has a Sun-specific hack to supply a prototype for madvise(),
> but the call site has apparently moved to arch_init.c.
>
> Haiku doesn't implement madvise() in favor of posix_madvise().
> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>
> Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
> as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
> Convert all callers to use qemu_madvise() and QEMU_MADV_*.
>
> Note that on Solaris the warning is fixed by moving the madvise() prototype,
> not by qemu_madvise() itself. It helps with porting though, and it simplifies
> most call sites.
>
> v6 -> v7:
> * Adopt madvise() rather than posix_madvise() semantics for returning errors.
> * Use EINVAL in place of ENOTSUP.
>
> v5 -> v6:
> * Replace two leftover instances of POSIX_MADV_NORMAL with QEMU_MADV_INVALID.
>  Spotted by Blue Swirl.
>
> v4 -> v5:
> * Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
>  Note that this relies on -1 not being a valid advice value.
>
> v3 -> v4:
> * Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
>  This will currently break the check in kvm-all.c by calling madvise() with
>  a supported flag, which will not fail. Ideas/patches welcome.
>
> v2 -> v3:
> * Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
> * Add configure check for madvise(), too.
>  Add defines to Makefile, not QEMU_CFLAGS.
>  Convert all callers, untested. Suggested by Blue Swirl.
> * Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
> * Display configure check results.
>
> v1 -> v2:
> * Don't rely on posix_madvise() availability, add qemu_madvise().
>  Suggested by Blue Swirl.
>
> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  arch_init.c         |    2 +-
>  configure           |   33 +++++++++++++++++++++++++++++++++
>  exec.c              |    8 ++------
>  hw/virtio-balloon.c |    4 ++--
>  kvm-all.c           |   12 ++++--------
>  osdep.c             |   20 ++++++++++++++++++++
>  osdep.h             |   35 +++++++++++++++++++++++++++++++++++
>  vl.c                |    3 ---
>  8 files changed, 97 insertions(+), 20 deletions(-)
>
> 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);
>             }
>  #endif
>         } else if (flags & RAM_SAVE_FLAG_PAGE) {
> diff --git a/configure b/configure
> index 3bfc5e9..6a21bf2 100755
> --- a/configure
> +++ b/configure
> @@ -2072,6 +2072,31 @@ if compile_prog "" "" ; then
>  fi
>
>  ##########################################
> +# check if we have madvise
> +
> +madvise=no
> +cat > $TMPC << EOF
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    madvise=yes
> +fi
> +
> +##########################################
> +# check if we have posix_madvise
> +
> +posix_madvise=no
> +cat > $TMPC << EOF
> +#include <sys/mman.h>
> +int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    posix_madvise=yes
> +fi
> +
> +##########################################
>  # check if trace backend exists
>
>  sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
> @@ -2238,6 +2263,8 @@ echo "KVM support       $kvm"
>  echo "fdt support       $fdt"
>  echo "preadv support    $preadv"
>  echo "fdatasync         $fdatasync"
> +echo "madvise           $madvise"
> +echo "posix_madvise     $posix_madvise"
>  echo "uuid support      $uuid"
>  echo "vhost-net support $vhost_net"
>  echo "Trace backend     $trace_backend"
> @@ -2478,6 +2505,12 @@ fi
>  if test "$fdatasync" = "yes" ; then
>   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
>  fi
> +if test "$madvise" = "yes" ; then
> +  echo "CONFIG_MADVISE=y" >> $config_host_mak
> +fi
> +if test "$posix_madvise" = "yes" ; then
> +  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
> +fi
>
>  # XXX: suppress that
>  if [ "$bsd" = "yes" ] ; then
> diff --git a/exec.c b/exec.c
> index 380dab5..9b5464f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2841,9 +2841,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>             new_block->host = file_ram_alloc(new_block, size, mem_path);
>             if (!new_block->host) {
>                 new_block->host = qemu_vmalloc(size);
> -#ifdef MADV_MERGEABLE
> -                madvise(new_block->host, size, MADV_MERGEABLE);
> -#endif
> +                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>             }
>  #else
>             fprintf(stderr, "-mem-path option unsupported\n");
> @@ -2858,9 +2856,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>  #else
>             new_block->host = qemu_vmalloc(size);
>  #endif
> -#ifdef MADV_MERGEABLE
> -            madvise(new_block->host, size, MADV_MERGEABLE);
> -#endif
> +            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>         }
>     }
>
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 9fe3886..1e74674 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
>  {
>  #if defined(__linux__)
>     if (!kvm_enabled() || kvm_has_sync_mmu())
> -        madvise(addr, TARGET_PAGE_SIZE,
> -                deflate ? MADV_WILLNEED : MADV_DONTNEED);
> +        qemu_madvise(addr, TARGET_PAGE_SIZE,
> +                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
>  #endif
>  }
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 58b0404..1cc696f 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1031,18 +1031,14 @@ int kvm_has_xcrs(void)
>  void kvm_setup_guest_memory(void *start, size_t size)
>  {
>     if (!kvm_has_sync_mmu()) {
> -#ifdef MADV_DONTFORK
> -        int ret = madvise(start, size, MADV_DONTFORK);
> +        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
>
>         if (ret) {
> -            perror("madvice");
> +            perror("qemu_madvise");
> +            fprintf(stderr,
> +                    "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
>             exit(1);
>         }
> -#else
> -        fprintf(stderr,
> -                "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
> -        exit(1);
> -#endif
>     }
>  }
>
> diff --git a/osdep.c b/osdep.c
> index 30426ff..56c6944 100644
> --- a/osdep.c
> +++ b/osdep.c
> @@ -28,6 +28,7 @@
>  #include <errno.h>
>  #include <unistd.h>
>  #include <fcntl.h>
> +#include <sys/mman.h>

With the patch applied, I get a warning here with mingw from Debian stable:
  CC    osdep.o
/src/qemu/osdep.c:31:22: error: sys/mman.h: No such file or directory

For some reason, it doesn't happen with newer mingw from Debian testing.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] Re: [PATCH v7] Introduce qemu_madvise()
  2010-09-20 20:33                                 ` [Qemu-devel] " Blue Swirl
@ 2010-09-24 18:08                                   ` Andreas Färber
  2010-09-25  7:49                                     ` Blue Swirl
  0 siblings, 1 reply; 28+ messages in thread
From: Andreas Färber @ 2010-09-24 18:08 UTC (permalink / raw)
  To: Blue Swirl; +Cc: Andrea Arcangeli, qemu-devel, Alexander Graf

Am 20.09.2010 um 22:33 schrieb Blue Swirl:

> On Sun, Sep 19, 2010 at 10:11 AM, Andreas Färber <andreas.faerber@web.de 
> > wrote:
>> From: Andreas Färber <afaerber@opensolaris.org>
>>
>> vl.c has a Sun-specific hack to supply a prototype for madvise(),
>> but the call site has apparently moved to arch_init.c.
>>
>> Haiku doesn't implement madvise() in favor of posix_madvise().
>> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>>
>> Check for madvise() and posix_madvise() in configure and supply  
>> qemu_madvise()
>> as wrapper. Prefer madvise() over posix_madvise() due to flag  
>> availability.
>> Convert all callers to use qemu_madvise() and QEMU_MADV_*.
>>
>> Note that on Solaris the warning is fixed by moving the madvise()  
>> prototype,
>> not by qemu_madvise() itself. It helps with porting though, and it  
>> simplifies
>> most call sites.
>>
>> v6 -> v7:
>> * Adopt madvise() rather than posix_madvise() semantics for  
>> returning errors.
>> * Use EINVAL in place of ENOTSUP.
>>
>> v5 -> v6:
>> * Replace two leftover instances of POSIX_MADV_NORMAL with  
>> QEMU_MADV_INVALID.
>>  Spotted by Blue Swirl.
>>
>> v4 -> v5:
>> * Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
>>  Note that this relies on -1 not being a valid advice value.
>>
>> v3 -> v4:
>> * Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue  
>> Swirl.
>>  This will currently break the check in kvm-all.c by calling  
>> madvise() with
>>  a supported flag, which will not fail. Ideas/patches welcome.
>>
>> v2 -> v3:
>> * Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by  
>> Alexander Graf.
>> * Add configure check for madvise(), too.
>>  Add defines to Makefile, not QEMU_CFLAGS.
>>  Convert all callers, untested. Suggested by Blue Swirl.
>> * Keep Solaris' madvise() prototype around. Pointed out by  
>> Alexander Graf.
>> * Display configure check results.
>>
>> v1 -> v2:
>> * Don't rely on posix_madvise() availability, add qemu_madvise().
>>  Suggested by Blue Swirl.
>>
>> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> Cc: Alexander Graf <agraf@suse.de>
>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>> ---
>>  arch_init.c         |    2 +-
>>  configure           |   33 +++++++++++++++++++++++++++++++++
>>  exec.c              |    8 ++------
>>  hw/virtio-balloon.c |    4 ++--
>>  kvm-all.c           |   12 ++++--------
>>  osdep.c             |   20 ++++++++++++++++++++
>>  osdep.h             |   35 +++++++++++++++++++++++++++++++++++
>>  vl.c                |    3 ---
>>  8 files changed, 97 insertions(+), 20 deletions(-)

>> diff --git a/osdep.c b/osdep.c
>> index 30426ff..56c6944 100644
>> --- a/osdep.c
>> +++ b/osdep.c
>> @@ -28,6 +28,7 @@
>>  #include <errno.h>
>>  #include <unistd.h>
>>  #include <fcntl.h>
>> +#include <sys/mman.h>
>
> With the patch applied, I get a warning here with mingw from Debian  
> stable:
>  CC    osdep.o
> /src/qemu/osdep.c:31:22: error: sys/mman.h: No such file or directory
>
> For some reason, it doesn't happen with newer mingw from Debian  
> testing.

Any suggestions what to do about that? Would it work without, i.e.  
could we enclose it in #ifndef _WIN32?

Andreas

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Qemu-devel] Re: [PATCH v7] Introduce qemu_madvise()
  2010-09-24 18:08                                   ` Andreas Färber
@ 2010-09-25  7:49                                     ` Blue Swirl
  2010-09-25 10:58                                       ` [Qemu-devel] [PATCH v8] " Andreas Färber
  0 siblings, 1 reply; 28+ messages in thread
From: Blue Swirl @ 2010-09-25  7:49 UTC (permalink / raw)
  To: Andreas Färber; +Cc: Andrea Arcangeli, qemu-devel, Alexander Graf

On Fri, Sep 24, 2010 at 6:08 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 20.09.2010 um 22:33 schrieb Blue Swirl:
>
>> On Sun, Sep 19, 2010 at 10:11 AM, Andreas Färber <andreas.faerber@web.de>
>> wrote:
>>>
>>> From: Andreas Färber <afaerber@opensolaris.org>
>>>
>>> vl.c has a Sun-specific hack to supply a prototype for madvise(),
>>> but the call site has apparently moved to arch_init.c.
>>>
>>> Haiku doesn't implement madvise() in favor of posix_madvise().
>>> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
>>>
>>> Check for madvise() and posix_madvise() in configure and supply
>>> qemu_madvise()
>>> as wrapper. Prefer madvise() over posix_madvise() due to flag
>>> availability.
>>> Convert all callers to use qemu_madvise() and QEMU_MADV_*.
>>>
>>> Note that on Solaris the warning is fixed by moving the madvise()
>>> prototype,
>>> not by qemu_madvise() itself. It helps with porting though, and it
>>> simplifies
>>> most call sites.
>>>
>>> v6 -> v7:
>>> * Adopt madvise() rather than posix_madvise() semantics for returning
>>> errors.
>>> * Use EINVAL in place of ENOTSUP.
>>>
>>> v5 -> v6:
>>> * Replace two leftover instances of POSIX_MADV_NORMAL with
>>> QEMU_MADV_INVALID.
>>>  Spotted by Blue Swirl.
>>>
>>> v4 -> v5:
>>> * Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
>>>  Note that this relies on -1 not being a valid advice value.
>>>
>>> v3 -> v4:
>>> * Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
>>>  This will currently break the check in kvm-all.c by calling madvise()
>>> with
>>>  a supported flag, which will not fail. Ideas/patches welcome.
>>>
>>> v2 -> v3:
>>> * Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander
>>> Graf.
>>> * Add configure check for madvise(), too.
>>>  Add defines to Makefile, not QEMU_CFLAGS.
>>>  Convert all callers, untested. Suggested by Blue Swirl.
>>> * Keep Solaris' madvise() prototype around. Pointed out by Alexander
>>> Graf.
>>> * Display configure check results.
>>>
>>> v1 -> v2:
>>> * Don't rely on posix_madvise() availability, add qemu_madvise().
>>>  Suggested by Blue Swirl.
>>>
>>> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
>>> Cc: Blue Swirl <blauwirbel@gmail.com>
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>>> ---
>>>  arch_init.c         |    2 +-
>>>  configure           |   33 +++++++++++++++++++++++++++++++++
>>>  exec.c              |    8 ++------
>>>  hw/virtio-balloon.c |    4 ++--
>>>  kvm-all.c           |   12 ++++--------
>>>  osdep.c             |   20 ++++++++++++++++++++
>>>  osdep.h             |   35 +++++++++++++++++++++++++++++++++++
>>>  vl.c                |    3 ---
>>>  8 files changed, 97 insertions(+), 20 deletions(-)
>
>>> diff --git a/osdep.c b/osdep.c
>>> index 30426ff..56c6944 100644
>>> --- a/osdep.c
>>> +++ b/osdep.c
>>> @@ -28,6 +28,7 @@
>>>  #include <errno.h>
>>>  #include <unistd.h>
>>>  #include <fcntl.h>
>>> +#include <sys/mman.h>
>>
>> With the patch applied, I get a warning here with mingw from Debian
>> stable:
>>  CC    osdep.o
>> /src/qemu/osdep.c:31:22: error: sys/mman.h: No such file or directory
>>
>> For some reason, it doesn't happen with newer mingw from Debian testing.
>
> Any suggestions what to do about that? Would it work without, i.e. could we
> enclose it in #ifndef _WIN32?

There's no madvise in mingw, so either that or #if
defined(CONFIG_MADVISE) || defined(CONFIG_POSIX_MADVISE).

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] [PATCH v8] Introduce qemu_madvise()
  2010-09-25  7:49                                     ` Blue Swirl
@ 2010-09-25 10:58                                       ` Andreas Färber
  2010-09-25 15:17                                         ` [Qemu-devel] " Blue Swirl
  0 siblings, 1 reply; 28+ messages in thread
From: Andreas Färber @ 2010-09-25 10:58 UTC (permalink / raw)
  To: qemu-devel; +Cc: Blue Swirl, Andrea Arcangeli, Alexander Graf

From: Andreas Färber <afaerber@opensolaris.org>

vl.c has a Sun-specific hack to supply a prototype for madvise(),
but the call site has apparently moved to arch_init.c.

Haiku doesn't implement madvise() in favor of posix_madvise().
OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
MinGW implements neither.

Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
Convert all callers to use qemu_madvise() and QEMU_MADV_*.

Note that on Solaris the warning is fixed by moving the madvise() prototype,
not by qemu_madvise() itself. It helps with porting though, and it simplifies
most call sites.

v7 -> v8:
* Some versions of MinGW have no sys/mman.h header. Reported by Blue Swirl.

v6 -> v7:
* Adopt madvise() rather than posix_madvise() semantics for returning errors.
* Use EINVAL in place of ENOTSUP.

v5 -> v6:
* Replace two leftover instances of POSIX_MADV_NORMAL with QEMU_MADV_INVALID.
  Spotted by Blue Swirl.

v4 -> v5:
* Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
  Note that this relies on -1 not being a valid advice value.

v3 -> v4:
* Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
  This will currently break the check in kvm-all.c by calling madvise() with
  a supported flag, which will not fail. Ideas/patches welcome.

v2 -> v3:
* Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
* Add configure check for madvise(), too.
  Add defines to Makefile, not QEMU_CFLAGS.
  Convert all callers, untested. Suggested by Blue Swirl.
* Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
* Display configure check results.

v1 -> v2:
* Don't rely on posix_madvise() availability, add qemu_madvise().
  Suggested by Blue Swirl.

Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Andrea Arcangeli <aarcange@redhat.com>
---
 arch_init.c         |    2 +-
 configure           |   33 +++++++++++++++++++++++++++++++++
 exec.c              |    8 ++------
 hw/virtio-balloon.c |    4 ++--
 kvm-all.c           |   12 ++++--------
 osdep.c             |   22 ++++++++++++++++++++++
 osdep.h             |   35 +++++++++++++++++++++++++++++++++++
 vl.c                |    3 ---
 8 files changed, 99 insertions(+), 20 deletions(-)

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);
             }
 #endif
         } else if (flags & RAM_SAVE_FLAG_PAGE) {
diff --git a/configure b/configure
index 3bfc5e9..6a21bf2 100755
--- a/configure
+++ b/configure
@@ -2072,6 +2072,31 @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
+# check if we have madvise
+
+madvise=no
+cat > $TMPC << EOF
+#include <sys/types.h>
+#include <sys/mman.h>
+int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    madvise=yes
+fi
+
+##########################################
+# check if we have posix_madvise
+
+posix_madvise=no
+cat > $TMPC << EOF
+#include <sys/mman.h>
+int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
+EOF
+if compile_prog "" "" ; then
+    posix_madvise=yes
+fi
+
+##########################################
 # check if trace backend exists
 
 sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
@@ -2238,6 +2263,8 @@ echo "KVM support       $kvm"
 echo "fdt support       $fdt"
 echo "preadv support    $preadv"
 echo "fdatasync         $fdatasync"
+echo "madvise           $madvise"
+echo "posix_madvise     $posix_madvise"
 echo "uuid support      $uuid"
 echo "vhost-net support $vhost_net"
 echo "Trace backend     $trace_backend"
@@ -2478,6 +2505,12 @@ fi
 if test "$fdatasync" = "yes" ; then
   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
 fi
+if test "$madvise" = "yes" ; then
+  echo "CONFIG_MADVISE=y" >> $config_host_mak
+fi
+if test "$posix_madvise" = "yes" ; then
+  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
+fi
 
 # XXX: suppress that
 if [ "$bsd" = "yes" ] ; then
diff --git a/exec.c b/exec.c
index 380dab5..9b5464f 100644
--- a/exec.c
+++ b/exec.c
@@ -2841,9 +2841,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
             new_block->host = file_ram_alloc(new_block, size, mem_path);
             if (!new_block->host) {
                 new_block->host = qemu_vmalloc(size);
-#ifdef MADV_MERGEABLE
-                madvise(new_block->host, size, MADV_MERGEABLE);
-#endif
+                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
             }
 #else
             fprintf(stderr, "-mem-path option unsupported\n");
@@ -2858,9 +2856,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
 #else
             new_block->host = qemu_vmalloc(size);
 #endif
-#ifdef MADV_MERGEABLE
-            madvise(new_block->host, size, MADV_MERGEABLE);
-#endif
+            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
         }
     }
 
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 9fe3886..1e74674 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
 {
 #if defined(__linux__)
     if (!kvm_enabled() || kvm_has_sync_mmu())
-        madvise(addr, TARGET_PAGE_SIZE,
-                deflate ? MADV_WILLNEED : MADV_DONTNEED);
+        qemu_madvise(addr, TARGET_PAGE_SIZE,
+                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
 #endif
 }
 
diff --git a/kvm-all.c b/kvm-all.c
index 58b0404..1cc696f 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1031,18 +1031,14 @@ int kvm_has_xcrs(void)
 void kvm_setup_guest_memory(void *start, size_t size)
 {
     if (!kvm_has_sync_mmu()) {
-#ifdef MADV_DONTFORK
-        int ret = madvise(start, size, MADV_DONTFORK);
+        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
 
         if (ret) {
-            perror("madvice");
+            perror("qemu_madvise");
+            fprintf(stderr,
+                    "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
             exit(1);
         }
-#else
-        fprintf(stderr,
-                "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
-        exit(1);
-#endif
     }
 }
 
diff --git a/osdep.c b/osdep.c
index 30426ff..2c53015 100644
--- a/osdep.c
+++ b/osdep.c
@@ -28,6 +28,9 @@
 #include <errno.h>
 #include <unistd.h>
 #include <fcntl.h>
+#if defined(CONFIG_MADVISE) || defined(CONFIG_POSIX_MADVISE)
+#include <sys/mman.h>
+#endif
 
 /* Needed early for CONFIG_BSD etc. */
 #include "config-host.h"
@@ -35,6 +38,9 @@
 #ifdef CONFIG_SOLARIS
 #include <sys/types.h>
 #include <sys/statvfs.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);
 #endif
 
 #ifdef CONFIG_EVENTFD
@@ -139,6 +145,22 @@ void qemu_vfree(void *ptr)
 
 #endif
 
+int qemu_madvise(void *addr, size_t len, int advice)
+{
+    if (advice == QEMU_MADV_INVALID) {
+        errno = EINVAL;
+        return -1;
+    }
+#if defined(CONFIG_MADVISE)
+    return madvise(addr, len, advice);
+#elif defined(CONFIG_POSIX_MADVISE)
+    return posix_madvise(addr, len, advice);
+#else
+    errno = EINVAL;
+    return -1;
+#endif
+}
+
 int qemu_create_pidfile(const char *filename)
 {
     char buffer[128];
diff --git a/osdep.h b/osdep.h
index 1cdc7e2..6716281 100644
--- a/osdep.h
+++ b/osdep.h
@@ -90,6 +90,41 @@ void *qemu_memalign(size_t alignment, size_t size);
 void *qemu_vmalloc(size_t size);
 void qemu_vfree(void *ptr);
 
+#define QEMU_MADV_INVALID -1
+
+#if defined(CONFIG_MADVISE)
+
+#define QEMU_MADV_WILLNEED  MADV_WILLNEED
+#define QEMU_MADV_DONTNEED  MADV_DONTNEED
+#ifdef MADV_DONTFORK
+#define QEMU_MADV_DONTFORK  MADV_DONTFORK
+#else
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#endif
+#ifdef MADV_MERGEABLE
+#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
+#else
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+#endif
+
+#elif defined(CONFIG_POSIX_MADVISE)
+
+#define QEMU_MADV_WILLNEED  POSIX_MADV_WILLNEED
+#define QEMU_MADV_DONTNEED  POSIX_MADV_DONTNEED
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+
+#else /* no-op */
+
+#define QEMU_MADV_WILLNEED  QEMU_MADV_INVALID
+#define QEMU_MADV_DONTNEED  QEMU_MADV_INVALID
+#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
+#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
+
+#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);
 #endif
 #endif
 #endif
-- 
1.7.2.2

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Qemu-devel] Re: [PATCH v8] Introduce qemu_madvise()
  2010-09-25 10:58                                       ` [Qemu-devel] [PATCH v8] " Andreas Färber
@ 2010-09-25 15:17                                         ` Blue Swirl
  0 siblings, 0 replies; 28+ messages in thread
From: Blue Swirl @ 2010-09-25 15:17 UTC (permalink / raw)
  To: Andreas Färber; +Cc: Andrea Arcangeli, qemu-devel, Alexander Graf

Thanks, applied. I moved the include line below #include
"config-host.h", otherwise CONFIG_xx would have been undefined.

On Sat, Sep 25, 2010 at 10:58 AM, Andreas Färber <andreas.faerber@web.de> wrote:
> From: Andreas Färber <afaerber@opensolaris.org>
>
> vl.c has a Sun-specific hack to supply a prototype for madvise(),
> but the call site has apparently moved to arch_init.c.
>
> Haiku doesn't implement madvise() in favor of posix_madvise().
> OpenBSD and Solaris 10 don't implement posix_madvise() but madvise().
> MinGW implements neither.
>
> Check for madvise() and posix_madvise() in configure and supply qemu_madvise()
> as wrapper. Prefer madvise() over posix_madvise() due to flag availability.
> Convert all callers to use qemu_madvise() and QEMU_MADV_*.
>
> Note that on Solaris the warning is fixed by moving the madvise() prototype,
> not by qemu_madvise() itself. It helps with porting though, and it simplifies
> most call sites.
>
> v7 -> v8:
> * Some versions of MinGW have no sys/mman.h header. Reported by Blue Swirl.
>
> v6 -> v7:
> * Adopt madvise() rather than posix_madvise() semantics for returning errors.
> * Use EINVAL in place of ENOTSUP.
>
> v5 -> v6:
> * Replace two leftover instances of POSIX_MADV_NORMAL with QEMU_MADV_INVALID.
>  Spotted by Blue Swirl.
>
> v4 -> v5:
> * Introduce QEMU_MADV_INVALID, suggested by Alexander Graf.
>  Note that this relies on -1 not being a valid advice value.
>
> v3 -> v4:
> * Eliminate #ifdefs at qemu_advise() call sites. Requested by Blue Swirl.
>  This will currently break the check in kvm-all.c by calling madvise() with
>  a supported flag, which will not fail. Ideas/patches welcome.
>
> v2 -> v3:
> * Reuse the *_MADV_* defines for QEMU_MADV_*. Suggested by Alexander Graf.
> * Add configure check for madvise(), too.
>  Add defines to Makefile, not QEMU_CFLAGS.
>  Convert all callers, untested. Suggested by Blue Swirl.
> * Keep Solaris' madvise() prototype around. Pointed out by Alexander Graf.
> * Display configure check results.
>
> v1 -> v2:
> * Don't rely on posix_madvise() availability, add qemu_madvise().
>  Suggested by Blue Swirl.
>
> Signed-off-by: Andreas Färber <afaerber@opensolaris.org>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  arch_init.c         |    2 +-
>  configure           |   33 +++++++++++++++++++++++++++++++++
>  exec.c              |    8 ++------
>  hw/virtio-balloon.c |    4 ++--
>  kvm-all.c           |   12 ++++--------
>  osdep.c             |   22 ++++++++++++++++++++++
>  osdep.h             |   35 +++++++++++++++++++++++++++++++++++
>  vl.c                |    3 ---
>  8 files changed, 99 insertions(+), 20 deletions(-)
>
> 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);
>             }
>  #endif
>         } else if (flags & RAM_SAVE_FLAG_PAGE) {
> diff --git a/configure b/configure
> index 3bfc5e9..6a21bf2 100755
> --- a/configure
> +++ b/configure
> @@ -2072,6 +2072,31 @@ if compile_prog "" "" ; then
>  fi
>
>  ##########################################
> +# check if we have madvise
> +
> +madvise=no
> +cat > $TMPC << EOF
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    madvise=yes
> +fi
> +
> +##########################################
> +# check if we have posix_madvise
> +
> +posix_madvise=no
> +cat > $TMPC << EOF
> +#include <sys/mman.h>
> +int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }
> +EOF
> +if compile_prog "" "" ; then
> +    posix_madvise=yes
> +fi
> +
> +##########################################
>  # check if trace backend exists
>
>  sh "$source_path/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
> @@ -2238,6 +2263,8 @@ echo "KVM support       $kvm"
>  echo "fdt support       $fdt"
>  echo "preadv support    $preadv"
>  echo "fdatasync         $fdatasync"
> +echo "madvise           $madvise"
> +echo "posix_madvise     $posix_madvise"
>  echo "uuid support      $uuid"
>  echo "vhost-net support $vhost_net"
>  echo "Trace backend     $trace_backend"
> @@ -2478,6 +2505,12 @@ fi
>  if test "$fdatasync" = "yes" ; then
>   echo "CONFIG_FDATASYNC=y" >> $config_host_mak
>  fi
> +if test "$madvise" = "yes" ; then
> +  echo "CONFIG_MADVISE=y" >> $config_host_mak
> +fi
> +if test "$posix_madvise" = "yes" ; then
> +  echo "CONFIG_POSIX_MADVISE=y" >> $config_host_mak
> +fi
>
>  # XXX: suppress that
>  if [ "$bsd" = "yes" ] ; then
> diff --git a/exec.c b/exec.c
> index 380dab5..9b5464f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2841,9 +2841,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>             new_block->host = file_ram_alloc(new_block, size, mem_path);
>             if (!new_block->host) {
>                 new_block->host = qemu_vmalloc(size);
> -#ifdef MADV_MERGEABLE
> -                madvise(new_block->host, size, MADV_MERGEABLE);
> -#endif
> +                qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>             }
>  #else
>             fprintf(stderr, "-mem-path option unsupported\n");
> @@ -2858,9 +2856,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>  #else
>             new_block->host = qemu_vmalloc(size);
>  #endif
> -#ifdef MADV_MERGEABLE
> -            madvise(new_block->host, size, MADV_MERGEABLE);
> -#endif
> +            qemu_madvise(new_block->host, size, QEMU_MADV_MERGEABLE);
>         }
>     }
>
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 9fe3886..1e74674 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -51,8 +51,8 @@ static void balloon_page(void *addr, int deflate)
>  {
>  #if defined(__linux__)
>     if (!kvm_enabled() || kvm_has_sync_mmu())
> -        madvise(addr, TARGET_PAGE_SIZE,
> -                deflate ? MADV_WILLNEED : MADV_DONTNEED);
> +        qemu_madvise(addr, TARGET_PAGE_SIZE,
> +                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
>  #endif
>  }
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 58b0404..1cc696f 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1031,18 +1031,14 @@ int kvm_has_xcrs(void)
>  void kvm_setup_guest_memory(void *start, size_t size)
>  {
>     if (!kvm_has_sync_mmu()) {
> -#ifdef MADV_DONTFORK
> -        int ret = madvise(start, size, MADV_DONTFORK);
> +        int ret = qemu_madvise(start, size, QEMU_MADV_DONTFORK);
>
>         if (ret) {
> -            perror("madvice");
> +            perror("qemu_madvise");
> +            fprintf(stderr,
> +                    "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
>             exit(1);
>         }
> -#else
> -        fprintf(stderr,
> -                "Need MADV_DONTFORK in absence of synchronous KVM MMU\n");
> -        exit(1);
> -#endif
>     }
>  }
>
> diff --git a/osdep.c b/osdep.c
> index 30426ff..2c53015 100644
> --- a/osdep.c
> +++ b/osdep.c
> @@ -28,6 +28,9 @@
>  #include <errno.h>
>  #include <unistd.h>
>  #include <fcntl.h>
> +#if defined(CONFIG_MADVISE) || defined(CONFIG_POSIX_MADVISE)
> +#include <sys/mman.h>
> +#endif
>
>  /* Needed early for CONFIG_BSD etc. */
>  #include "config-host.h"
> @@ -35,6 +38,9 @@
>  #ifdef CONFIG_SOLARIS
>  #include <sys/types.h>
>  #include <sys/statvfs.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);
>  #endif
>
>  #ifdef CONFIG_EVENTFD
> @@ -139,6 +145,22 @@ void qemu_vfree(void *ptr)
>
>  #endif
>
> +int qemu_madvise(void *addr, size_t len, int advice)
> +{
> +    if (advice == QEMU_MADV_INVALID) {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +#if defined(CONFIG_MADVISE)
> +    return madvise(addr, len, advice);
> +#elif defined(CONFIG_POSIX_MADVISE)
> +    return posix_madvise(addr, len, advice);
> +#else
> +    errno = EINVAL;
> +    return -1;
> +#endif
> +}
> +
>  int qemu_create_pidfile(const char *filename)
>  {
>     char buffer[128];
> diff --git a/osdep.h b/osdep.h
> index 1cdc7e2..6716281 100644
> --- a/osdep.h
> +++ b/osdep.h
> @@ -90,6 +90,41 @@ void *qemu_memalign(size_t alignment, size_t size);
>  void *qemu_vmalloc(size_t size);
>  void qemu_vfree(void *ptr);
>
> +#define QEMU_MADV_INVALID -1
> +
> +#if defined(CONFIG_MADVISE)
> +
> +#define QEMU_MADV_WILLNEED  MADV_WILLNEED
> +#define QEMU_MADV_DONTNEED  MADV_DONTNEED
> +#ifdef MADV_DONTFORK
> +#define QEMU_MADV_DONTFORK  MADV_DONTFORK
> +#else
> +#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
> +#endif
> +#ifdef MADV_MERGEABLE
> +#define QEMU_MADV_MERGEABLE MADV_MERGEABLE
> +#else
> +#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
> +#endif
> +
> +#elif defined(CONFIG_POSIX_MADVISE)
> +
> +#define QEMU_MADV_WILLNEED  POSIX_MADV_WILLNEED
> +#define QEMU_MADV_DONTNEED  POSIX_MADV_DONTNEED
> +#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
> +#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
> +
> +#else /* no-op */
> +
> +#define QEMU_MADV_WILLNEED  QEMU_MADV_INVALID
> +#define QEMU_MADV_DONTNEED  QEMU_MADV_INVALID
> +#define QEMU_MADV_DONTFORK  QEMU_MADV_INVALID
> +#define QEMU_MADV_MERGEABLE QEMU_MADV_INVALID
> +
> +#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);
>  #endif
>  #endif
>  #endif
> --
> 1.7.2.2
>
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2010-09-25 15:18 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-11 17:05 [Qemu-devel] [PATCH] Introduce qemu_madvise() Andreas Färber
2010-09-11 21:37 ` Alexander Graf
2010-09-11 22:39   ` Andreas Färber
2010-09-11 22:47     ` Alexander Graf
2010-09-12  7:20 ` [Qemu-devel] " Blue Swirl
2010-09-12  8:50   ` Andreas Färber
2010-09-12  9:02     ` Blue Swirl
2010-09-12 12:55     ` [Qemu-devel] [PATCH v3] " Andreas Färber
2010-09-12 17:29       ` [Qemu-devel] " Blue Swirl
2010-09-13 12:02         ` Alexander Graf
2010-09-13 21:26         ` [Qemu-devel] [RFC v4] " Andreas Färber
2010-09-14 16:31           ` [Qemu-devel] " Blue Swirl
2010-09-14 16:34             ` Alexander Graf
2010-09-14 17:10               ` Blue Swirl
2010-09-14 20:28                 ` [Qemu-devel] [PATCH v5] " Andreas Färber
2010-09-14 20:36                   ` [Qemu-devel] " Blue Swirl
2010-09-14 20:39                     ` Andreas Färber
2010-09-15 18:09                       ` [Qemu-devel] [PATCH v6] " Andreas Färber
2010-09-15 19:00                         ` [Qemu-devel] " Blue Swirl
2010-09-15 19:35                           ` Andreas Färber
2010-09-15 19:50                             ` Blue Swirl
2010-09-15 20:07                               ` Andreas Färber
2010-09-19 10:11                               ` [Qemu-devel] [PATCH v7] " Andreas Färber
2010-09-20 20:33                                 ` [Qemu-devel] " Blue Swirl
2010-09-24 18:08                                   ` Andreas Färber
2010-09-25  7:49                                     ` Blue Swirl
2010-09-25 10:58                                       ` [Qemu-devel] [PATCH v8] " Andreas Färber
2010-09-25 15:17                                         ` [Qemu-devel] " Blue Swirl

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.