From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:37819) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qz1qz-0000O7-Lr for qemu-devel@nongnu.org; Thu, 01 Sep 2011 03:38:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qz1qy-00061X-AC for qemu-devel@nongnu.org; Thu, 01 Sep 2011 03:38:25 -0400 Received: from mail-fx0-f45.google.com ([209.85.161.45]:52726) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qz1qx-00061R-RV for qemu-devel@nongnu.org; Thu, 01 Sep 2011 03:38:24 -0400 Received: by fxbb27 with SMTP id b27so473604fxb.4 for ; Thu, 01 Sep 2011 00:38:23 -0700 (PDT) From: Sasha Levin In-Reply-To: <1314862622.26813.6.camel@lappy> References: <1314857389-13363-1-git-send-email-david@gibson.dropbear.id.au> <1314862622.26813.6.camel@lappy> Content-Type: text/plain; charset="us-ascii" Date: Thu, 01 Sep 2011 10:38:16 +0300 Message-ID: <1314862696.26813.7.camel@lappy> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio: Make memory barriers be memory barriers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: aliguori@us.ibm.com, aik@ozlabs.ru, rusty@rustcorp.com.au, qemu-devel@nongnu.org, agraf@suse.de, pbonzini@redhat.com On Thu, 2011-09-01 at 10:37 +0300, Sasha Levin wrote: > On Thu, 2011-09-01 at 16:09 +1000, David Gibson wrote: > > The virtio code already has memory barrier wmb() macros in the code. > > However they are was defined as no-ops. The comment claims that real > > barriers are not necessary because the code does not run concurrent. > > However, with kvm and io-thread enabled, this is not true and this qemu > > code can indeed run concurrently with the guest kernel. This does not > > cause problems on x86 due to it's strongly ordered storage model, but it > > causes a race leading to virtio errors on POWER which has a relaxed storage > > ordering model. > > > > Specifically, the QEMU puts new element into the "used" ring and then > > updates the ring free-running counter. Without a barrier between these > > under the right circumstances, the guest linux driver can receive an > > interrupt, read the counter change but find the ring element to be handled > > still has an old value, leading to an "id %u is not a head!\n" error > > message. > > > > The problem is easy to reproduce on POWER using virtio-net with heavy > > traffic. > > > > The patch defines wmb() as __sync_synchronize(), a cross platform memory > > barrier primitive available in sufficiently recent gcc versions (gcc 4.2 > > and after?). If we care about older gccs then this patch will need to > > be updated with some sort of fallback. > > > > Signed-off-by: Alexey Kardashevskiy > > Signed-off-by: David Gibson > > --- > > hw/virtio.c | 10 ++-------- > > 1 files changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/hw/virtio.c b/hw/virtio.c > > index 13aa0fa..c9f0e75 100644 > > --- a/hw/virtio.c > > +++ b/hw/virtio.c > > @@ -21,14 +21,8 @@ > > * x86 pagesize again. */ > > #define VIRTIO_PCI_VRING_ALIGN 4096 > > > > -/* QEMU doesn't strictly need write barriers since everything runs in > > - * lock-step. We'll leave the calls to wmb() in though to make it obvious for > > - * KVM or if kqemu gets SMP support. > > - * In any case, we must prevent the compiler from reordering the code. > > - * TODO: we likely need some rmb()/mb() as well. > > - */ > > - > > -#define wmb() __asm__ __volatile__("": : :"memory") > > + /* TODO: we may also need rmb()s. It hasn't bitten us yet, but.. */ > > + #define wmb() __sync_synchronize() > > That asm directive also implicitly provided a compiler barrier, I could couldn't. > find whether __sync_synchronize() provides one as well. > > Any idea if it does? > > > > > typedef struct VRingDesc > > { > -- Sasha.