From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756109AbbLQNQY (ORCPT ); Thu, 17 Dec 2015 08:16:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46297 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756007AbbLQNQX (ORCPT ); Thu, 17 Dec 2015 08:16:23 -0500 Date: Thu, 17 Dec 2015 15:16:20 +0200 From: "Michael S. Tsirkin" To: Peter Zijlstra Cc: linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Jason Wang , qemu-devel@nongnu.org Subject: Re: [PATCH] virtio_ring: use smp_store_mb Message-ID: <20151217131554-mutt-send-email-mst@redhat.com> References: <1450347932-16325-1-git-send-email-mst@redhat.com> <20151217105238.GA6375@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151217105238.GA6375@twins.programming.kicks-ass.net> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 17, 2015 at 11:52:38AM +0100, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote: > > +static inline void virtio_store_mb(bool weak_barriers, > > + __virtio16 *p, __virtio16 v) > > +{ > > +#ifdef CONFIG_SMP > > + if (weak_barriers) > > + smp_store_mb(*p, v); > > + else > > +#endif > > + { > > + WRITE_ONCE(*p, v); > > + mb(); > > + } > > +} > > This is a different barrier depending on SMP, that seems wrong. Of course it's wrong in the sense that it's suboptimal on UP. What we would really like is to have, on UP, exactly the same barrier as on SMP. This is because a UP guest can run on an SMP host. But Linux doesn't provide this ability: if CONFIG_SMP is not defined is optimizes most barriers out to a compiler barrier. Consider for example x86: what we want is xchg (NOT mfence - see below for why) but if built without CONFIG_SMP smp_store_mb does not include this. > smp_mb(), as (should be) used by smp_store_mb() does not provide a > barrier against IO. mb() otoh does. > > Since this is virtIO I would expect you always want mb(). No because it's VIRTio not real io :) It just switches to the hyprevisor mode - kind of like a function call really. The weak_barriers flag is cleared for when it's used with real devices with real IO. All this is explained in some detail at the top of include/linux/virtio.h -- MST From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58944) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9YQ1-0002M0-HN for qemu-devel@nongnu.org; Thu, 17 Dec 2015 08:16:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9YPw-0003NG-Qj for qemu-devel@nongnu.org; Thu, 17 Dec 2015 08:16:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60094) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9YPw-0003NA-LU for qemu-devel@nongnu.org; Thu, 17 Dec 2015 08:16:24 -0500 Date: Thu, 17 Dec 2015 15:16:20 +0200 From: "Michael S. Tsirkin" Message-ID: <20151217131554-mutt-send-email-mst@redhat.com> References: <1450347932-16325-1-git-send-email-mst@redhat.com> <20151217105238.GA6375@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151217105238.GA6375@twins.programming.kicks-ass.net> Subject: Re: [Qemu-devel] [PATCH] virtio_ring: use smp_store_mb List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Zijlstra Cc: qemu-devel@nongnu.org, Jason Wang , linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org On Thu, Dec 17, 2015 at 11:52:38AM +0100, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote: > > +static inline void virtio_store_mb(bool weak_barriers, > > + __virtio16 *p, __virtio16 v) > > +{ > > +#ifdef CONFIG_SMP > > + if (weak_barriers) > > + smp_store_mb(*p, v); > > + else > > +#endif > > + { > > + WRITE_ONCE(*p, v); > > + mb(); > > + } > > +} > > This is a different barrier depending on SMP, that seems wrong. Of course it's wrong in the sense that it's suboptimal on UP. What we would really like is to have, on UP, exactly the same barrier as on SMP. This is because a UP guest can run on an SMP host. But Linux doesn't provide this ability: if CONFIG_SMP is not defined is optimizes most barriers out to a compiler barrier. Consider for example x86: what we want is xchg (NOT mfence - see below for why) but if built without CONFIG_SMP smp_store_mb does not include this. > smp_mb(), as (should be) used by smp_store_mb() does not provide a > barrier against IO. mb() otoh does. > > Since this is virtIO I would expect you always want mb(). No because it's VIRTio not real io :) It just switches to the hyprevisor mode - kind of like a function call really. The weak_barriers flag is cleared for when it's used with real devices with real IO. All this is explained in some detail at the top of include/linux/virtio.h -- MST From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH] virtio_ring: use smp_store_mb Date: Thu, 17 Dec 2015 15:16:20 +0200 Message-ID: <20151217131554-mutt-send-email-mst@redhat.com> References: <1450347932-16325-1-git-send-email-mst@redhat.com> <20151217105238.GA6375@twins.programming.kicks-ass.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20151217105238.GA6375@twins.programming.kicks-ass.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Peter Zijlstra Cc: qemu-devel@nongnu.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org List-Id: virtualization@lists.linuxfoundation.org On Thu, Dec 17, 2015 at 11:52:38AM +0100, Peter Zijlstra wrote: > On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin wrote: > > +static inline void virtio_store_mb(bool weak_barriers, > > + __virtio16 *p, __virtio16 v) > > +{ > > +#ifdef CONFIG_SMP > > + if (weak_barriers) > > + smp_store_mb(*p, v); > > + else > > +#endif > > + { > > + WRITE_ONCE(*p, v); > > + mb(); > > + } > > +} > > This is a different barrier depending on SMP, that seems wrong. Of course it's wrong in the sense that it's suboptimal on UP. What we would really like is to have, on UP, exactly the same barrier as on SMP. This is because a UP guest can run on an SMP host. But Linux doesn't provide this ability: if CONFIG_SMP is not defined is optimizes most barriers out to a compiler barrier. Consider for example x86: what we want is xchg (NOT mfence - see below for why) but if built without CONFIG_SMP smp_store_mb does not include this. > smp_mb(), as (should be) used by smp_store_mb() does not provide a > barrier against IO. mb() otoh does. > > Since this is virtIO I would expect you always want mb(). No because it's VIRTio not real io :) It just switches to the hyprevisor mode - kind of like a function call really. The weak_barriers flag is cleared for when it's used with real devices with real IO. All this is explained in some detail at the top of include/linux/virtio.h -- MST