[PATCH] virtio: use smp_XX barriers

[PATCH] virtio: use smp_XX barriers

[Michael S. Tsirkin]

Documentation/memory-barriers.txt says:
Mandatory barriers should not be used to control SMP effects, since
mandatory barriers unnecessarily impose overhead on UP systems.

This rule applies to virtio, so let's do it correctly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_ring.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fbd2ecd..2216587 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -36,10 +36,10 @@
 			panic("%s:in_use = %i\n",		\
 			      (_vq)->vq.name, (_vq)->in_use);	\
 		(_vq)->in_use = __LINE__;			\
-		mb();						\
+		smp_mb();						\
 	} while (0)
 #define END_USE(_vq) \
-	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; mb(); } while(0)
+	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; smp_mb(); } while(0)
 #else
 #define BAD_RING(_vq, fmt, args...)				\
 	do {							\
@@ -221,13 +221,13 @@ static void vring_kick(struct virtqueue *_vq)
 	START_USE(vq);
 	/* Descriptors and available array need to be set before we expose the
 	 * new available array entries. */
-	wmb();
+	smp_wmb();
 
 	vq->vring.avail->idx += vq->num_added;
 	vq->num_added = 0;
 
 	/* Need to update avail index before checking if we should notify */
-	mb();
+	smp_mb();
 
 	if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
 		/* Prod other side to tell it about changes. */
@@ -286,7 +286,7 @@ static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
 	}
 
 	/* Only get used array entries after they have been exposed by host. */
-	rmb();
+	smp_rmb();
 
 	i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
 	*len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
@@ -324,7 +324,7 @@ static bool vring_enable_cb(struct virtqueue *_vq)
 	/* We optimistically turn back on interrupts, then check if there was
 	 * more to do. */
 	vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
-	mb();
+	smp_mb();
 	if (unlikely(more_used(vq))) {
 		END_USE(vq);
 		return false;
-- 
1.6.6.144.g5c3af

Re: [PATCH] virtio: use smp_XX barriers

[Avi Kivity]

On 01/21/2010 07:10 PM, Michael S. Tsirkin wrote:
> Documentation/memory-barriers.txt says:
> Mandatory barriers should not be used to control SMP effects, since
> mandatory barriers unnecessarily impose overhead on UP systems.
>
> This rule applies to virtio, so let's do it correctly.
>
>    

This is wrong.  A UP guest still runs in parallel with the virtio device.

smp_mb() is used for processor-vs-processor ordering, which can't happen 
on UP systems, but for process-vs-device, we must use mb().

(this shows up if running a UP guest on an SMP host).


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] virtio: use smp_XX barriers

[Michael S. Tsirkin]

On Wed, Jan 27, 2010 at 11:28:32AM +0200, Avi Kivity wrote:
> On 01/21/2010 07:10 PM, Michael S. Tsirkin wrote:
>> Documentation/memory-barriers.txt says:
>> Mandatory barriers should not be used to control SMP effects, since
>> mandatory barriers unnecessarily impose overhead on UP systems.
>>
>> This rule applies to virtio, so let's do it correctly.
>>
>>    
>
> This is wrong.  A UP guest still runs in parallel with the virtio device.

Yes, you are right here. I forgot that these macros
are compiled out on UP guest. Rusty please ignore this patch.

> smp_mb() is used for processor-vs-processor ordering, which can't happen  
> on UP systems, but for process-vs-device, we must use mb().
>
> (this shows up if running a UP guest on an SMP host).

Currently, yes. But virtio is not a real device.
Here's what I was really trying to improve: rmb() is an lfence on
x86_64, but smp_rmb() is a barrier() and this is really sufficient for
virtio because x86_64 does not reorder memory reads.

Does this mean such an optimization would need a new macro?

-- 
MST

Re: [PATCH] virtio: use smp_XX barriers

[Avi Kivity]

On 01/27/2010 11:36 AM, Michael S. Tsirkin wrote:
>
>
>> smp_mb() is used for processor-vs-processor ordering, which can't happen
>> on UP systems, but for process-vs-device, we must use mb().
>>
>> (this shows up if running a UP guest on an SMP host).
>>      
> Currently, yes. But virtio is not a real device.
> Here's what I was really trying to improve: rmb() is an lfence on
> x86_64, but smp_rmb() is a barrier() and this is really sufficient for
> virtio because x86_64 does not reorder memory reads.
>
>    

x86_64 can do speculative and reordered reads.

> Does this mean such an optimization would need a new macro?
>    

Please no.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] virtio: use smp_XX barriers

[Michael S. Tsirkin]

On Wed, Jan 27, 2010 at 11:46:32AM +0200, Avi Kivity wrote:
> On 01/27/2010 11:36 AM, Michael S. Tsirkin wrote:
>>
>>
>>> smp_mb() is used for processor-vs-processor ordering, which can't happen
>>> on UP systems, but for process-vs-device, we must use mb().
>>>
>>> (this shows up if running a UP guest on an SMP host).
>>>      
>> Currently, yes. But virtio is not a real device.
>> Here's what I was really trying to improve: rmb() is an lfence on
>> x86_64, but smp_rmb() is a barrier() and this is really sufficient for
>> virtio because x86_64 does not reorder memory reads.
>>
>>    
>
> x86_64 can do speculative and reordered reads.

Intel system programming says:
7.2.3.2 Neither Loads Nor Stores Are Reordered with Like Operations
Doesn't this mean that reads are not reordered?  If no what does it
mean?

>> Does this mean such an optimization would need a new macro?
>>    
>
> Please no.
>
> -- 
> Do not meddle in the internals of kernels, for they are subtle and quick to panic.