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

[Blue Swirl <blauwirbel@gmail.com>]

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