From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:58568)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <aarcange@redhat.com>) id 1V2IqQ-0005jv-73
	for qemu-devel@nongnu.org; Thu, 25 Jul 2013 06:32:28 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <aarcange@redhat.com>) id 1V2IqN-0000t8-Lp
	for qemu-devel@nongnu.org; Thu, 25 Jul 2013 06:32:26 -0400
Date: Thu, 25 Jul 2013 12:32:20 +0200
From: Andrea Arcangeli <aarcange@redhat.com>
Message-ID: <20130725103220.GH10517@redhat.com>
References: <1374747075-7172-1-git-send-email-aarcange@redhat.com>
	<CAFEAcA8vb3+n+F3=VfX204HnEmCrc9Db+e=V-zeqG_eySb7VDA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAFEAcA8vb3+n+F3=VfX204HnEmCrc9Db+e=V-zeqG_eySb7VDA@mail.gmail.com>
Subject: Re: [Qemu-devel] [PATCH] KVM: always use MADV_DONTFORK
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org, Gleb Natapov <gleb@redhat.com>, qemu-stable@nongnu.org

On Thu, Jul 25, 2013 at 11:16:44AM +0100, Peter Maydell wrote:
> On 25 July 2013 11:11, Andrea Arcangeli <aarcange@redhat.com> wrote:
> > diff --git a/exec.c b/exec.c
> > index c99a883..d3bb58d 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1162,6 +1162,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
> >
> >      qemu_ram_setup_dump(new_block->host, size);
> >      qemu_madvise(new_block->host, size, QEMU_MADV_HUGEPAGE);
> > +    qemu_madvise(new_block->host, size, QEMU_MADV_DONTFORK);
> >
> >      if (kvm_enabled())
> >          kvm_setup_guest_memory(new_block->host, size);
> >
> 
> kvm_setup_guest_memory() already calls
>   qemu_madvise(start, size, QEMU_MADV_DONTFORK)
> so why do we need to do it here as well?

That only runs if kvm is enabled and mmu is not sync. But we need it
in the common case too, to prevent -ENOMEM (if MADV_DONTFORK is
available in the host OS, otherwise well we'll just do best effort and
skip). See commit message for more details.

> If we should be doing it in all cases presumably the right
> fix is to move the if (!kvm_has_sync_mmu()) check in
> kvm_setup_guest_memory() from "do we call madvise" to
> "do we fail with an error if it failed".

We could pass an error to kvm_setup_guest_memory but it's not worth it
considering more likely we should abort if kvm is enabled and mmu is
not sync (without bothering to call MADV_DONTFORK there).