| Message ID | 20100201172101.GA10900@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Feb 01, 2010 at 07:21:02PM +0200, Michael S. Tsirkin wrote: > vhost-net only uses memory barriers to control SMP effects > (communication with userspace potentially running on a different CPU), > so it should use SMP barriers and not mandatory barriers for memory > access ordering, as suggested by Documentation/memory-barriers.txt > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Rusty, any feedback on this one? Thanks! > --- > drivers/vhost/vhost.c | 10 +++++----- > 1 files changed, 5 insertions(+), 5 deletions(-) > > The above applies on top of net-next-2.6. Does not seem to give any > measureable performance gain, but seems to generate less code > and I think it's better to use correct APIs. > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index c8c25db..6eb1525 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -685,7 +685,7 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log, > int i, r; > > /* Make sure data written is seen before log. */ > - wmb(); > + smp_wmb(); > for (i = 0; i < log_num; ++i) { > u64 l = min(log[i].len, len); > r = log_write(vq->log_base, log[i].addr, l); > @@ -884,7 +884,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq, > return vq->num; > > /* Only get avail ring entries after they have been exposed by guest. */ > - rmb(); > + smp_rmb(); > > /* Grab the next descriptor number they're advertising, and increment > * the index we've seen. */ > @@ -996,14 +996,14 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len) > return -EFAULT; > } > /* Make sure buffer is written before we update index. */ > - wmb(); > + smp_wmb(); > if (put_user(vq->last_used_idx + 1, &vq->used->idx)) { > vq_err(vq, "Failed to increment used idx"); > return -EFAULT; > } > if (unlikely(vq->log_used)) { > /* Make sure data is seen before log. */ > - wmb(); > + smp_wmb(); > log_write(vq->log_base, vq->log_addr + sizeof *vq->used->ring * > (vq->last_used_idx % vq->num), > sizeof *vq->used->ring); > @@ -1060,7 +1060,7 @@ bool vhost_enable_notify(struct vhost_virtqueue *vq) > } > /* They could have slipped one in as we were doing that: make > * sure it's written, then check again. */ > - mb(); > + smp_mb(); > r = get_user(avail_idx, &vq->avail->idx); > if (r) { > vq_err(vq, "Failed to check avail idx at %p: %d\n", > -- > 1.6.6.144.g5c3af -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/01/2010 07:21 PM, Michael S. Tsirkin wrote: > vhost-net only uses memory barriers to control SMP effects > (communication with userspace potentially running on a different CPU), > so it should use SMP barriers and not mandatory barriers for memory > access ordering, as suggested by Documentation/memory-barriers.txt > > A UP guest running on an SMP host still needs those barriers.
On Sun, Feb 07, 2010 at 11:42:29AM +0200, Avi Kivity wrote: > On 02/01/2010 07:21 PM, Michael S. Tsirkin wrote: >> vhost-net only uses memory barriers to control SMP effects >> (communication with userspace potentially running on a different CPU), >> so it should use SMP barriers and not mandatory barriers for memory >> access ordering, as suggested by Documentation/memory-barriers.txt >> >> > > A UP guest running on an SMP host still needs those barriers. Correct. And since vhost net is running host-side, smp_XX barriers will do exactly the right thing, right? > -- > error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/07/2010 11:44 AM, Michael S. Tsirkin wrote: > On Sun, Feb 07, 2010 at 11:42:29AM +0200, Avi Kivity wrote: > >> On 02/01/2010 07:21 PM, Michael S. Tsirkin wrote: >> >>> vhost-net only uses memory barriers to control SMP effects >>> (communication with userspace potentially running on a different CPU), >>> so it should use SMP barriers and not mandatory barriers for memory >>> access ordering, as suggested by Documentation/memory-barriers.txt >>> >>> >>> >> A UP guest running on an SMP host still needs those barriers. >> > Correct. And since vhost net is running host-side, smp_XX > barriers will do exactly the right thing, right? > Right, of course. Mixed up virtio and vhost.
On Sun, 7 Feb 2010 07:37:49 pm Michael S. Tsirkin wrote: > On Mon, Feb 01, 2010 at 07:21:02PM +0200, Michael S. Tsirkin wrote: > > vhost-net only uses memory barriers to control SMP effects > > (communication with userspace potentially running on a different CPU), > > so it should use SMP barriers and not mandatory barriers for memory > > access ordering, as suggested by Documentation/memory-barriers.txt > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > Rusty, any feedback on this one? > Thanks! Yep. barrier() is correct on UP to guard against preemption. Acked-by: Rusty Russell <rusty@rustcorp.com.au> Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 08, 2010 at 06:49:39PM +1030, Rusty Russell wrote: > On Sun, 7 Feb 2010 07:37:49 pm Michael S. Tsirkin wrote: > > On Mon, Feb 01, 2010 at 07:21:02PM +0200, Michael S. Tsirkin wrote: > > > vhost-net only uses memory barriers to control SMP effects > > > (communication with userspace potentially running on a different CPU), > > > so it should use SMP barriers and not mandatory barriers for memory > > > access ordering, as suggested by Documentation/memory-barriers.txt > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > Rusty, any feedback on this one? > > Thanks! > > Yep. barrier() is correct on UP to guard against preemption. > > Acked-by: Rusty Russell <rusty@rustcorp.com.au> > > Thanks, > Rusty. Dave, I see it's marked "not applicable": http://patchwork.ozlabs.org/patch/44207/ the patch applies to net-next as of b3b3f04fb587ecb61b5baa6c1c5f0e666fd12d73. Can this be queued up please? Should I resubmit with Rusty's ack? Thanks!
From: "Michael S. Tsirkin" <mst@redhat.com> Date: Sat, 13 Feb 2010 19:39:11 +0200 > Dave, I see it's marked "not applicable": > http://patchwork.ozlabs.org/patch/44207/ > the patch applies to net-next as of > b3b3f04fb587ecb61b5baa6c1c5f0e666fd12d73. > Can this be queued up please? > Should I resubmit with Rusty's ack? Sorry about that, I must have thought Rusty would queue it up. I'll fix the state to under-review and process it in my backlog. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index c8c25db..6eb1525 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -685,7 +685,7 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log, int i, r; /* Make sure data written is seen before log. */ - wmb(); + smp_wmb(); for (i = 0; i < log_num; ++i) { u64 l = min(log[i].len, len); r = log_write(vq->log_base, log[i].addr, l); @@ -884,7 +884,7 @@ unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq, return vq->num; /* Only get avail ring entries after they have been exposed by guest. */ - rmb(); + smp_rmb(); /* Grab the next descriptor number they're advertising, and increment * the index we've seen. */ @@ -996,14 +996,14 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len) return -EFAULT; } /* Make sure buffer is written before we update index. */ - wmb(); + smp_wmb(); if (put_user(vq->last_used_idx + 1, &vq->used->idx)) { vq_err(vq, "Failed to increment used idx"); return -EFAULT; } if (unlikely(vq->log_used)) { /* Make sure data is seen before log. */ - wmb(); + smp_wmb(); log_write(vq->log_base, vq->log_addr + sizeof *vq->used->ring * (vq->last_used_idx % vq->num), sizeof *vq->used->ring); @@ -1060,7 +1060,7 @@ bool vhost_enable_notify(struct vhost_virtqueue *vq) } /* They could have slipped one in as we were doing that: make * sure it's written, then check again. */ - mb(); + smp_mb(); r = get_user(avail_idx, &vq->avail->idx); if (r) { vq_err(vq, "Failed to check avail idx at %p: %d\n",
vhost-net only uses memory barriers to control SMP effects (communication with userspace potentially running on a different CPU), so it should use SMP barriers and not mandatory barriers for memory access ordering, as suggested by Documentation/memory-barriers.txt Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- drivers/vhost/vhost.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) The above applies on top of net-next-2.6. Does not seem to give any measureable performance gain, but seems to generate less code and I think it's better to use correct APIs.