[PATCH] vhost-net: switch to smp barriers

[PATCH] vhost-net: switch to smp barriers

[Michael S. Tsirkin]

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.

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

Re: [PATCH] vhost-net: switch to smp barriers

[Michael S. Tsirkin]

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

Re: [PATCH] vhost-net: switch to smp barriers

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] vhost-net: switch to smp barriers

[Michael S. Tsirkin]

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

Re: [PATCH] vhost-net: switch to smp barriers

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] vhost-net: switch to smp barriers

[Rusty Russell]

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.

Re: [PATCH] vhost-net: switch to smp barriers

[Michael S. Tsirkin]

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!

-- 
MST

Re: [PATCH] vhost-net: switch to smp barriers

[David Miller]

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.