From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=38559 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OuYk2-0001sv-KM for qemu-devel@nongnu.org; Sat, 11 Sep 2010 18:40:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OuYk1-0005fk-6w for qemu-devel@nongnu.org; Sat, 11 Sep 2010 18:40:14 -0400 Received: from fmmailgate01.web.de ([217.72.192.221]:57280) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OuYk0-0005fW-Pl for qemu-devel@nongnu.org; Sat, 11 Sep 2010 18:40:13 -0400 Message-Id: <982D5612-5993-42A1-B8AB-B8AFA1106C97@web.de> From: =?ISO-8859-1?Q?Andreas_F=E4rber?= In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed; delsp=yes Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 (Apple Message framework v936) Subject: Re: [Qemu-devel] [PATCH] Introduce qemu_madvise() Date: Sun, 12 Sep 2010 00:39:49 +0200 References: <1284224755-11299-1-git-send-email-andreas.faerber@web.de> Sender: andreas.faerber@web.de List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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=E4rber wrote: > >> Remaining madvise() users: >> exec.c: limited to __linux__ and/or MADV_MERGEABLE (no POSIX =20 >> 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 =20 >> version_id) >> #ifndef _WIN32 >> if (ch =3D=3D 0 && >> (!kvm_enabled() || kvm_has_sync_mmu())) { >> - madvise(host, TARGET_PAGE_SIZE, MADV_DONTNEED); >> + qemu_madvise(host, TARGET_PAGE_SIZE, =20 >> QEMU_MADV_DONTNEED); > > Is this the only occurence in the whole code base? This patch should =20= > 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 =20 and convert the callers, too, but what's the point when only madvise =20 supports it? Using the current #ifdef mechanism we can detect/handle it at compile-=20= time; inside qemu_madvise it would be deferred to runtime. Or do you =20 have a suggestion how to handle that scenario other than returning an =20= 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 =3D 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 =20= > that makes the bits slightly more obvious. You mean, "qemu_madvise: Advice %x ..."? Or "Advice %x for =20 posix_madvise ..."? >> + abort(); >> + } >> + return posix_madvise(addr, len, advice); >> +#else >> + switch (advice) { >> + case QEMU_MADV_DONTNEED: >> + advice =3D 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 =20 like OpenSolaris. I was hoping for Michael Lotz to resurface though and haven't retried =20= myself yet. Platforms that have neither posix_madvise nor madvise are equally =20 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 =20 >> 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 =20= > make sense to just directly map qemu_madvise and real madvise bits. =20= > 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 =20= > 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 >> #include >> #include >> -/* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=3D7156) for >> - discussion about Solaris header problems */ >> -extern int madvise(caddr_t, size_t, int); > > Does Solaris have posix_madvise? Otherwise it would still require =20 > this header hack, no? I tested on OpenSolaris 2009.06. Haven't tested on Solaris 10 yet, =20 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=