[dpdk-dev] [PATCH] vhost: call write barrier before used index update

[dpdk-dev] [PATCH] vhost: call write barrier before used index update

[Nikita Kalyazin]

Descriptors that have been put into the used vring must be observable by
guest earlier than the new used index value.
Although compiler barrier serves well for Intel architectue here, the
proper cross-platform solution is to use write barrier before the used
index is updated.

Signed-off-by: Nikita Kalyazin <n.kalyazin@samsung.com>
---
 lib/librte_vhost/vhost_rxtx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 7026bfa..d955287 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -216,7 +216,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 		}
 	}
 
-	rte_compiler_barrier();
+	rte_wmb();
 
 	/* Wait until it's our turn to add our buffer to the used ring. */
 	while (unlikely(vq->last_used_idx != res_base_idx))
@@ -512,7 +512,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 		entry_success = copy_from_mbuf_to_vring(dev, res_base_idx,
 			res_cur_idx, pkts[pkt_idx]);
 
-		rte_compiler_barrier();
+		rte_wmb();
 
 		/*
 		 * Wait until it's our turn to add our buffer
@@ -751,7 +751,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
 		entry_success++;
 	}
 
-	rte_compiler_barrier();
+	rte_wmb();
 	vq->used->idx += entry_success;
 	/* Kick guest if required. */
 	if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
-- 
2.5.3

Re: [dpdk-dev] [PATCH] vhost: call write barrier before used index update

[Bruce Richardson]

On Tue, Oct 20, 2015 at 05:07:46PM +0300, Nikita Kalyazin wrote:
> Descriptors that have been put into the used vring must be observable by
> guest earlier than the new used index value.
> Although compiler barrier serves well for Intel architectue here, the
> proper cross-platform solution is to use write barrier before the used
> index is updated.
> 
> Signed-off-by: Nikita Kalyazin <n.kalyazin@samsung.com>
> ---
Yes, but no! :-)

This has been discussed a number of times before on list, and the consensus
seems to be that the correct way to fix this is to introduce a set of specific
barrier operations that insert the correct barrier type on each architecture,
i.e. compiler barriers on IA, and full wmbs on architectures that require that.

See discussion here: http://dpdk.org/dev/patchwork/patch/4293/
and in the thread here: http://dpdk.org/ml/archives/dev/2015-March/015202.html

So correct problem statment, but unfortunately NAK for the implementation.

Patches for general memory barrier implementation as described above welcome :-)

Regards,
/Bruce

Re: [dpdk-dev] [PATCH] vhost: call write barrier before used index update

[Nikita Kalyazin]

Hi,

> This has been discussed a number of times before on list, and the consensus
> seems to be that the correct way to fix this is to introduce a set of specific
> barrier operations that insert the correct barrier type on each architecture,
> i.e. compiler barriers on IA, and full wmbs on architectures that require that.
Linux kernel contains two sets of macros: *mb() and smp_*mb().  As far as
I understand, the former are meant to order memory accesses when interacting
with peripherals (physical NICs in our case), and the latter - to provide
synchronization between CPU cores (applicable for virtual NICs in our case).
smp_*mb() for Intel architecture would be a simple compiler barrier, whereas
for processors with weaker memory model they may use real barrier
instructions.
Maybe implementing barriers similar way would work in DPDK as well?

-- 

Best regards,

Nikita Kalyazin,
n.kalyazin@samsung.com

Software Engineer
Virtualization Group
Samsung R&D Institute Russia
Tel: +7 (495) 797-25-00 #3816
Tel: +7 (495) 797-25-03
Office #1501, 12-1, Dvintsev str.,
Moscow, 127018, Russia

On Tue, Oct 20, 2015 at 03:29:51PM +0100, Bruce Richardson wrote:
> On Tue, Oct 20, 2015 at 05:07:46PM +0300, Nikita Kalyazin wrote:
> > Descriptors that have been put into the used vring must be observable by
> > guest earlier than the new used index value.
> > Although compiler barrier serves well for Intel architectue here, the
> > proper cross-platform solution is to use write barrier before the used
> > index is updated.
> > 
> > Signed-off-by: Nikita Kalyazin <n.kalyazin@samsung.com>
> > ---
> Yes, but no! :-)
> 
> This has been discussed a number of times before on list, and the consensus
> seems to be that the correct way to fix this is to introduce a set of specific
> barrier operations that insert the correct barrier type on each architecture,
> i.e. compiler barriers on IA, and full wmbs on architectures that require that.
> 
> See discussion here: http://dpdk.org/dev/patchwork/patch/4293/
> and in the thread here: http://dpdk.org/ml/archives/dev/2015-March/015202.html
> 
> So correct problem statment, but unfortunately NAK for the implementation.
> 
> Patches for general memory barrier implementation as described above welcome :-)
> 
> Regards,
> /Bruce

Re: [dpdk-dev] [PATCH] vhost: call write barrier before used index update

[Bruce Richardson]

On 21/10/2015 16:42, Nikita Kalyazin wrote:
> Hi,
>
>> This has been discussed a number of times before on list, and the consensus
>> seems to be that the correct way to fix this is to introduce a set of specific
>> barrier operations that insert the correct barrier type on each architecture,
>> i.e. compiler barriers on IA, and full wmbs on architectures that require that.
> Linux kernel contains two sets of macros: *mb() and smp_*mb().  As far as
> I understand, the former are meant to order memory accesses when interacting
> with peripherals (physical NICs in our case), and the latter - to provide
> synchronization between CPU cores (applicable for virtual NICs in our case).
> smp_*mb() for Intel architecture would be a simple compiler barrier, whereas
> for processors with weaker memory model they may use real barrier
> instructions.
> Maybe implementing barriers similar way would work in DPDK as well?
>
It should work indeed.
Not sure if we need two separate sets of barrier functions, the 
smb_*mb() might work on their own (certainly for IA, they should do - 
not sure for other architectures), but I think the general consensus, 
based on previous discussions, is that this is the direction we want to 
go in for these barrier issues.

/Bruce