[dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[Ilya Maximets]

Implementation of rte_vhost_enqueue_burst() based on lockless ring-buffer
algorithm and contains almost all to be thread-safe, but it's not.

This set adds required changes.

First patch in set is a standalone patch that fixes many times discussed
issue with barriers on different architectures.

Second and third adds fixes to make rte_vhost_enqueue_burst thread safe.

version 3:
	* Rebased on top of current master.

version 2:
	* Documentation patch dropped. Other patches of series still
	  may be merged to fix existing issues and keep code in
	  consistent state for the future.
	* buf_vec field of struct vhost_virtqueue marked as deprecated.

 Ilya Maximets (3):
  vhost: use SMP barriers instead of compiler ones.
  vhost: make buf vector for scatter RX local.
  vhost: avoid reordering of used->idx and last_used_idx updating.

 doc/guides/rel_notes/deprecation.rst |  1 +
 lib/librte_vhost/rte_virtio_net.h    |  2 +-
 lib/librte_vhost/vhost_rxtx.c        | 71 ++++++++++++++++++++----------------
 3 files changed, 42 insertions(+), 32 deletions(-)

-- 
2.5.0

[dpdk-dev] [PATCH RFC v3 1/3] vhost: use SMP barriers instead of compiler ones.

[Ilya Maximets]

Since commit 4c02e453cc62 ("eal: introduce SMP memory barriers") virtio
uses architecture dependent SMP barriers. vHost should use them too.

Fixes: 4c02e453cc62 ("eal: introduce SMP memory barriers")

Signed-off-by: Ilya Maximets <i.maximets@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 12ce0cc..14c2159 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -316,7 +316,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 		}
 	}
 
-	rte_compiler_barrier();
+	rte_smp_wmb();
 
 	/* Wait until it's our turn to add our buffer to the used ring. */
 	while (unlikely(vq->last_used_idx != res_base_idx))
@@ -634,7 +634,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 		entry_success = copy_from_mbuf_to_vring(dev, queue_id,
 			res_base_idx, res_cur_idx, pkts[pkt_idx]);
 
-		rte_compiler_barrier();
+		rte_smp_wmb();
 
 		/*
 		 * Wait until it's our turn to add our buffer
@@ -979,7 +979,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
 		entry_success++;
 	}
 
-	rte_compiler_barrier();
+	rte_smp_rmb();
 	vq->used->idx += entry_success;
 	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
 			sizeof(vq->used->idx));
-- 
2.5.0

[dpdk-dev] [PATCH RFC v3 2/3] vhost: make buf vector for scatter RX local.

[Ilya Maximets]

Array of buf_vector's is just an array for temporary storing information
about available descriptors. It used only locally in virtio_dev_merge_rx()
and there is no reason for that array to be shared.

Fix that by allocating local buf_vec inside virtio_dev_merge_rx().
buf_vec field of struct vhost_virtqueue marked as deprecated.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 doc/guides/rel_notes/deprecation.rst |  1 +
 lib/librte_vhost/rte_virtio_net.h    |  2 +-
 lib/librte_vhost/vhost_rxtx.c        | 49 ++++++++++++++++++------------------
 3 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index e94d4a2..40f350d 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -7,6 +7,7 @@ API and ABI deprecation notices are to be posted here.
 
 Deprecation Notices
 -------------------
+* Field buf_vec of struct vhost_virtqueue have been deprecated.
 
 * The following fields have been deprecated in rte_eth_stats:
   ibadcrc, ibadlen, imcasts, fdirmatch, fdirmiss,
diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
index 4a2303a..e6e5cf3 100644
--- a/lib/librte_vhost/rte_virtio_net.h
+++ b/lib/librte_vhost/rte_virtio_net.h
@@ -93,7 +93,7 @@ struct vhost_virtqueue {
 	int			enabled;
 	uint64_t		log_guest_addr;		/**< Physical address of used ring, for logging */
 	uint64_t		reserved[15];		/**< Reserve some spaces for future extension. */
-	struct buf_vector	buf_vec[BUF_VECTOR_MAX];	/**< for scatter RX. */
+	struct buf_vector       buf_vec[BUF_VECTOR_MAX] __rte_deprecated;        /**< @deprecated Buffer for scatter RX. */
 } __rte_cache_aligned;
 
 
diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 14c2159..a8e2582 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -340,7 +340,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 static inline uint32_t __attribute__((always_inline))
 copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 			uint16_t res_base_idx, uint16_t res_end_idx,
-			struct rte_mbuf *pkt)
+			struct rte_mbuf *pkt, struct buf_vector *buf_vec)
 {
 	uint32_t vec_idx = 0;
 	uint32_t entry_success = 0;
@@ -371,7 +371,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 	 */
 	vq = dev->virtqueue[queue_id];
 
-	vb_addr = gpa_to_vva(dev, vq->buf_vec[vec_idx].buf_addr);
+	vb_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
 	vb_hdr_addr = vb_addr;
 
 	/* Prefetch buffer address. */
@@ -386,24 +386,24 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 
 	rte_memcpy((void *)(uintptr_t)vb_hdr_addr,
 		(const void *)&virtio_hdr, vq->vhost_hlen);
-	vhost_log_write(dev, vq->buf_vec[vec_idx].buf_addr, vq->vhost_hlen);
+	vhost_log_write(dev, buf_vec[vec_idx].buf_addr, vq->vhost_hlen);
 
 	PRINT_PACKET(dev, (uintptr_t)vb_hdr_addr, vq->vhost_hlen, 1);
 
 	seg_avail = rte_pktmbuf_data_len(pkt);
 	vb_offset = vq->vhost_hlen;
-	vb_avail = vq->buf_vec[vec_idx].buf_len - vq->vhost_hlen;
+	vb_avail = buf_vec[vec_idx].buf_len - vq->vhost_hlen;
 
 	entry_len = vq->vhost_hlen;
 
 	if (vb_avail == 0) {
-		uint32_t desc_idx = vq->buf_vec[vec_idx].desc_idx;
+		uint32_t desc_idx = buf_vec[vec_idx].desc_idx;
 
 		if ((vq->desc[desc_idx].flags & VRING_DESC_F_NEXT) == 0) {
 			idx = cur_idx & (vq->size - 1);
 
 			/* Update used ring with desc information */
-			vq->used->ring[idx].id = vq->buf_vec[vec_idx].desc_idx;
+			vq->used->ring[idx].id = buf_vec[vec_idx].desc_idx;
 			vq->used->ring[idx].len = entry_len;
 
 			vhost_log_used_vring(dev, vq,
@@ -416,12 +416,12 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 		}
 
 		vec_idx++;
-		vb_addr = gpa_to_vva(dev, vq->buf_vec[vec_idx].buf_addr);
+		vb_addr = gpa_to_vva(dev, buf_vec[vec_idx].buf_addr);
 
 		/* Prefetch buffer address. */
 		rte_prefetch0((void *)(uintptr_t)vb_addr);
 		vb_offset = 0;
-		vb_avail = vq->buf_vec[vec_idx].buf_len;
+		vb_avail = buf_vec[vec_idx].buf_len;
 	}
 
 	cpy_len = RTE_MIN(vb_avail, seg_avail);
@@ -431,7 +431,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 		rte_memcpy((void *)(uintptr_t)(vb_addr + vb_offset),
 			rte_pktmbuf_mtod_offset(pkt, const void *, seg_offset),
 			cpy_len);
-		vhost_log_write(dev, vq->buf_vec[vec_idx].buf_addr + vb_offset,
+		vhost_log_write(dev, buf_vec[vec_idx].buf_addr + vb_offset,
 			cpy_len);
 
 		PRINT_PACKET(dev,
@@ -450,12 +450,12 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 			 * entry reach to its end.
 			 * But the segment doesn't complete.
 			 */
-			if ((vq->desc[vq->buf_vec[vec_idx].desc_idx].flags &
+			if ((vq->desc[buf_vec[vec_idx].desc_idx].flags &
 				VRING_DESC_F_NEXT) == 0) {
 				/* Update used ring with desc information */
 				idx = cur_idx & (vq->size - 1);
 				vq->used->ring[idx].id
-					= vq->buf_vec[vec_idx].desc_idx;
+					= buf_vec[vec_idx].desc_idx;
 				vq->used->ring[idx].len = entry_len;
 				vhost_log_used_vring(dev, vq,
 					offsetof(struct vring_used, ring[idx]),
@@ -467,9 +467,9 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 
 			vec_idx++;
 			vb_addr = gpa_to_vva(dev,
-				vq->buf_vec[vec_idx].buf_addr);
+				buf_vec[vec_idx].buf_addr);
 			vb_offset = 0;
-			vb_avail = vq->buf_vec[vec_idx].buf_len;
+			vb_avail = buf_vec[vec_idx].buf_len;
 			cpy_len = RTE_MIN(vb_avail, seg_avail);
 		} else {
 			/*
@@ -488,7 +488,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 					 * from buf_vec.
 					 */
 					uint32_t desc_idx =
-						vq->buf_vec[vec_idx].desc_idx;
+						buf_vec[vec_idx].desc_idx;
 
 					if ((vq->desc[desc_idx].flags &
 						VRING_DESC_F_NEXT) == 0) {
@@ -512,9 +512,9 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 					/* Get next buffer from buf_vec. */
 					vec_idx++;
 					vb_addr = gpa_to_vva(dev,
-						vq->buf_vec[vec_idx].buf_addr);
+						buf_vec[vec_idx].buf_addr);
 					vb_avail =
-						vq->buf_vec[vec_idx].buf_len;
+						buf_vec[vec_idx].buf_len;
 					vb_offset = 0;
 				}
 
@@ -528,7 +528,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 				/* Update used ring with desc information */
 				idx = cur_idx & (vq->size - 1);
 				vq->used->ring[idx].id
-					= vq->buf_vec[vec_idx].desc_idx;
+					= buf_vec[vec_idx].desc_idx;
 				vq->used->ring[idx].len = entry_len;
 				vhost_log_used_vring(dev, vq,
 					offsetof(struct vring_used, ring[idx]),
@@ -544,7 +544,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
 
 static inline void __attribute__((always_inline))
 update_secure_len(struct vhost_virtqueue *vq, uint32_t id,
-	uint32_t *secure_len, uint32_t *vec_idx)
+	uint32_t *secure_len, uint32_t *vec_idx, struct buf_vector *buf_vec)
 {
 	uint16_t wrapped_idx = id & (vq->size - 1);
 	uint32_t idx = vq->avail->ring[wrapped_idx];
@@ -555,9 +555,9 @@ update_secure_len(struct vhost_virtqueue *vq, uint32_t id,
 	do {
 		next_desc = 0;
 		len += vq->desc[idx].len;
-		vq->buf_vec[vec_id].buf_addr = vq->desc[idx].addr;
-		vq->buf_vec[vec_id].buf_len = vq->desc[idx].len;
-		vq->buf_vec[vec_id].desc_idx = idx;
+		buf_vec[vec_id].buf_addr = vq->desc[idx].addr;
+		buf_vec[vec_id].buf_len = vq->desc[idx].len;
+		buf_vec[vec_id].desc_idx = idx;
 		vec_id++;
 
 		if (vq->desc[idx].flags & VRING_DESC_F_NEXT) {
@@ -582,6 +582,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 	uint16_t avail_idx;
 	uint16_t res_base_idx, res_cur_idx;
 	uint8_t success = 0;
+	struct buf_vector buf_vec[BUF_VECTOR_MAX];
 
 	LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_merge_rx()\n",
 		dev->device_fh);
@@ -620,8 +621,8 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 				if (unlikely(res_cur_idx == avail_idx))
 					goto merge_rx_exit;
 
-				update_secure_len(vq, res_cur_idx,
-						  &secure_len, &vec_idx);
+				update_secure_len(vq, res_cur_idx, &secure_len,
+						  &vec_idx, buf_vec);
 				res_cur_idx++;
 			} while (pkt_len > secure_len);
 
@@ -632,7 +633,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 		} while (success == 0);
 
 		entry_success = copy_from_mbuf_to_vring(dev, queue_id,
-			res_base_idx, res_cur_idx, pkts[pkt_idx]);
+			res_base_idx, res_cur_idx, pkts[pkt_idx], buf_vec);
 
 		rte_smp_wmb();
 
-- 
2.5.0

[dpdk-dev] [PATCH RFC v3 3/3] vhost: avoid reordering of used->idx and last_used_idx updating.

[Ilya Maximets]

Calling rte_vhost_enqueue_burst() simultaneously from different threads
for the same queue_id requires additional SMP memory barrier to avoid
reordering of used->idx and last_used_idx updates.

In case of virtio_dev_rx() memory barrier rte_mb() simply moved one
instruction higher.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/librte_vhost/vhost_rxtx.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index a8e2582..4d37aa3 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -323,13 +323,16 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 		rte_pause();
 
 	*(volatile uint16_t *)&vq->used->idx += count;
-	vq->last_used_idx = res_end_idx;
 	vhost_log_used_vring(dev, vq,
 		offsetof(struct vring_used, idx),
 		sizeof(vq->used->idx));
 
-	/* flush used->idx update before we read avail->flags. */
+	/*
+	 * Flush used->idx update to make it visible to virtio and all other
+	 * threads before allowing to modify it.
+	 */
 	rte_mb();
+	vq->last_used_idx = res_end_idx;
 
 	/* Kick the guest if necessary. */
 	if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
@@ -645,19 +648,24 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 			rte_pause();
 
 		*(volatile uint16_t *)&vq->used->idx += entry_success;
+		/*
+		 * Flush used->idx update to make it visible to all
+		 * other threads before allowing to modify it.
+		 */
+		rte_smp_wmb();
+
 		vq->last_used_idx = res_cur_idx;
 	}
 
 merge_rx_exit:
 	if (likely(pkt_idx)) {
-		/* flush used->idx update before we read avail->flags. */
+		/* Flush used->idx update to make it visible to virtio. */
 		rte_mb();
 
 		/* Kick the guest if necessary. */
 		if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
 			eventfd_write(vq->callfd, (eventfd_t)1);
 	}
-
 	return pkt_idx;
 }
 
-- 
2.5.0

Re: [dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[Thomas Monjalon]

2016-02-24 14:47, Ilya Maximets:
> Implementation of rte_vhost_enqueue_burst() based on lockless ring-buffer
> algorithm and contains almost all to be thread-safe, but it's not.
> 
> This set adds required changes.
> 
> First patch in set is a standalone patch that fixes many times discussed
> issue with barriers on different architectures.
> 
> Second and third adds fixes to make rte_vhost_enqueue_burst thread safe.

My understanding is that we do not want to pollute Rx/Tx with locks.

Huawei, Yuanhan, Bruce, do you confirm?

Re: [dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[Yuanhan Liu]

On Thu, Mar 17, 2016 at 04:29:32PM +0100, Thomas Monjalon wrote:
> 2016-02-24 14:47, Ilya Maximets:
> > Implementation of rte_vhost_enqueue_burst() based on lockless ring-buffer
> > algorithm and contains almost all to be thread-safe, but it's not.
> > 
> > This set adds required changes.
> > 
> > First patch in set is a standalone patch that fixes many times discussed
> > issue with barriers on different architectures.
> > 
> > Second and third adds fixes to make rte_vhost_enqueue_burst thread safe.
> 
> My understanding is that we do not want to pollute Rx/Tx with locks.
> 
> Huawei, Yuanhan, Bruce, do you confirm?

Huawei would like to do that, and I'm behind that. Let's do it. The
question is can we do that in this release? As I replied in another
thread, I'm wondering we might need do an announce first and do it
in next release?

Both are Okay to me; I just want to know which one is more proper.

Thoughts?

	--yliu

Re: [dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[Thomas Monjalon]

2016-03-18 16:00, Yuanhan Liu:
> On Thu, Mar 17, 2016 at 04:29:32PM +0100, Thomas Monjalon wrote:
> > 2016-02-24 14:47, Ilya Maximets:
> > > Implementation of rte_vhost_enqueue_burst() based on lockless ring-buffer
> > > algorithm and contains almost all to be thread-safe, but it's not.
> > > 
> > > This set adds required changes.
> > > 
> > > First patch in set is a standalone patch that fixes many times discussed
> > > issue with barriers on different architectures.
> > > 
> > > Second and third adds fixes to make rte_vhost_enqueue_burst thread safe.
> > 
> > My understanding is that we do not want to pollute Rx/Tx with locks.
> > 
> > Huawei, Yuanhan, Bruce, do you confirm?
> 
> Huawei would like to do that, and I'm behind that. Let's do it.

I'm not sure to understand. What do you want to do exactly?

> The question is can we do that in this release? As I replied in another
> thread, I'm wondering we might need do an announce first and do it
> in next release?
> 
> Both are Okay to me; I just want to know which one is more proper.
> 
> Thoughts?

Re: [dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[Yuanhan Liu]

On Fri, Mar 18, 2016 at 09:09:04AM +0100, Thomas Monjalon wrote:
> 2016-03-18 16:00, Yuanhan Liu:
> > On Thu, Mar 17, 2016 at 04:29:32PM +0100, Thomas Monjalon wrote:
> > > 2016-02-24 14:47, Ilya Maximets:
> > > > Implementation of rte_vhost_enqueue_burst() based on lockless ring-buffer
> > > > algorithm and contains almost all to be thread-safe, but it's not.
> > > > 
> > > > This set adds required changes.
> > > > 
> > > > First patch in set is a standalone patch that fixes many times discussed
> > > > issue with barriers on different architectures.
> > > > 
> > > > Second and third adds fixes to make rte_vhost_enqueue_burst thread safe.
> > > 
> > > My understanding is that we do not want to pollute Rx/Tx with locks.
> > > 
> > > Huawei, Yuanhan, Bruce, do you confirm?
> > 
> > Huawei would like to do that, and I'm behind that. Let's do it.
> 
> I'm not sure to understand. What do you want to do exactly?

I was thinking we are on the same page :(

"do not want to pollute Rx/Tx with locks" == "remove lockless Rx/Tx, the
proposal from Huawei", right?

In another way, I'm behind the following patch from Huawei:

    http://dpdk.org/dev/patchwork/patch/9740/

	--yliu

Re: [dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[Thomas Monjalon]

2016-03-18 17:16, Yuanhan Liu:
> On Fri, Mar 18, 2016 at 09:09:04AM +0100, Thomas Monjalon wrote:
> > 2016-03-18 16:00, Yuanhan Liu:
> > > On Thu, Mar 17, 2016 at 04:29:32PM +0100, Thomas Monjalon wrote:
> > > > 2016-02-24 14:47, Ilya Maximets:
> > > > > Implementation of rte_vhost_enqueue_burst() based on lockless ring-buffer
> > > > > algorithm and contains almost all to be thread-safe, but it's not.
> > > > > 
> > > > > This set adds required changes.
> > > > > 
> > > > > First patch in set is a standalone patch that fixes many times discussed
> > > > > issue with barriers on different architectures.
> > > > > 
> > > > > Second and third adds fixes to make rte_vhost_enqueue_burst thread safe.
> > > > 
> > > > My understanding is that we do not want to pollute Rx/Tx with locks.
> > > > 
> > > > Huawei, Yuanhan, Bruce, do you confirm?
> > > 
> > > Huawei would like to do that, and I'm behind that. Let's do it.
> > 
> > I'm not sure to understand. What do you want to do exactly?
> 
> I was thinking we are on the same page :(

Yes we are on the same page.
But it's better to make things explicit.

There should be no lock in Rx/Tx drivers (including vhost).
The locking must be done by the caller (application level).
That's why this series "Thread safe rte_vhost_enqueue_burst" is rejected.

> "do not want to pollute Rx/Tx with locks" == "remove lockless Rx/Tx, the
> proposal from Huawei", right?
> 
> In another way, I'm behind the following patch from Huawei:
> 
>     http://dpdk.org/dev/patchwork/patch/9740/

The patch "vhost: remove lockless enqueue to the virtio ring" must me
reworked in 2 patches:
1/ announce API change
2/ remove locks

Please avoid talking about removing "lockless" when actually removing locks.

Thanks

Re: [dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[Yuanhan Liu]

On Fri, Mar 18, 2016 at 10:34:56AM +0100, Thomas Monjalon wrote:
> 2016-03-18 17:16, Yuanhan Liu:
> > On Fri, Mar 18, 2016 at 09:09:04AM +0100, Thomas Monjalon wrote:
> > > 2016-03-18 16:00, Yuanhan Liu:
> > > > On Thu, Mar 17, 2016 at 04:29:32PM +0100, Thomas Monjalon wrote:
> > > > > 2016-02-24 14:47, Ilya Maximets:
> > > > > > Implementation of rte_vhost_enqueue_burst() based on lockless ring-buffer
> > > > > > algorithm and contains almost all to be thread-safe, but it's not.
> > > > > > 
> > > > > > This set adds required changes.
> > > > > > 
> > > > > > First patch in set is a standalone patch that fixes many times discussed
> > > > > > issue with barriers on different architectures.
> > > > > > 
> > > > > > Second and third adds fixes to make rte_vhost_enqueue_burst thread safe.
> > > > > 
> > > > > My understanding is that we do not want to pollute Rx/Tx with locks.
> > > > > 
> > > > > Huawei, Yuanhan, Bruce, do you confirm?
> > > > 
> > > > Huawei would like to do that, and I'm behind that. Let's do it.
> > > 
> > > I'm not sure to understand. What do you want to do exactly?
> > 
> > I was thinking we are on the same page :(
> 
> Yes we are on the same page.
> But it's better to make things explicit.
> 
> There should be no lock in Rx/Tx drivers (including vhost).
> The locking must be done by the caller (application level).

Yeah, that's why Huawei made the proposal and the patch.

> That's why this series "Thread safe rte_vhost_enqueue_burst" is rejected.
> 
> > "do not want to pollute Rx/Tx with locks" == "remove lockless Rx/Tx, the
> > proposal from Huawei", right?
> > 
> > In another way, I'm behind the following patch from Huawei:
> > 
> >     http://dpdk.org/dev/patchwork/patch/9740/
> 
> The patch "vhost: remove lockless enqueue to the virtio ring" must me
> reworked in 2 patches:
> 1/ announce API change

That was my concern, and agreed we need that.

> 2/ remove locks
> 
> Please avoid talking about removing "lockless" when actually removing locks.

Sorry, my bad.

	--yliu

Re: [dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[Ilya Maximets]

On 18.03.2016 12:46, Yuanhan Liu wrote:
> On Fri, Mar 18, 2016 at 10:34:56AM +0100, Thomas Monjalon wrote:
>> 2016-03-18 17:16, Yuanhan Liu:
>>> On Fri, Mar 18, 2016 at 09:09:04AM +0100, Thomas Monjalon wrote:
>>>> 2016-03-18 16:00, Yuanhan Liu:
>>>>> On Thu, Mar 17, 2016 at 04:29:32PM +0100, Thomas Monjalon wrote:
>>>>>> 2016-02-24 14:47, Ilya Maximets:
>>>>>>> Implementation of rte_vhost_enqueue_burst() based on lockless ring-buffer
>>>>>>> algorithm and contains almost all to be thread-safe, but it's not.
>>>>>>>
>>>>>>> This set adds required changes.
>>>>>>>
>>>>>>> First patch in set is a standalone patch that fixes many times discussed
>>>>>>> issue with barriers on different architectures.
>>>>>>>
>>>>>>> Second and third adds fixes to make rte_vhost_enqueue_burst thread safe.
>>>>>>
>>>>>> My understanding is that we do not want to pollute Rx/Tx with locks.
>>>>>>
>>>>>> Huawei, Yuanhan, Bruce, do you confirm?
>>>>>
>>>>> Huawei would like to do that, and I'm behind that. Let's do it.
>>>>
>>>> I'm not sure to understand. What do you want to do exactly?
>>>
>>> I was thinking we are on the same page :(
>>
>> Yes we are on the same page.
>> But it's better to make things explicit.
>>
>> There should be no lock in Rx/Tx drivers (including vhost).
>> The locking must be done by the caller (application level).
> 
> Yeah, that's why Huawei made the proposal and the patch.
> 
>> That's why this series "Thread safe rte_vhost_enqueue_burst" is rejected.

Hi all.
And what about first patch of this series:
"vhost: use SMP barriers instead of compiler ones." ?

It's not about thread safety in terms of 'lockless'. It is the standalone
patch that fixes many times discussed issue with barriers on arm.

Best regards, Ilya Maximets.

>>
>>> "do not want to pollute Rx/Tx with locks" == "remove lockless Rx/Tx, the
>>> proposal from Huawei", right?
>>>
>>> In another way, I'm behind the following patch from Huawei:
>>>
>>>     http://dpdk.org/dev/patchwork/patch/9740/
>>
>> The patch "vhost: remove lockless enqueue to the virtio ring" must me
>> reworked in 2 patches:
>> 1/ announce API change
> 
> That was my concern, and agreed we need that.
> 
>> 2/ remove locks
>>
>> Please avoid talking about removing "lockless" when actually removing locks.
> 
> Sorry, my bad.
> 
> 	--yliu
> 
> 

Re: [dpdk-dev] [PATCH RFC v3 1/3] vhost: use SMP barriers instead of compiler ones.

[Xie, Huawei]

On 2/24/2016 7:47 PM, Ilya Maximets wrote:
>  		 * Wait until it's our turn to add our buffer
> @@ -979,7 +979,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>  		entry_success++;
>  	}
>  
> -	rte_compiler_barrier();
> +	rte_smp_rmb();

smp_rmb()?

>  	vq->used->idx += entry_success;
>  	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
>  			sizeof(vq->used->idx));
> -- 2.5.0

Re: [dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[Xie, Huawei]

On 3/18/2016 5:55 PM, Ilya Maximets wrote:
> Hi all.
> And what about first patch of this series:
> "vhost: use SMP barriers instead of compiler ones." ?
>
> It's not about thread safety in terms of 'lockless'. It is the standalone
> patch that fixes many times discussed issue with barriers on arm.
>
> Best regards, Ilya Maximets.
Right, for ARM. I put a comment there. Btw, could you add -in-reply-to
when you send you next patch?

Re: [dpdk-dev] [PATCH RFC v3 1/3] vhost: use SMP barriers instead of compiler ones.

[Ilya Maximets]

On 18.03.2016 13:08, Xie, Huawei wrote:
> On 2/24/2016 7:47 PM, Ilya Maximets wrote:
>>  		 * Wait until it's our turn to add our buffer
>> @@ -979,7 +979,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>>  		entry_success++;
>>  	}
>>  
>> -	rte_compiler_barrier();
>> +	rte_smp_rmb();
> 
> smp_rmb()?

There is no such function 'smp_rmb' in DPDK.
But:
.../arch/arm/rte_atomic.h:#define rte_smp_rmb() rte_rmb()
.../arch/ppc_64/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
.../arch/tile/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
.../arch/x86/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()

> 
>>  	vq->used->idx += entry_success;
>>  	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
>>  			sizeof(vq->used->idx));
>> -- 2.5.0
> 
> 
> 

Re: [dpdk-dev] [PATCH RFC v3 0/3] Thread safe rte_vhost_enqueue_burst().

[Thomas Monjalon]

2016-03-18 10:10, Xie, Huawei:
> On 3/18/2016 5:55 PM, Ilya Maximets wrote:
> > Hi all.
> > And what about first patch of this series:
> > "vhost: use SMP barriers instead of compiler ones." ?
> >
> > It's not about thread safety in terms of 'lockless'. It is the standalone
> > patch that fixes many times discussed issue with barriers on arm.
> >
> > Best regards, Ilya Maximets.
> Right, for ARM. I put a comment there. Btw, could you add -in-reply-to
> when you send you next patch?

Please Ilya, send it alone when you'll have an ack from a vhost maintainer.

Re: [dpdk-dev] [PATCH RFC v3 1/3] vhost: use SMP barriers instead of compiler ones.

[Xie, Huawei]

On 3/18/2016 6:23 PM, Ilya Maximets wrote:
> On 18.03.2016 13:08, Xie, Huawei wrote:
>> On 2/24/2016 7:47 PM, Ilya Maximets wrote:
>>>  		 * Wait until it's our turn to add our buffer
>>> @@ -979,7 +979,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>>>  		entry_success++;
>>>  	}
>>>  
>>> -	rte_compiler_barrier();
>>> +	rte_smp_rmb();
>> smp_rmb()?
> There is no such function 'smp_rmb' in DPDK.
> But:
> .../arch/arm/rte_atomic.h:#define rte_smp_rmb() rte_rmb()
> .../arch/ppc_64/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
> .../arch/tile/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
> .../arch/x86/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()

I mean shoudn't be rte_smp_wmb()?

>
>>>  	vq->used->idx += entry_success;
>>>  	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
>>>  			sizeof(vq->used->idx));
>>> -- 2.5.0
>>
>>

Re: [dpdk-dev] [PATCH RFC v3 1/3] vhost: use SMP barriers instead of compiler ones.

[Ilya Maximets]

On 18.03.2016 13:27, Xie, Huawei wrote:
> On 3/18/2016 6:23 PM, Ilya Maximets wrote:
>> On 18.03.2016 13:08, Xie, Huawei wrote:
>>> On 2/24/2016 7:47 PM, Ilya Maximets wrote:
>>>>  		 * Wait until it's our turn to add our buffer
>>>> @@ -979,7 +979,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>>>>  		entry_success++;
>>>>  	}
>>>>  
>>>> -	rte_compiler_barrier();
>>>> +	rte_smp_rmb();
>>> smp_rmb()?
>> There is no such function 'smp_rmb' in DPDK.
>> But:
>> .../arch/arm/rte_atomic.h:#define rte_smp_rmb() rte_rmb()
>> .../arch/ppc_64/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>> .../arch/tile/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>> .../arch/x86/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
> 
> I mean shoudn't be rte_smp_wmb()?

No. Here we need to be sure that copying of data from descriptor to
our local mbuf completed before 'vq->used->idx += entry_success;'.

Read memory barrier will help us with it.

In other places write barriers used because copying performed in
opposite direction.

> 
>>
>>>>  	vq->used->idx += entry_success;
>>>>  	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
>>>>  			sizeof(vq->used->idx));
>>>> -- 2.5.0
>>>
>>>
> 
> 
> 

Re: [dpdk-dev] [PATCH RFC v3 1/3] vhost: use SMP barriers instead of compiler ones.

[Xie, Huawei]

On 3/18/2016 6:39 PM, Ilya Maximets wrote:
>
> On 18.03.2016 13:27, Xie, Huawei wrote:
>> On 3/18/2016 6:23 PM, Ilya Maximets wrote:
>>> On 18.03.2016 13:08, Xie, Huawei wrote:
>>>> On 2/24/2016 7:47 PM, Ilya Maximets wrote:
>>>>>  		 * Wait until it's our turn to add our buffer
>>>>> @@ -979,7 +979,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>>>>>  		entry_success++;
>>>>>  	}
>>>>>  
>>>>> -	rte_compiler_barrier();
>>>>> +	rte_smp_rmb();
>>>> smp_rmb()?
>>> There is no such function 'smp_rmb' in DPDK.
>>> But:
>>> .../arch/arm/rte_atomic.h:#define rte_smp_rmb() rte_rmb()
>>> .../arch/ppc_64/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>>> .../arch/tile/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>>> .../arch/x86/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>> I mean shoudn't be rte_smp_wmb()?
> No. Here we need to be sure that copying of data from descriptor to
> our local mbuf completed before 'vq->used->idx += entry_success;'.
>
> Read memory barrier will help us with it.
>
> In other places write barriers used because copying performed in
> opposite direction.

What about the udpate to the used ring?


>
>>>>>  	vq->used->idx += entry_success;
>>>>>  	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
>>>>>  			sizeof(vq->used->idx));
>>>>> -- 2.5.0
>>>>
>>
>>

Re: [dpdk-dev] [PATCH RFC v3 1/3] vhost: use SMP barriers instead of compiler ones.

[Ilya Maximets]

On 18.03.2016 13:47, Xie, Huawei wrote:
> On 3/18/2016 6:39 PM, Ilya Maximets wrote:
>>
>> On 18.03.2016 13:27, Xie, Huawei wrote:
>>> On 3/18/2016 6:23 PM, Ilya Maximets wrote:
>>>> On 18.03.2016 13:08, Xie, Huawei wrote:
>>>>> On 2/24/2016 7:47 PM, Ilya Maximets wrote:
>>>>>>  		 * Wait until it's our turn to add our buffer
>>>>>> @@ -979,7 +979,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>>>>>>  		entry_success++;
>>>>>>  	}
>>>>>>  
>>>>>> -	rte_compiler_barrier();
>>>>>> +	rte_smp_rmb();
>>>>> smp_rmb()?
>>>> There is no such function 'smp_rmb' in DPDK.
>>>> But:
>>>> .../arch/arm/rte_atomic.h:#define rte_smp_rmb() rte_rmb()
>>>> .../arch/ppc_64/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>>>> .../arch/tile/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>>>> .../arch/x86/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>>> I mean shoudn't be rte_smp_wmb()?
>> No. Here we need to be sure that copying of data from descriptor to
>> our local mbuf completed before 'vq->used->idx += entry_success;'.
>>
>> Read memory barrier will help us with it.
>>
>> In other places write barriers used because copying performed in
>> opposite direction.
> 
> What about the udpate to the used ring?

Next line is the only place where this vq->used->idx accessed.
So, there must be no issues.

>>>>>>  	vq->used->idx += entry_success;
>>>>>>  	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
>>>>>>  			sizeof(vq->used->idx));
>>>>>> -- 2.5.0
>>>>>
>>>
>>>
> 
> 
> 

[dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.

[Ilya Maximets]

Since commit 4c02e453cc62 ("eal: introduce SMP memory barriers") virtio
uses architecture dependent SMP barriers. vHost should use them too.

Fixes: 4c02e453cc62 ("eal: introduce SMP memory barriers")

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/librte_vhost/vhost_rxtx.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index b4da665..859c669 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -315,7 +315,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 			rte_prefetch0(&vq->desc[desc_indexes[i+1]]);
 	}
 
-	rte_compiler_barrier();
+	rte_smp_wmb();
 
 	/* Wait until it's our turn to add our buffer to the used ring. */
 	while (unlikely(vq->last_used_idx != res_start_idx))
@@ -565,7 +565,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 
 		nr_used = copy_mbuf_to_desc_mergeable(dev, vq, start, end,
 						      pkts[pkt_idx]);
-		rte_compiler_barrier();
+		rte_smp_wmb();
 
 		/*
 		 * Wait until it's our turn to add our buffer
@@ -923,7 +923,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
 				sizeof(vq->used->ring[used_idx]));
 	}
 
-	rte_compiler_barrier();
+	rte_smp_wmb();
+	rte_smp_rmb();
 	vq->used->idx += i;
 	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
 			sizeof(vq->used->idx));
-- 
2.5.0

Re: [dpdk-dev] [PATCH RFC v3 1/3] vhost: use SMP barriers instead of compiler ones.

[Ilya Maximets]

CC to list.

On 18.03.2016 15:19, Ilya Maximets wrote:
> 
> 
> On 18.03.2016 14:42, Ilya Maximets wrote:
>> On 18.03.2016 14:30, Xie, Huawei wrote:
>>> On 3/18/2016 7:00 PM, Ilya Maximets wrote:
>>>> On 18.03.2016 13:47, Xie, Huawei wrote:
>>>>> On 3/18/2016 6:39 PM, Ilya Maximets wrote:
>>>>>> On 18.03.2016 13:27, Xie, Huawei wrote:
>>>>>>> On 3/18/2016 6:23 PM, Ilya Maximets wrote:
>>>>>>>> On 18.03.2016 13:08, Xie, Huawei wrote:
>>>>>>>>> On 2/24/2016 7:47 PM, Ilya Maximets wrote:
>>>>>>>>>>  		 * Wait until it's our turn to add our buffer
>>>>>>>>>> @@ -979,7 +979,7 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>>>>>>>>>>  		entry_success++;
>>>>>>>>>>  	}
>>>>>>>>>>  
>>>>>>>>>> -	rte_compiler_barrier();
>>>>>>>>>> +	rte_smp_rmb();
>>>>>>>>> smp_rmb()?
>>>>>>>> There is no such function 'smp_rmb' in DPDK.
>>>>>>>> But:
>>>>>>>> .../arch/arm/rte_atomic.h:#define rte_smp_rmb() rte_rmb()
>>>>>>>> .../arch/ppc_64/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>>>>>>>> .../arch/tile/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>>>>>>>> .../arch/x86/rte_atomic.h:#define rte_smp_rmb() rte_compiler_barrier()
>>>>>>> I mean shoudn't be rte_smp_wmb()?
>>>>>> No. Here we need to be sure that copying of data from descriptor to
>>>>>> our local mbuf completed before 'vq->used->idx += entry_success;'.
>>>>>>
>>>>>> Read memory barrier will help us with it.
>>>>>>
>>>>>> In other places write barriers used because copying performed in
>>>>>> opposite direction.
>>>>> What about the udpate to the used ring?
>>>> Next line is the only place where this vq->used->idx accessed.
>>>> So, there must be no issues.
>>>
>>> The update to the used ring entries, happened before the update to the
>>> used ring index.
>>
>> Oh. Sorry. In that case we need full barrier here because we need reads and
>> writes both completed before updating of used->idx:
>> 	rte_smp_mb();
> 
> Hmmm.. But as I see, rte_smp_mb is a real mm_fence on x86. May be the pair
> of barriers will be better here:
> 	rte_smp_rmb();
> 	rte_smp_wmb();
> 
> It is normal because next increment uses read and write. So, we don't need to
> synchronize reads with writes here.
> 
>>
>>>
>>>>
>>>>>>>>>>  	vq->used->idx += entry_success;
>>>>>>>>>>  	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
>>>>>>>>>>  			sizeof(vq->used->idx));
>>>>>>>>>> -- 2.5.0
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>>>

Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.

[Yuanhan Liu]

On Fri, Mar 18, 2016 at 03:23:53PM +0300, Ilya Maximets wrote:
> Since commit 4c02e453cc62 ("eal: introduce SMP memory barriers") virtio
> uses architecture dependent SMP barriers. vHost should use them too.
> 
> Fixes: 4c02e453cc62 ("eal: introduce SMP memory barriers")
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  lib/librte_vhost/vhost_rxtx.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> index b4da665..859c669 100644
> --- a/lib/librte_vhost/vhost_rxtx.c
> +++ b/lib/librte_vhost/vhost_rxtx.c
> @@ -315,7 +315,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
>  			rte_prefetch0(&vq->desc[desc_indexes[i+1]]);
>  	}
>  
> -	rte_compiler_barrier();
> +	rte_smp_wmb();
>  
>  	/* Wait until it's our turn to add our buffer to the used ring. */
>  	while (unlikely(vq->last_used_idx != res_start_idx))
> @@ -565,7 +565,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
>  
>  		nr_used = copy_mbuf_to_desc_mergeable(dev, vq, start, end,
>  						      pkts[pkt_idx]);
> -		rte_compiler_barrier();
> +		rte_smp_wmb();
>  
>  		/*
>  		 * Wait until it's our turn to add our buffer
> @@ -923,7 +923,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>  				sizeof(vq->used->ring[used_idx]));
>  	}
>  
> -	rte_compiler_barrier();
> +	rte_smp_wmb();
> +	rte_smp_rmb();

rte_smp_mb?

	--yliu

Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.

[Ilya Maximets]

On 18.03.2016 15:41, Yuanhan Liu wrote:
> On Fri, Mar 18, 2016 at 03:23:53PM +0300, Ilya Maximets wrote:
>> Since commit 4c02e453cc62 ("eal: introduce SMP memory barriers") virtio
>> uses architecture dependent SMP barriers. vHost should use them too.
>>
>> Fixes: 4c02e453cc62 ("eal: introduce SMP memory barriers")
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>>  lib/librte_vhost/vhost_rxtx.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
>> index b4da665..859c669 100644
>> --- a/lib/librte_vhost/vhost_rxtx.c
>> +++ b/lib/librte_vhost/vhost_rxtx.c
>> @@ -315,7 +315,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
>>  			rte_prefetch0(&vq->desc[desc_indexes[i+1]]);
>>  	}
>>  
>> -	rte_compiler_barrier();
>> +	rte_smp_wmb();
>>  
>>  	/* Wait until it's our turn to add our buffer to the used ring. */
>>  	while (unlikely(vq->last_used_idx != res_start_idx))
>> @@ -565,7 +565,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
>>  
>>  		nr_used = copy_mbuf_to_desc_mergeable(dev, vq, start, end,
>>  						      pkts[pkt_idx]);
>> -		rte_compiler_barrier();
>> +		rte_smp_wmb();
>>  
>>  		/*
>>  		 * Wait until it's our turn to add our buffer
>> @@ -923,7 +923,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>>  				sizeof(vq->used->ring[used_idx]));
>>  	}
>>  
>> -	rte_compiler_barrier();
>> +	rte_smp_wmb();
>> +	rte_smp_rmb();
> 
> rte_smp_mb?

rte_smp_mb() is a real mm_fence() on x86. And we don't need to synchronize reads with
writes here, only reads with reads and writes with writes. It is enough because next
increment uses read and write. Pair of barriers is better because it will not impact
on performance on x86.

Best regards, Ilya Maximets.

Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya Maximets
> Sent: Monday, March 21, 2016 4:50 AM
> To: Yuanhan Liu
> Cc: dev@dpdk.org; Xie, Huawei; Dyasly Sergey
> Subject: Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.
> 
> 
> 
> On 18.03.2016 15:41, Yuanhan Liu wrote:
> > On Fri, Mar 18, 2016 at 03:23:53PM +0300, Ilya Maximets wrote:
> >> Since commit 4c02e453cc62 ("eal: introduce SMP memory barriers") virtio
> >> uses architecture dependent SMP barriers. vHost should use them too.
> >>
> >> Fixes: 4c02e453cc62 ("eal: introduce SMP memory barriers")
> >>
> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >> ---
> >>  lib/librte_vhost/vhost_rxtx.c | 7 ++++---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> >> index b4da665..859c669 100644
> >> --- a/lib/librte_vhost/vhost_rxtx.c
> >> +++ b/lib/librte_vhost/vhost_rxtx.c
> >> @@ -315,7 +315,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
> >>  			rte_prefetch0(&vq->desc[desc_indexes[i+1]]);
> >>  	}
> >>
> >> -	rte_compiler_barrier();
> >> +	rte_smp_wmb();
> >>
> >>  	/* Wait until it's our turn to add our buffer to the used ring. */
> >>  	while (unlikely(vq->last_used_idx != res_start_idx))
> >> @@ -565,7 +565,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
> >>
> >>  		nr_used = copy_mbuf_to_desc_mergeable(dev, vq, start, end,
> >>  						      pkts[pkt_idx]);
> >> -		rte_compiler_barrier();
> >> +		rte_smp_wmb();
> >>
> >>  		/*
> >>  		 * Wait until it's our turn to add our buffer
> >> @@ -923,7 +923,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
> >>  				sizeof(vq->used->ring[used_idx]));
> >>  	}
> >>
> >> -	rte_compiler_barrier();
> >> +	rte_smp_wmb();
> >> +	rte_smp_rmb();
> >
> > rte_smp_mb?
> 
> rte_smp_mb() is a real mm_fence() on x86. And we don't need to synchronize reads with
> writes here, only reads with reads and writes with writes. It is enough because next
> increment uses read and write. Pair of barriers is better because it will not impact
> on performance on x86.

Not arguing about that particular patch, just a question:
Why do we have:
#define rte_smp_mb() rte_mb()
for  x86?
Why not just:
#define rte_smp_mb() rte_compiler_barrier()
here?
I meant for situations when we do need real mfence, there is an 'rte_mb' to use.
Konstantin

> 
> Best regards, Ilya Maximets.

Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.

[Xie, Huawei]

On 3/21/2016 10:07 PM, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya Maximets
>> Sent: Monday, March 21, 2016 4:50 AM
>> To: Yuanhan Liu
>> Cc: dev@dpdk.org; Xie, Huawei; Dyasly Sergey
>> Subject: Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.
>>
>>
>>
>> On 18.03.2016 15:41, Yuanhan Liu wrote:
>>> On Fri, Mar 18, 2016 at 03:23:53PM +0300, Ilya Maximets wrote:
>>>> Since commit 4c02e453cc62 ("eal: introduce SMP memory barriers") virtio
>>>> uses architecture dependent SMP barriers. vHost should use them too.
>>>>
>>>> Fixes: 4c02e453cc62 ("eal: introduce SMP memory barriers")
>>>>
>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>> ---
>>>>  lib/librte_vhost/vhost_rxtx.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
>>>> index b4da665..859c669 100644
>>>> --- a/lib/librte_vhost/vhost_rxtx.c
>>>> +++ b/lib/librte_vhost/vhost_rxtx.c
>>>> @@ -315,7 +315,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
>>>>  			rte_prefetch0(&vq->desc[desc_indexes[i+1]]);
>>>>  	}
>>>>
>>>> -	rte_compiler_barrier();
>>>> +	rte_smp_wmb();
>>>>
>>>>  	/* Wait until it's our turn to add our buffer to the used ring. */
>>>>  	while (unlikely(vq->last_used_idx != res_start_idx))
>>>> @@ -565,7 +565,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
>>>>
>>>>  		nr_used = copy_mbuf_to_desc_mergeable(dev, vq, start, end,
>>>>  						      pkts[pkt_idx]);
>>>> -		rte_compiler_barrier();
>>>> +		rte_smp_wmb();
>>>>
>>>>  		/*
>>>>  		 * Wait until it's our turn to add our buffer
>>>> @@ -923,7 +923,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>>>>  				sizeof(vq->used->ring[used_idx]));
>>>>  	}
>>>>
>>>> -	rte_compiler_barrier();
>>>> +	rte_smp_wmb();
>>>> +	rte_smp_rmb();
>>> rte_smp_mb?
>> rte_smp_mb() is a real mm_fence() on x86. And we don't need to synchronize reads with
>> writes here, only reads with reads and writes with writes. It is enough because next
>> increment uses read and write. Pair of barriers is better because it will not impact
>> on performance on x86.
> Not arguing about that particular patch, just a question:
> Why do we have:
> #define rte_smp_mb() rte_mb()

Konstantine, actually smp_mb is defined as mfence while smp_r/wmb are
defined as barrier in kernel_src/arch/x86/include/asm/barrier.h.

> for  x86?
> Why not just:
> #define rte_smp_mb() rte_compiler_barrier()
> here?
> I meant for situations when we do need real mfence, there is an 'rte_mb' to use.
> Konstantin
>
>> Best regards, Ilya Maximets.

Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.

[Ananyev, Konstantin]

> -----Original Message-----
> From: Xie, Huawei
> Sent: Monday, March 21, 2016 5:26 PM
> To: Ananyev, Konstantin; Ilya Maximets; Yuanhan Liu
> Cc: dev@dpdk.org; Dyasly Sergey
> Subject: Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.
> 
> On 3/21/2016 10:07 PM, Ananyev, Konstantin wrote:
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ilya Maximets
> >> Sent: Monday, March 21, 2016 4:50 AM
> >> To: Yuanhan Liu
> >> Cc: dev@dpdk.org; Xie, Huawei; Dyasly Sergey
> >> Subject: Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.
> >>
> >>
> >>
> >> On 18.03.2016 15:41, Yuanhan Liu wrote:
> >>> On Fri, Mar 18, 2016 at 03:23:53PM +0300, Ilya Maximets wrote:
> >>>> Since commit 4c02e453cc62 ("eal: introduce SMP memory barriers") virtio
> >>>> uses architecture dependent SMP barriers. vHost should use them too.
> >>>>
> >>>> Fixes: 4c02e453cc62 ("eal: introduce SMP memory barriers")
> >>>>
> >>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >>>> ---
> >>>>  lib/librte_vhost/vhost_rxtx.c | 7 ++++---
> >>>>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> >>>> index b4da665..859c669 100644
> >>>> --- a/lib/librte_vhost/vhost_rxtx.c
> >>>> +++ b/lib/librte_vhost/vhost_rxtx.c
> >>>> @@ -315,7 +315,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
> >>>>  			rte_prefetch0(&vq->desc[desc_indexes[i+1]]);
> >>>>  	}
> >>>>
> >>>> -	rte_compiler_barrier();
> >>>> +	rte_smp_wmb();
> >>>>
> >>>>  	/* Wait until it's our turn to add our buffer to the used ring. */
> >>>>  	while (unlikely(vq->last_used_idx != res_start_idx))
> >>>> @@ -565,7 +565,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
> >>>>
> >>>>  		nr_used = copy_mbuf_to_desc_mergeable(dev, vq, start, end,
> >>>>  						      pkts[pkt_idx]);
> >>>> -		rte_compiler_barrier();
> >>>> +		rte_smp_wmb();
> >>>>
> >>>>  		/*
> >>>>  		 * Wait until it's our turn to add our buffer
> >>>> @@ -923,7 +923,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
> >>>>  				sizeof(vq->used->ring[used_idx]));
> >>>>  	}
> >>>>
> >>>> -	rte_compiler_barrier();
> >>>> +	rte_smp_wmb();
> >>>> +	rte_smp_rmb();
> >>> rte_smp_mb?
> >> rte_smp_mb() is a real mm_fence() on x86. And we don't need to synchronize reads with
> >> writes here, only reads with reads and writes with writes. It is enough because next
> >> increment uses read and write. Pair of barriers is better because it will not impact
> >> on performance on x86.
> > Not arguing about that particular patch, just a question:
> > Why do we have:
> > #define rte_smp_mb() rte_mb()
> 
> Konstantine, actually smp_mb is defined as mfence while smp_r/wmb are
> defined as barrier in kernel_src/arch/x86/include/asm/barrier.h.

I am aware of that, but who said that we should do the same?
Konstantin

> 
> > for  x86?
> > Why not just:
> > #define rte_smp_mb() rte_compiler_barrier()
> > here?
> > I meant for situations when we do need real mfence, there is an 'rte_mb' to use.
> > Konstantin
> >
> >> Best regards, Ilya Maximets.

Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.

[Xie, Huawei]

On 3/18/2016 8:24 PM, Ilya Maximets wrote:
> Since commit 4c02e453cc62 ("eal: introduce SMP memory barriers") virtio
> uses architecture dependent SMP barriers. vHost should use them too.
>
> Fixes: 4c02e453cc62 ("eal: introduce SMP memory barriers")
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

Acked-by: Huawei Xie <huawei.xie@intel.com>

It fixes the issue for other archs. Will recheck in future.

Re: [dpdk-dev] [PATCH v4] vhost: use SMP barriers instead of compiler ones.

[Thomas Monjalon]

2016-03-23 14:07, Xie, Huawei:
> On 3/18/2016 8:24 PM, Ilya Maximets wrote:
> > Since commit 4c02e453cc62 ("eal: introduce SMP memory barriers") virtio
> > uses architecture dependent SMP barriers. vHost should use them too.
> >
> > Fixes: 4c02e453cc62 ("eal: introduce SMP memory barriers")
> >
> > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> 
> Acked-by: Huawei Xie <huawei.xie@intel.com>
> 
> It fixes the issue for other archs. Will recheck in future.

Applied, thanks