All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: David Vrabel <david.vrabel@citrix.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Jason Wang <jasowang@redhat.com>,
	Rusty Russell <rusty@rustcorp.com.au>,
	qemu-devel@nongnu.org,
	Alexander Duyck <alexander.duyck@gmail.com>,
	linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	virtualization@lists.linux-foundation.org,
	Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
Date: Mon, 21 Dec 2015 13:52:44 +0200	[thread overview]
Message-ID: <20151221134325-mutt-send-email-mst@redhat.com> (raw)
In-Reply-To: <5677D8D5.7080700@citrix.com>

On Mon, Dec 21, 2015 at 10:47:49AM +0000, David Vrabel wrote:
> On 20/12/15 09:25, Michael S. Tsirkin wrote:
> > 
> > I noticed that drivers/xen/xenbus/xenbus_comms.c uses
> > full memory barriers to communicate with the other side.
> > For example:
> > 
> >                 /* Must write data /after/ reading the consumer index.  * */
> >                 mb();
> > 
> >                 memcpy(dst, data, avail);
> >                 data += avail;
> >                 len -= avail;
> >         
> >                 /* Other side must not see new producer until data is * there. */
> >                 wmb();
> >                 intf->req_prod += avail;
> >                 
> >                 /* Implies mb(): other side will see the updated producer. */
> >                 notify_remote_via_evtchn(xen_store_evtchn);
> > 
> > To me, it looks like for guests compiled with CONFIG_SMP, smp_wmb and smp_mb
> > would be sufficient, so mb() and wmb() here are only needed if
> > a non-SMP guest runs on an SMP host.
> > 
> > Is my analysis correct?
> 
> For x86, yes.
> 
> For arm/arm64 I think so, but would prefer one of the Xen arm
> maintainers to confirm.  In particular, whether inner-shareable barriers
> are sufficient for memory shared with the hypervisor.
> 
> > So what I'm suggesting is something like the below patch,
> > except instead of using virtio directly, a new set of barriers
> > that behaves identically for SMP and non-SMP guests will be introduced.
> > 
> > And of course the weak barriers flag is not needed for Xen -
> > that's a virtio only thing.
> > 
> > For example:
> > 
> > smp_pv_wmb()
> > smp_pv_rmb()
> > smp_pv_mb()
> 
> The smp_ prefix doesn't make a lot of sense to me here since these
> barriers are going to be the same whether the kernel is SMP or not.
> 
> David

Guest kernel - yes. But it's only needed because you
are running on an SMP host.

-- 
MST

WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: David Vrabel <david.vrabel@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Jason Wang <jasowang@redhat.com>,
	Rusty Russell <rusty@rustcorp.com.au>,
	qemu-devel@nongnu.org,
	Alexander Duyck <alexander.duyck@gmail.com>,
	linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	virtualization@lists.linux-foundation.org
Subject: Re: [Qemu-devel] [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
Date: Mon, 21 Dec 2015 13:52:44 +0200	[thread overview]
Message-ID: <20151221134325-mutt-send-email-mst@redhat.com> (raw)
In-Reply-To: <5677D8D5.7080700@citrix.com>

On Mon, Dec 21, 2015 at 10:47:49AM +0000, David Vrabel wrote:
> On 20/12/15 09:25, Michael S. Tsirkin wrote:
> > 
> > I noticed that drivers/xen/xenbus/xenbus_comms.c uses
> > full memory barriers to communicate with the other side.
> > For example:
> > 
> >                 /* Must write data /after/ reading the consumer index.  * */
> >                 mb();
> > 
> >                 memcpy(dst, data, avail);
> >                 data += avail;
> >                 len -= avail;
> >         
> >                 /* Other side must not see new producer until data is * there. */
> >                 wmb();
> >                 intf->req_prod += avail;
> >                 
> >                 /* Implies mb(): other side will see the updated producer. */
> >                 notify_remote_via_evtchn(xen_store_evtchn);
> > 
> > To me, it looks like for guests compiled with CONFIG_SMP, smp_wmb and smp_mb
> > would be sufficient, so mb() and wmb() here are only needed if
> > a non-SMP guest runs on an SMP host.
> > 
> > Is my analysis correct?
> 
> For x86, yes.
> 
> For arm/arm64 I think so, but would prefer one of the Xen arm
> maintainers to confirm.  In particular, whether inner-shareable barriers
> are sufficient for memory shared with the hypervisor.
> 
> > So what I'm suggesting is something like the below patch,
> > except instead of using virtio directly, a new set of barriers
> > that behaves identically for SMP and non-SMP guests will be introduced.
> > 
> > And of course the weak barriers flag is not needed for Xen -
> > that's a virtio only thing.
> > 
> > For example:
> > 
> > smp_pv_wmb()
> > smp_pv_rmb()
> > smp_pv_mb()
> 
> The smp_ prefix doesn't make a lot of sense to me here since these
> barriers are going to be the same whether the kernel is SMP or not.
> 
> David

Guest kernel - yes. But it's only needed because you
are running on an SMP host.

-- 
MST

WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: David Vrabel <david.vrabel@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>,
	Peter Zijlstra <peterz@infradead.org>,
	qemu-devel@nongnu.org,
	Alexander Duyck <alexander.duyck@gmail.com>,
	linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	virtualization@lists.linux-foundation.org
Subject: Re: [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)
Date: Mon, 21 Dec 2015 13:52:44 +0200	[thread overview]
Message-ID: <20151221134325-mutt-send-email-mst@redhat.com> (raw)
In-Reply-To: <5677D8D5.7080700@citrix.com>

On Mon, Dec 21, 2015 at 10:47:49AM +0000, David Vrabel wrote:
> On 20/12/15 09:25, Michael S. Tsirkin wrote:
> > 
> > I noticed that drivers/xen/xenbus/xenbus_comms.c uses
> > full memory barriers to communicate with the other side.
> > For example:
> > 
> >                 /* Must write data /after/ reading the consumer index.  * */
> >                 mb();
> > 
> >                 memcpy(dst, data, avail);
> >                 data += avail;
> >                 len -= avail;
> >         
> >                 /* Other side must not see new producer until data is * there. */
> >                 wmb();
> >                 intf->req_prod += avail;
> >                 
> >                 /* Implies mb(): other side will see the updated producer. */
> >                 notify_remote_via_evtchn(xen_store_evtchn);
> > 
> > To me, it looks like for guests compiled with CONFIG_SMP, smp_wmb and smp_mb
> > would be sufficient, so mb() and wmb() here are only needed if
> > a non-SMP guest runs on an SMP host.
> > 
> > Is my analysis correct?
> 
> For x86, yes.
> 
> For arm/arm64 I think so, but would prefer one of the Xen arm
> maintainers to confirm.  In particular, whether inner-shareable barriers
> are sufficient for memory shared with the hypervisor.
> 
> > So what I'm suggesting is something like the below patch,
> > except instead of using virtio directly, a new set of barriers
> > that behaves identically for SMP and non-SMP guests will be introduced.
> > 
> > And of course the weak barriers flag is not needed for Xen -
> > that's a virtio only thing.
> > 
> > For example:
> > 
> > smp_pv_wmb()
> > smp_pv_rmb()
> > smp_pv_mb()
> 
> The smp_ prefix doesn't make a lot of sense to me here since these
> barriers are going to be the same whether the kernel is SMP or not.
> 
> David

Guest kernel - yes. But it's only needed because you
are running on an SMP host.

-- 
MST

  parent reply	other threads:[~2015-12-21 11:52 UTC|newest]

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-17 10:32 [PATCH] virtio_ring: use smp_store_mb Michael S. Tsirkin
2015-12-17 10:32 ` [Qemu-devel] " Michael S. Tsirkin
2015-12-17 10:52 ` Peter Zijlstra
2015-12-17 10:52   ` Peter Zijlstra
2015-12-17 10:52   ` [Qemu-devel] " Peter Zijlstra
2015-12-17 13:16   ` Michael S. Tsirkin
2015-12-17 13:16     ` Michael S. Tsirkin
2015-12-17 13:16     ` [Qemu-devel] " Michael S. Tsirkin
2015-12-17 13:57     ` Peter Zijlstra
2015-12-17 13:57       ` Peter Zijlstra
2015-12-17 13:57       ` [Qemu-devel] " Peter Zijlstra
2015-12-17 14:33       ` Michael S. Tsirkin
2015-12-17 14:33         ` Michael S. Tsirkin
2015-12-17 14:33         ` [Qemu-devel] " Michael S. Tsirkin
2015-12-17 14:39         ` Peter Zijlstra
2015-12-17 14:39           ` Peter Zijlstra
2015-12-17 14:39           ` [Qemu-devel] " Peter Zijlstra
2015-12-17 14:43           ` Michael S. Tsirkin
2015-12-17 14:43           ` Michael S. Tsirkin
2015-12-17 14:43             ` [Qemu-devel] " Michael S. Tsirkin
2015-12-20  9:25           ` new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb) Michael S. Tsirkin
2015-12-20  9:25             ` Michael S. Tsirkin
2015-12-20  9:25             ` [Qemu-devel] " Michael S. Tsirkin
2015-12-20 17:07             ` Andrew Cooper
2015-12-20 17:07             ` [Xen-devel] " Andrew Cooper
2015-12-20 17:07               ` Andrew Cooper
2015-12-20 17:07               ` [Qemu-devel] " Andrew Cooper
2015-12-20 19:59               ` Peter Zijlstra
2015-12-20 19:59                 ` Peter Zijlstra
2015-12-20 19:59                 ` [Qemu-devel] " Peter Zijlstra
2015-12-21  7:10                 ` Michael S. Tsirkin
2015-12-21  7:10                 ` [Xen-devel] " Michael S. Tsirkin
2015-12-21  7:10                 ` Michael S. Tsirkin
2015-12-21  7:10                   ` [Qemu-devel] " Michael S. Tsirkin
2015-12-21  7:22                   ` [PATCH RFC] smp_store_mb should use smp_mb Michael S. Tsirkin
2015-12-21  7:22                   ` Michael S. Tsirkin
2015-12-21  7:22                   ` Michael S. Tsirkin
2015-12-21  7:22                     ` [Qemu-devel] " Michael S. Tsirkin
2015-12-20 19:59               ` new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb) Peter Zijlstra
2015-12-21 10:47             ` David Vrabel
2015-12-21 10:47             ` [Xen-devel] " David Vrabel
2015-12-21 10:47               ` David Vrabel
2015-12-21 10:47               ` [Qemu-devel] " David Vrabel
2015-12-21 11:52               ` Michael S. Tsirkin
2015-12-21 11:52               ` Michael S. Tsirkin [this message]
2015-12-21 11:52                 ` [Xen-devel] " Michael S. Tsirkin
2015-12-21 11:52                 ` [Qemu-devel] " Michael S. Tsirkin
2015-12-21 14:50               ` Stefano Stabellini
2015-12-21 14:50                 ` Stefano Stabellini
2015-12-21 14:50                 ` Stefano Stabellini
2015-12-21 14:50                 ` Stefano Stabellini
2015-12-21 14:50               ` [Qemu-devel] " Stefano Stabellini
2015-12-20  9:25           ` Michael S. Tsirkin
2015-12-17 11:22 ` [PATCH] virtio_ring: use smp_store_mb Peter Zijlstra
2015-12-17 11:22   ` [Qemu-devel] " Peter Zijlstra
2015-12-17 13:26   ` Michael S. Tsirkin
2015-12-17 13:26     ` Michael S. Tsirkin
2015-12-17 13:26     ` [Qemu-devel] " Michael S. Tsirkin
2015-12-17 14:02     ` Peter Zijlstra
2015-12-17 14:02     ` Peter Zijlstra
2015-12-17 14:02       ` [Qemu-devel] " Peter Zijlstra
2015-12-17 14:34       ` Michael S. Tsirkin
2015-12-17 14:34       ` Michael S. Tsirkin
2015-12-17 14:34         ` [Qemu-devel] " Michael S. Tsirkin
2015-12-17 15:09         ` Peter Zijlstra
2015-12-17 15:09         ` Peter Zijlstra
2015-12-17 15:09           ` [Qemu-devel] " Peter Zijlstra
2015-12-17 15:52           ` Will Deacon
2015-12-17 15:52           ` Will Deacon
2015-12-17 15:52             ` [Qemu-devel] " Will Deacon
2015-12-17 19:21             ` Michael S. Tsirkin
2015-12-17 19:21               ` Michael S. Tsirkin
2015-12-17 19:21               ` [Qemu-devel] " Michael S. Tsirkin
2015-12-17 11:22 ` Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20151221134325-mutt-send-email-mst@redhat.com \
    --to=mst@redhat.com \
    --cc=alexander.duyck@gmail.com \
    --cc=boris.ostrovsky@oracle.com \
    --cc=david.vrabel@citrix.com \
    --cc=ian.campbell@citrix.com \
    --cc=jasowang@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=qemu-devel@nongnu.org \
    --cc=rusty@rustcorp.com.au \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.