DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/virtio: add platform memory ordering feature support
       [not found] <CGME20181214153817eucas1p19a41cdd791879252e1f3a5d77c427845@eucas1p1.samsung.com>
@ 2018-12-14 15:38 ` Ilya Maximets
  2018-12-14 17:00   ` Michael S. Tsirkin
       [not found]   ` <CGME20181226163717eucas1p15276eb45e35abe2c9cf3e7c1e0050823@eucas1p1.samsung.com>
  0 siblings, 2 replies; 28+ messages in thread
From: Ilya Maximets @ 2018-12-14 15:38 UTC (permalink / raw)
  To: dev, Maxime Coquelin, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, Ilya Maximets

VIRTIO_F_ORDER_PLATFORM is required to use proper memory barriers
in case of HW vhost implementations like vDPA.

DMA barriers (rte_cio_*) are sufficent for that purpose.

Previously known as VIRTIO_F_IO_BARRIER.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---

Based on "[RFC] net/virtio: use real barriers for vDPA".

RFC --> PATCH:
  * Dropped vendor-specific hack to determine if we need real barriers.
  * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.

Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
      to VIRTIO_F_ORDER_PLATFORM is not merged yet:
      https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg04114.html

 drivers/net/virtio/virtio_ethdev.c |  2 ++
 drivers/net/virtio/virtio_ethdev.h |  3 ++-
 drivers/net/virtio/virtio_pci.h    |  7 ++++++
 drivers/net/virtio/virtio_rxtx.c   | 14 ++++++------
 drivers/net/virtio/virtqueue.h     | 35 +++++++++++++++++++++++++-----
 5 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index cb2b2e0bf..5ae7a9650 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1474,6 +1474,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 	if (virtio_negotiate_features(hw, req_features) < 0)
 		return -1;
 
+	hw->weak_barriers = !vtpci_with_feature(hw, VIRTIO_F_ORDER_PLATFORM);
+
 	if (!hw->virtio_user_dev) {
 		pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
 		rte_eth_copy_pci_info(eth_dev, pci_dev);
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index e0f80e5a4..c098e5ac0 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -34,7 +34,8 @@
 	 1u << VIRTIO_RING_F_INDIRECT_DESC |    \
 	 1ULL << VIRTIO_F_VERSION_1       |	\
 	 1ULL << VIRTIO_F_IN_ORDER        |	\
-	 1ULL << VIRTIO_F_IOMMU_PLATFORM)
+	 1ULL << VIRTIO_F_IOMMU_PLATFORM  |	\
+	 1ULL << VIRTIO_F_ORDER_PLATFORM)
 
 #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES	\
 	(VIRTIO_PMD_DEFAULT_GUEST_FEATURES |	\
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index e961a58ca..e2f096185 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -128,6 +128,12 @@ struct virtnet_ctl;
  */
 #define VIRTIO_F_IN_ORDER 35
 
+/*
+ * This feature indicates that memory accesses by the driver and the device
+ * are ordered in a way described by the platform.
+ */
+#define VIRTIO_F_ORDER_PLATFORM 36
+
 /* The Guest publishes the used index for which it expects an interrupt
  * at the end of the avail ring. Host should ignore the avail->flags field. */
 /* The Host publishes the avail index for which it expects a kick
@@ -240,6 +246,7 @@ struct virtio_hw {
 	uint8_t     use_simple_rx;
 	uint8_t     use_inorder_rx;
 	uint8_t     use_inorder_tx;
+	uint8_t     weak_barriers;
 	bool        has_tx_offload;
 	bool        has_rx_offload;
 	uint16_t    port_id;
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index cb8f89f18..66195bf47 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -906,7 +906,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 
 	num = likely(nb_used <= nb_pkts) ? nb_used : nb_pkts;
 	if (unlikely(num > VIRTIO_MBUF_BURST_SZ))
@@ -1017,7 +1017,7 @@ virtio_recv_mergeable_pkts_inorder(void *rx_queue,
 	nb_used = RTE_MIN(nb_used, nb_pkts);
 	nb_used = RTE_MIN(nb_used, VIRTIO_MBUF_BURST_SZ);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 
 	PMD_RX_LOG(DEBUG, "used:%d", nb_used);
 
@@ -1202,7 +1202,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 
 	PMD_RX_LOG(DEBUG, "used:%d", nb_used);
 
@@ -1365,7 +1365,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 	if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh))
 		virtio_xmit_cleanup(vq, nb_used);
 
@@ -1407,7 +1407,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		/* Positive value indicates it need free vring descriptors */
 		if (unlikely(need > 0)) {
 			nb_used = VIRTQUEUE_NUSED(vq);
-			virtio_rmb();
+			virtio_rmb(hw->weak_barriers);
 			need = RTE_MIN(need, (int)nb_used);
 
 			virtio_xmit_cleanup(vq, need);
@@ -1463,7 +1463,7 @@ virtio_xmit_pkts_inorder(void *tx_queue,
 	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 	if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh))
 		virtio_xmit_cleanup_inorder(vq, nb_used);
 
@@ -1511,7 +1511,7 @@ virtio_xmit_pkts_inorder(void *tx_queue,
 		need = slots - vq->vq_free_cnt;
 		if (unlikely(need > 0)) {
 			nb_used = VIRTQUEUE_NUSED(vq);
-			virtio_rmb();
+			virtio_rmb(hw->weak_barriers);
 			need = RTE_MIN(need, (int)nb_used);
 
 			virtio_xmit_cleanup_inorder(vq, need);
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 26518ed98..6b9055a1f 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -19,15 +19,40 @@
 struct rte_mbuf;
 
 /*
- * Per virtio_config.h in Linux.
+ * Per virtio_ring.h in Linux.
  *     For virtio_pci on SMP, we don't need to order with respect to MMIO
  *     accesses through relaxed memory I/O windows, so smp_mb() et al are
  *     sufficient.
  *
+ *     For using virtio to talk to real devices (eg. vDPA) we do need real
+ *     barriers.
  */
-#define virtio_mb()	rte_smp_mb()
-#define virtio_rmb()	rte_smp_rmb()
-#define virtio_wmb()	rte_smp_wmb()
+static inline void
+virtio_mb(uint8_t weak_barriers)
+{
+	if (weak_barriers)
+		rte_smp_mb();
+	else
+		rte_mb();
+}
+
+static inline void
+virtio_rmb(uint8_t weak_barriers)
+{
+	if (weak_barriers)
+		rte_smp_rmb();
+	else
+		rte_cio_rmb();
+}
+
+static inline void
+virtio_wmb(uint8_t weak_barriers)
+{
+	if (weak_barriers)
+		rte_smp_wmb();
+	else
+		rte_cio_wmb();
+}
 
 #ifdef RTE_PMD_PACKET_PREFETCH
 #define rte_packet_prefetch(p)  rte_prefetch1(p)
@@ -312,7 +337,7 @@ void vq_ring_free_inorder(struct virtqueue *vq, uint16_t desc_idx,
 static inline void
 vq_update_avail_idx(struct virtqueue *vq)
 {
-	virtio_wmb();
+	virtio_wmb(vq->hw->weak_barriers);
 	vq->vq_ring.avail->idx = vq->vq_avail_idx;
 }
 
-- 
2.17.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: add platform memory ordering feature support
  2018-12-14 15:38 ` [dpdk-dev] [PATCH] net/virtio: add platform memory ordering feature support Ilya Maximets
@ 2018-12-14 17:00   ` Michael S. Tsirkin
  2018-12-14 17:23     ` Ilya Maximets
       [not found]   ` <CGME20181226163717eucas1p15276eb45e35abe2c9cf3e7c1e0050823@eucas1p1.samsung.com>
  1 sibling, 1 reply; 28+ messages in thread
From: Michael S. Tsirkin @ 2018-12-14 17:00 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: dev, Maxime Coquelin, Xiao Wang, Tiwei Bie, Zhihong Wang,
	jfreimann, Jason Wang, xiaolong.ye, alejandro.lucero

On Fri, Dec 14, 2018 at 06:38:12PM +0300, Ilya Maximets wrote:
> VIRTIO_F_ORDER_PLATFORM is required to use proper memory barriers
> in case of HW vhost implementations like vDPA.
> 
> DMA barriers (rte_cio_*) are sufficent for that purpose.
> 
> Previously known as VIRTIO_F_IO_BARRIER.
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> 
> Based on "[RFC] net/virtio: use real barriers for vDPA".
> 
> RFC --> PATCH:
>   * Dropped vendor-specific hack to determine if we need real barriers.
>   * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.
> 
> Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
>       to VIRTIO_F_ORDER_PLATFORM is not merged yet:
>       https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg04114.html
> 
>  drivers/net/virtio/virtio_ethdev.c |  2 ++
>  drivers/net/virtio/virtio_ethdev.h |  3 ++-
>  drivers/net/virtio/virtio_pci.h    |  7 ++++++
>  drivers/net/virtio/virtio_rxtx.c   | 14 ++++++------
>  drivers/net/virtio/virtqueue.h     | 35 +++++++++++++++++++++++++-----
>  5 files changed, 48 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index cb2b2e0bf..5ae7a9650 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1474,6 +1474,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
>  	if (virtio_negotiate_features(hw, req_features) < 0)
>  		return -1;
>  
> +	hw->weak_barriers = !vtpci_with_feature(hw, VIRTIO_F_ORDER_PLATFORM);
> +
>  	if (!hw->virtio_user_dev) {
>  		pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
>  		rte_eth_copy_pci_info(eth_dev, pci_dev);
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index e0f80e5a4..c098e5ac0 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -34,7 +34,8 @@
>  	 1u << VIRTIO_RING_F_INDIRECT_DESC |    \
>  	 1ULL << VIRTIO_F_VERSION_1       |	\
>  	 1ULL << VIRTIO_F_IN_ORDER        |	\
> -	 1ULL << VIRTIO_F_IOMMU_PLATFORM)
> +	 1ULL << VIRTIO_F_IOMMU_PLATFORM  |	\
> +	 1ULL << VIRTIO_F_ORDER_PLATFORM)
>  
>  #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES	\
>  	(VIRTIO_PMD_DEFAULT_GUEST_FEATURES |	\
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
> index e961a58ca..e2f096185 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -128,6 +128,12 @@ struct virtnet_ctl;
>   */
>  #define VIRTIO_F_IN_ORDER 35
>  
> +/*
> + * This feature indicates that memory accesses by the driver and the device
> + * are ordered in a way described by the platform.
> + */
> +#define VIRTIO_F_ORDER_PLATFORM 36
> +
>  /* The Guest publishes the used index for which it expects an interrupt
>   * at the end of the avail ring. Host should ignore the avail->flags field. */
>  /* The Host publishes the avail index for which it expects a kick
> @@ -240,6 +246,7 @@ struct virtio_hw {
>  	uint8_t     use_simple_rx;
>  	uint8_t     use_inorder_rx;
>  	uint8_t     use_inorder_tx;
> +	uint8_t     weak_barriers;
>  	bool        has_tx_offload;
>  	bool        has_rx_offload;
>  	uint16_t    port_id;
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index cb8f89f18..66195bf47 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -906,7 +906,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
>  
>  	nb_used = VIRTQUEUE_NUSED(vq);
>  
> -	virtio_rmb();
> +	virtio_rmb(hw->weak_barriers);
>  
>  	num = likely(nb_used <= nb_pkts) ? nb_used : nb_pkts;
>  	if (unlikely(num > VIRTIO_MBUF_BURST_SZ))
> @@ -1017,7 +1017,7 @@ virtio_recv_mergeable_pkts_inorder(void *rx_queue,
>  	nb_used = RTE_MIN(nb_used, nb_pkts);
>  	nb_used = RTE_MIN(nb_used, VIRTIO_MBUF_BURST_SZ);
>  
> -	virtio_rmb();
> +	virtio_rmb(hw->weak_barriers);
>  
>  	PMD_RX_LOG(DEBUG, "used:%d", nb_used);
>  
> @@ -1202,7 +1202,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
>  
>  	nb_used = VIRTQUEUE_NUSED(vq);
>  
> -	virtio_rmb();
> +	virtio_rmb(hw->weak_barriers);
>  
>  	PMD_RX_LOG(DEBUG, "used:%d", nb_used);
>  
> @@ -1365,7 +1365,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>  	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
>  	nb_used = VIRTQUEUE_NUSED(vq);
>  
> -	virtio_rmb();
> +	virtio_rmb(hw->weak_barriers);
>  	if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh))
>  		virtio_xmit_cleanup(vq, nb_used);
>  
> @@ -1407,7 +1407,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>  		/* Positive value indicates it need free vring descriptors */
>  		if (unlikely(need > 0)) {
>  			nb_used = VIRTQUEUE_NUSED(vq);
> -			virtio_rmb();
> +			virtio_rmb(hw->weak_barriers);
>  			need = RTE_MIN(need, (int)nb_used);
>  
>  			virtio_xmit_cleanup(vq, need);
> @@ -1463,7 +1463,7 @@ virtio_xmit_pkts_inorder(void *tx_queue,
>  	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
>  	nb_used = VIRTQUEUE_NUSED(vq);
>  
> -	virtio_rmb();
> +	virtio_rmb(hw->weak_barriers);
>  	if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh))
>  		virtio_xmit_cleanup_inorder(vq, nb_used);
>  
> @@ -1511,7 +1511,7 @@ virtio_xmit_pkts_inorder(void *tx_queue,
>  		need = slots - vq->vq_free_cnt;
>  		if (unlikely(need > 0)) {
>  			nb_used = VIRTQUEUE_NUSED(vq);
> -			virtio_rmb();
> +			virtio_rmb(hw->weak_barriers);
>  			need = RTE_MIN(need, (int)nb_used);
>  
>  			virtio_xmit_cleanup_inorder(vq, need);
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index 26518ed98..6b9055a1f 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -19,15 +19,40 @@
>  struct rte_mbuf;
>  
>  /*
> - * Per virtio_config.h in Linux.
> + * Per virtio_ring.h in Linux.
>   *     For virtio_pci on SMP, we don't need to order with respect to MMIO
>   *     accesses through relaxed memory I/O windows, so smp_mb() et al are
>   *     sufficient.
>   *
> + *     For using virtio to talk to real devices (eg. vDPA) we do need real
> + *     barriers.
>   */
> -#define virtio_mb()	rte_smp_mb()
> -#define virtio_rmb()	rte_smp_rmb()
> -#define virtio_wmb()	rte_smp_wmb()
> +static inline void
> +virtio_mb(uint8_t weak_barriers)
> +{
> +	if (weak_barriers)
> +		rte_smp_mb();
> +	else
> +		rte_mb();
> +}
> +

Why doesn't rte_cio_rmb exit?

> +static inline void
> +virtio_rmb(uint8_t weak_barriers)
> +{
> +	if (weak_barriers)
> +		rte_smp_rmb();
> +	else
> +		rte_cio_rmb();
> +}
> +
> +static inline void
> +virtio_wmb(uint8_t weak_barriers)
> +{
> +	if (weak_barriers)
> +		rte_smp_wmb();
> +	else
> +		rte_cio_wmb();
> +}
>  
>  #ifdef RTE_PMD_PACKET_PREFETCH
>  #define rte_packet_prefetch(p)  rte_prefetch1(p)
> @@ -312,7 +337,7 @@ void vq_ring_free_inorder(struct virtqueue *vq, uint16_t desc_idx,
>  static inline void
>  vq_update_avail_idx(struct virtqueue *vq)
>  {
> -	virtio_wmb();
> +	virtio_wmb(vq->hw->weak_barriers);
>  	vq->vq_ring.avail->idx = vq->vq_avail_idx;
>  }
>  
> -- 
> 2.17.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: add platform memory ordering feature support
  2018-12-14 17:00   ` Michael S. Tsirkin
@ 2018-12-14 17:23     ` Ilya Maximets
  0 siblings, 0 replies; 28+ messages in thread
From: Ilya Maximets @ 2018-12-14 17:23 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: dev, Maxime Coquelin, Xiao Wang, Tiwei Bie, Zhihong Wang,
	jfreimann, Jason Wang, xiaolong.ye, alejandro.lucero,
	Yongseok Koh

On 14.12.2018 20:00, Michael S. Tsirkin wrote:
> On Fri, Dec 14, 2018 at 06:38:12PM +0300, Ilya Maximets wrote:
>> VIRTIO_F_ORDER_PLATFORM is required to use proper memory barriers
>> in case of HW vhost implementations like vDPA.
>>
>> DMA barriers (rte_cio_*) are sufficent for that purpose.
>>
>> Previously known as VIRTIO_F_IO_BARRIER.
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>>
>> Based on "[RFC] net/virtio: use real barriers for vDPA".
>>
>> RFC --> PATCH:
>>   * Dropped vendor-specific hack to determine if we need real barriers.
>>   * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.
>>
>> Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
>>       to VIRTIO_F_ORDER_PLATFORM is not merged yet:
>>       https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg04114.html
>>
>>  drivers/net/virtio/virtio_ethdev.c |  2 ++
>>  drivers/net/virtio/virtio_ethdev.h |  3 ++-
>>  drivers/net/virtio/virtio_pci.h    |  7 ++++++
>>  drivers/net/virtio/virtio_rxtx.c   | 14 ++++++------
>>  drivers/net/virtio/virtqueue.h     | 35 +++++++++++++++++++++++++-----
>>  5 files changed, 48 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index cb2b2e0bf..5ae7a9650 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1474,6 +1474,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
>>  	if (virtio_negotiate_features(hw, req_features) < 0)
>>  		return -1;
>>  
>> +	hw->weak_barriers = !vtpci_with_feature(hw, VIRTIO_F_ORDER_PLATFORM);
>> +
>>  	if (!hw->virtio_user_dev) {
>>  		pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
>>  		rte_eth_copy_pci_info(eth_dev, pci_dev);
>> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
>> index e0f80e5a4..c098e5ac0 100644
>> --- a/drivers/net/virtio/virtio_ethdev.h
>> +++ b/drivers/net/virtio/virtio_ethdev.h
>> @@ -34,7 +34,8 @@
>>  	 1u << VIRTIO_RING_F_INDIRECT_DESC |    \
>>  	 1ULL << VIRTIO_F_VERSION_1       |	\
>>  	 1ULL << VIRTIO_F_IN_ORDER        |	\
>> -	 1ULL << VIRTIO_F_IOMMU_PLATFORM)
>> +	 1ULL << VIRTIO_F_IOMMU_PLATFORM  |	\
>> +	 1ULL << VIRTIO_F_ORDER_PLATFORM)
>>  
>>  #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES	\
>>  	(VIRTIO_PMD_DEFAULT_GUEST_FEATURES |	\
>> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
>> index e961a58ca..e2f096185 100644
>> --- a/drivers/net/virtio/virtio_pci.h
>> +++ b/drivers/net/virtio/virtio_pci.h
>> @@ -128,6 +128,12 @@ struct virtnet_ctl;
>>   */
>>  #define VIRTIO_F_IN_ORDER 35
>>  
>> +/*
>> + * This feature indicates that memory accesses by the driver and the device
>> + * are ordered in a way described by the platform.
>> + */
>> +#define VIRTIO_F_ORDER_PLATFORM 36
>> +
>>  /* The Guest publishes the used index for which it expects an interrupt
>>   * at the end of the avail ring. Host should ignore the avail->flags field. */
>>  /* The Host publishes the avail index for which it expects a kick
>> @@ -240,6 +246,7 @@ struct virtio_hw {
>>  	uint8_t     use_simple_rx;
>>  	uint8_t     use_inorder_rx;
>>  	uint8_t     use_inorder_tx;
>> +	uint8_t     weak_barriers;
>>  	bool        has_tx_offload;
>>  	bool        has_rx_offload;
>>  	uint16_t    port_id;
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>> index cb8f89f18..66195bf47 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -906,7 +906,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
>>  
>>  	nb_used = VIRTQUEUE_NUSED(vq);
>>  
>> -	virtio_rmb();
>> +	virtio_rmb(hw->weak_barriers);
>>  
>>  	num = likely(nb_used <= nb_pkts) ? nb_used : nb_pkts;
>>  	if (unlikely(num > VIRTIO_MBUF_BURST_SZ))
>> @@ -1017,7 +1017,7 @@ virtio_recv_mergeable_pkts_inorder(void *rx_queue,
>>  	nb_used = RTE_MIN(nb_used, nb_pkts);
>>  	nb_used = RTE_MIN(nb_used, VIRTIO_MBUF_BURST_SZ);
>>  
>> -	virtio_rmb();
>> +	virtio_rmb(hw->weak_barriers);
>>  
>>  	PMD_RX_LOG(DEBUG, "used:%d", nb_used);
>>  
>> @@ -1202,7 +1202,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
>>  
>>  	nb_used = VIRTQUEUE_NUSED(vq);
>>  
>> -	virtio_rmb();
>> +	virtio_rmb(hw->weak_barriers);
>>  
>>  	PMD_RX_LOG(DEBUG, "used:%d", nb_used);
>>  
>> @@ -1365,7 +1365,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>>  	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
>>  	nb_used = VIRTQUEUE_NUSED(vq);
>>  
>> -	virtio_rmb();
>> +	virtio_rmb(hw->weak_barriers);
>>  	if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh))
>>  		virtio_xmit_cleanup(vq, nb_used);
>>  
>> @@ -1407,7 +1407,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>>  		/* Positive value indicates it need free vring descriptors */
>>  		if (unlikely(need > 0)) {
>>  			nb_used = VIRTQUEUE_NUSED(vq);
>> -			virtio_rmb();
>> +			virtio_rmb(hw->weak_barriers);
>>  			need = RTE_MIN(need, (int)nb_used);
>>  
>>  			virtio_xmit_cleanup(vq, need);
>> @@ -1463,7 +1463,7 @@ virtio_xmit_pkts_inorder(void *tx_queue,
>>  	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
>>  	nb_used = VIRTQUEUE_NUSED(vq);
>>  
>> -	virtio_rmb();
>> +	virtio_rmb(hw->weak_barriers);
>>  	if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh))
>>  		virtio_xmit_cleanup_inorder(vq, nb_used);
>>  
>> @@ -1511,7 +1511,7 @@ virtio_xmit_pkts_inorder(void *tx_queue,
>>  		need = slots - vq->vq_free_cnt;
>>  		if (unlikely(need > 0)) {
>>  			nb_used = VIRTQUEUE_NUSED(vq);
>> -			virtio_rmb();
>> +			virtio_rmb(hw->weak_barriers);
>>  			need = RTE_MIN(need, (int)nb_used);
>>  
>>  			virtio_xmit_cleanup_inorder(vq, need);
>> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>> index 26518ed98..6b9055a1f 100644
>> --- a/drivers/net/virtio/virtqueue.h
>> +++ b/drivers/net/virtio/virtqueue.h
>> @@ -19,15 +19,40 @@
>>  struct rte_mbuf;
>>  
>>  /*
>> - * Per virtio_config.h in Linux.
>> + * Per virtio_ring.h in Linux.
>>   *     For virtio_pci on SMP, we don't need to order with respect to MMIO
>>   *     accesses through relaxed memory I/O windows, so smp_mb() et al are
>>   *     sufficient.
>>   *
>> + *     For using virtio to talk to real devices (eg. vDPA) we do need real
>> + *     barriers.
>>   */
>> -#define virtio_mb()	rte_smp_mb()
>> -#define virtio_rmb()	rte_smp_rmb()
>> -#define virtio_wmb()	rte_smp_wmb()
>> +static inline void
>> +virtio_mb(uint8_t weak_barriers)
>> +{
>> +	if (weak_barriers)
>> +		rte_smp_mb();
>> +	else
>> +		rte_mb();
>> +}
>> +
> 
> Why doesn't rte_cio_rmb exit?

Assuming your question was "Why doesn't rte_cio_mb exist?".
I guess 'cio' barriers was copied from 'dma_' barriers from kernel.
And 'rte_cio_mb' does not exist because there is no 'dma_mb' in kernel.

OTOH, maybe we can use 'rte_io_mb' here to be more consistent, but it
equals to the 'rte_mb' on all supported architectures. So, I'm not sure
if it's needed.

> 
>> +static inline void
>> +virtio_rmb(uint8_t weak_barriers)
>> +{
>> +	if (weak_barriers)
>> +		rte_smp_rmb();
>> +	else
>> +		rte_cio_rmb();
>> +}
>> +
>> +static inline void
>> +virtio_wmb(uint8_t weak_barriers)
>> +{
>> +	if (weak_barriers)
>> +		rte_smp_wmb();
>> +	else
>> +		rte_cio_wmb();
>> +}
>>  
>>  #ifdef RTE_PMD_PACKET_PREFETCH
>>  #define rte_packet_prefetch(p)  rte_prefetch1(p)
>> @@ -312,7 +337,7 @@ void vq_ring_free_inorder(struct virtqueue *vq, uint16_t desc_idx,
>>  static inline void
>>  vq_update_avail_idx(struct virtqueue *vq)
>>  {
>> -	virtio_wmb();
>> +	virtio_wmb(vq->hw->weak_barriers);
>>  	vq->vq_ring.avail->idx = vq->vq_avail_idx;
>>  }
>>  
>> -- 
>> 2.17.1
> 
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support
       [not found]   ` <CGME20181226163717eucas1p15276eb45e35abe2c9cf3e7c1e0050823@eucas1p1.samsung.com>
@ 2018-12-26 16:37     ` Ilya Maximets
  2018-12-27 10:07       ` Shahaf Shuler
       [not found]       ` <CGME20190109145021eucas1p1bfe194ffafaaaa5df62243c92b2ed6cd@eucas1p1.samsung.com>
  0 siblings, 2 replies; 28+ messages in thread
From: Ilya Maximets @ 2018-12-26 16:37 UTC (permalink / raw)
  To: dev, Maxime Coquelin, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, Ilya Maximets

VIRTIO_F_ORDER_PLATFORM is required to use proper memory barriers
in case of HW vhost implementations like vDPA.

DMA barriers (rte_cio_*) are sufficent for that purpose.

Previously known as VIRTIO_F_IO_BARRIER.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---

Version 2:
  * rebased on current master (packed rings).

RFC --> Version 1:
  * Dropped vendor-specific hack to determine if we need real barriers.
  * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.

Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
      to VIRTIO_F_ORDER_PLATFORM is not merged yet:
      https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg04114.html

 drivers/net/virtio/virtio_ethdev.c |  2 ++
 drivers/net/virtio/virtio_ethdev.h |  3 ++-
 drivers/net/virtio/virtio_pci.h    |  7 ++++++
 drivers/net/virtio/virtio_rxtx.c   | 16 ++++++------
 drivers/net/virtio/virtqueue.h     | 39 ++++++++++++++++++++++++------
 5 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 446c338fc..6d461180c 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1613,6 +1613,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 	if (virtio_negotiate_features(hw, req_features) < 0)
 		return -1;
 
+	hw->weak_barriers = !vtpci_with_feature(hw, VIRTIO_F_ORDER_PLATFORM);
+
 	if (!hw->virtio_user_dev) {
 		pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
 		rte_eth_copy_pci_info(eth_dev, pci_dev);
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index f8d8a56ab..b8aab7da4 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -35,7 +35,8 @@
 	 1ULL << VIRTIO_F_VERSION_1       |	\
 	 1ULL << VIRTIO_F_IN_ORDER        |	\
 	 1ULL << VIRTIO_F_RING_PACKED	  |	\
-	 1ULL << VIRTIO_F_IOMMU_PLATFORM)
+	 1ULL << VIRTIO_F_IOMMU_PLATFORM  |	\
+	 1ULL << VIRTIO_F_ORDER_PLATFORM)
 
 #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES	\
 	(VIRTIO_PMD_DEFAULT_GUEST_FEATURES |	\
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index b22b62dad..38a0261da 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -129,6 +129,12 @@ struct virtnet_ctl;
  */
 #define VIRTIO_F_IN_ORDER 35
 
+/*
+ * This feature indicates that memory accesses by the driver and the device
+ * are ordered in a way described by the platform.
+ */
+#define VIRTIO_F_ORDER_PLATFORM 36
+
 /* The Guest publishes the used index for which it expects an interrupt
  * at the end of the avail ring. Host should ignore the avail->flags field. */
 /* The Host publishes the avail index for which it expects a kick
@@ -241,6 +247,7 @@ struct virtio_hw {
 	uint8_t     use_simple_rx;
 	uint8_t     use_inorder_rx;
 	uint8_t     use_inorder_tx;
+	uint8_t     weak_barriers;
 	bool        has_tx_offload;
 	bool        has_rx_offload;
 	uint16_t    port_id;
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 2309b71d6..ebb86ef70 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1152,7 +1152,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 
 	num = likely(nb_used <= nb_pkts) ? nb_used : nb_pkts;
 	if (unlikely(num > VIRTIO_MBUF_BURST_SZ))
@@ -1361,7 +1361,7 @@ virtio_recv_pkts_inorder(void *rx_queue,
 	nb_used = RTE_MIN(nb_used, nb_pkts);
 	nb_used = RTE_MIN(nb_used, VIRTIO_MBUF_BURST_SZ);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 
 	PMD_RX_LOG(DEBUG, "used:%d", nb_used);
 
@@ -1549,7 +1549,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 
 	PMD_RX_LOG(DEBUG, "used:%d", nb_used);
 
@@ -1940,7 +1940,7 @@ virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
 
 		/* Positive value indicates it need free vring descriptors */
 		if (unlikely(need > 0)) {
-			virtio_rmb();
+			virtio_rmb(hw->weak_barriers);
 			need = RTE_MIN(need, (int)nb_pkts);
 			virtio_xmit_cleanup_packed(vq, need);
 			need = slots - vq->vq_free_cnt;
@@ -1988,7 +1988,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 	if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh))
 		virtio_xmit_cleanup(vq, nb_used);
 
@@ -2030,7 +2030,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		/* Positive value indicates it need free vring descriptors */
 		if (unlikely(need > 0)) {
 			nb_used = VIRTQUEUE_NUSED(vq);
-			virtio_rmb();
+			virtio_rmb(hw->weak_barriers);
 			need = RTE_MIN(need, (int)nb_used);
 
 			virtio_xmit_cleanup(vq, need);
@@ -2086,7 +2086,7 @@ virtio_xmit_pkts_inorder(void *tx_queue,
 	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 	if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh))
 		virtio_xmit_cleanup_inorder(vq, nb_used);
 
@@ -2134,7 +2134,7 @@ virtio_xmit_pkts_inorder(void *tx_queue,
 		need = slots - vq->vq_free_cnt;
 		if (unlikely(need > 0)) {
 			nb_used = VIRTQUEUE_NUSED(vq);
-			virtio_rmb();
+			virtio_rmb(hw->weak_barriers);
 			need = RTE_MIN(need, (int)nb_used);
 
 			virtio_xmit_cleanup_inorder(vq, need);
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index d8ae5cdec..a66a37f61 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -19,15 +19,40 @@
 struct rte_mbuf;
 
 /*
- * Per virtio_config.h in Linux.
+ * Per virtio_ring.h in Linux.
  *     For virtio_pci on SMP, we don't need to order with respect to MMIO
  *     accesses through relaxed memory I/O windows, so smp_mb() et al are
  *     sufficient.
  *
+ *     For using virtio to talk to real devices (eg. vDPA) we do need real
+ *     barriers.
  */
-#define virtio_mb()	rte_smp_mb()
-#define virtio_rmb()	rte_smp_rmb()
-#define virtio_wmb()	rte_smp_wmb()
+static inline void
+virtio_mb(uint8_t weak_barriers)
+{
+	if (weak_barriers)
+		rte_smp_mb();
+	else
+		rte_mb();
+}
+
+static inline void
+virtio_rmb(uint8_t weak_barriers)
+{
+	if (weak_barriers)
+		rte_smp_rmb();
+	else
+		rte_cio_rmb();
+}
+
+static inline void
+virtio_wmb(uint8_t weak_barriers)
+{
+	if (weak_barriers)
+		rte_smp_wmb();
+	else
+		rte_cio_wmb();
+}
 
 #ifdef RTE_PMD_PACKET_PREFETCH
 #define rte_packet_prefetch(p)  rte_prefetch1(p)
@@ -325,7 +350,7 @@ virtqueue_enable_intr_packed(struct virtqueue *vq)
 
 
 	if (vq->event_flags_shadow == RING_EVENT_FLAGS_DISABLE) {
-		virtio_wmb();
+		virtio_wmb(vq->hw->weak_barriers);
 		vq->event_flags_shadow = RING_EVENT_FLAGS_ENABLE;
 		*event_flags = vq->event_flags_shadow;
 	}
@@ -391,7 +416,7 @@ void vq_ring_free_inorder(struct virtqueue *vq, uint16_t desc_idx,
 static inline void
 vq_update_avail_idx(struct virtqueue *vq)
 {
-	virtio_wmb();
+	virtio_wmb(vq->hw->weak_barriers);
 	vq->vq_ring.avail->idx = vq->vq_avail_idx;
 }
 
@@ -423,7 +448,7 @@ virtqueue_kick_prepare_packed(struct virtqueue *vq)
 {
 	uint16_t flags;
 
-	virtio_mb();
+	virtio_mb(vq->hw->weak_barriers);
 	flags = vq->ring_packed.device_event->desc_event_flags;
 
 	return flags != RING_EVENT_FLAGS_DISABLE;
-- 
2.17.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support
  2018-12-26 16:37     ` [dpdk-dev] [PATCH v2] " Ilya Maximets
@ 2018-12-27 10:07       ` Shahaf Shuler
  2019-01-09 14:34         ` Ilya Maximets
       [not found]       ` <CGME20190109145021eucas1p1bfe194ffafaaaa5df62243c92b2ed6cd@eucas1p1.samsung.com>
  1 sibling, 1 reply; 28+ messages in thread
From: Shahaf Shuler @ 2018-12-27 10:07 UTC (permalink / raw)
  To: Ilya Maximets, dev, Maxime Coquelin, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, Daniel Marcovitch

Hi Ilya, 

Wednesday, December 26, 2018 6:37 PM, Ilya Maximets:
> Subject: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering
> feature support
> 
> VIRTIO_F_ORDER_PLATFORM is required to use proper memory barriers in
> case of HW vhost implementations like vDPA.
> 
> DMA barriers (rte_cio_*) are sufficent for that purpose.
> 
> Previously known as VIRTIO_F_IO_BARRIER.
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> 
> Version 2:
>   * rebased on current master (packed rings).
> 
> RFC --> Version 1:
>   * Dropped vendor-specific hack to determine if we need real barriers.
>   * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.
> 
> Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
>       to VIRTIO_F_ORDER_PLATFORM is not merged yet:
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.m
> ail-archive.com%2Fvirtio-dev%40lists.oasis-
> open.org%2Fmsg04114.html&amp;data=02%7C01%7Cshahafs%40mellanox.co
> m%7C147684bb9b754e30781408d66b506965%7Ca652971c7d2e4d9ba6a4d149
> 256f461b%7C0%7C0%7C636814390449088148&amp;sdata=f4W1YLftBtZ4zAQ3
> %2Bv7LV5w%2FDhM5aWpROV2c2gHY8iU%3D&amp;reserved=0
> 
>  drivers/net/virtio/virtio_ethdev.c |  2 ++  drivers/net/virtio/virtio_ethdev.h |  3
> ++-
>  drivers/net/virtio/virtio_pci.h    |  7 ++++++
>  drivers/net/virtio/virtio_rxtx.c   | 16 ++++++------
>  drivers/net/virtio/virtqueue.h     | 39 ++++++++++++++++++++++++------
>  5 files changed, 51 insertions(+), 16 deletions(-)

[...]

> 
>  /*
> - * Per virtio_config.h in Linux.
> + * Per virtio_ring.h in Linux.
>   *     For virtio_pci on SMP, we don't need to order with respect to MMIO
>   *     accesses through relaxed memory I/O windows, so smp_mb() et al are
>   *     sufficient.
>   *
> + *     For using virtio to talk to real devices (eg. vDPA) we do need real
> + *     barriers.
>   */
> -#define virtio_mb()	rte_smp_mb()
> -#define virtio_rmb()	rte_smp_rmb()
> -#define virtio_wmb()	rte_smp_wmb()
> +static inline void
> +virtio_mb(uint8_t weak_barriers)
> +{
> +	if (weak_barriers)
> +		rte_smp_mb();
> +	else
> +		rte_mb();
> +}
> +
> +static inline void
> +virtio_rmb(uint8_t weak_barriers)
> +{
> +	if (weak_barriers)
> +		rte_smp_rmb();
> +	else
> +		rte_cio_rmb();
> +}
> +
> +static inline void
> +virtio_wmb(uint8_t weak_barriers)
> +{
> +	if (weak_barriers)
> +		rte_smp_wmb();
> +	else
> +		rte_cio_wmb();

Just wondering if the cio barrier is enough here.
This virtio_wmb will be called, for example in the following sequence for transmit (not of packed queue):
if (likely(nb_tx)) {                                                 
        vq_update_avail_idx(vq);                <--- virito_wmb()                     
                                                                     
        if (unlikely(virtqueue_kick_prepare(vq))) {                 <--- no barrier 
                virtqueue_notify(vq);                                
                PMD_TX_LOG(DEBUG, "Notified backend after xmit");    
        }                                                            
}                                                                    

Assuming the backhand has vDPA device. The notify will be most likely mapped to the device PCI bar as write combining.
This means we need to keep ordering here between two memory domains - the PCI and the host local memory. We must make sure that when the notify reaches the device, the store to the host local memory is already written to memory (and not in the write buffer).
Is complier barrier is enough for such purpose? Or we need something stronger like sfence? 

Note #1 - This issue (if exists) is not caused directly by your code, however you are making things right here w/ respect to memory ordering so worth taking care also on this one
Note #2 - if indeed there is an issue here, for packed queue we are somewhat OK, since virtio_mb() will be called before the kick. I am not sure why we need such hard barrier and sfence is not enough. Do you know? 


> +}
> 
>  #ifdef RTE_PMD_PACKET_PREFETCH
>  #define rte_packet_prefetch(p)  rte_prefetch1(p) @@ -325,7 +350,7 @@
> virtqueue_enable_intr_packed(struct virtqueue *vq)
> 
> 
>  	if (vq->event_flags_shadow == RING_EVENT_FLAGS_DISABLE) {
> -		virtio_wmb();
> +		virtio_wmb(vq->hw->weak_barriers);
>  		vq->event_flags_shadow = RING_EVENT_FLAGS_ENABLE;
>  		*event_flags = vq->event_flags_shadow;
>  	}
> @@ -391,7 +416,7 @@ void vq_ring_free_inorder(struct virtqueue *vq,
> uint16_t desc_idx,  static inline void  vq_update_avail_idx(struct virtqueue *vq)
> {
> -	virtio_wmb();
> +	virtio_wmb(vq->hw->weak_barriers);
>  	vq->vq_ring.avail->idx = vq->vq_avail_idx;  }
> 
> @@ -423,7 +448,7 @@ virtqueue_kick_prepare_packed(struct virtqueue *vq)  {
>  	uint16_t flags;
> 
> -	virtio_mb();
> +	virtio_mb(vq->hw->weak_barriers);
>  	flags = vq->ring_packed.device_event->desc_event_flags;
> 
>  	return flags != RING_EVENT_FLAGS_DISABLE;
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support
  2018-12-27 10:07       ` Shahaf Shuler
@ 2019-01-09 14:34         ` Ilya Maximets
  2019-01-09 15:50           ` Michael S. Tsirkin
  0 siblings, 1 reply; 28+ messages in thread
From: Ilya Maximets @ 2019-01-09 14:34 UTC (permalink / raw)
  To: Shahaf Shuler, dev, Maxime Coquelin, Michael S . Tsirkin,
	Xiao Wang, jfreimann
  Cc: Tiwei Bie, Zhihong Wang, Jason Wang, xiaolong.ye,
	alejandro.lucero, Daniel Marcovitch

On 27.12.2018 13:07, Shahaf Shuler wrote:
> Hi Ilya, 
> 
> Wednesday, December 26, 2018 6:37 PM, Ilya Maximets:
>> Subject: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering
>> feature support
>>
>> VIRTIO_F_ORDER_PLATFORM is required to use proper memory barriers in
>> case of HW vhost implementations like vDPA.
>>
>> DMA barriers (rte_cio_*) are sufficent for that purpose.
>>
>> Previously known as VIRTIO_F_IO_BARRIER.
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>>
>> Version 2:
>>   * rebased on current master (packed rings).
>>
>> RFC --> Version 1:
>>   * Dropped vendor-specific hack to determine if we need real barriers.
>>   * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.
>>
>> Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
>>       to VIRTIO_F_ORDER_PLATFORM is not merged yet:
>>
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.m
>> ail-archive.com%2Fvirtio-dev%40lists.oasis-
>> open.org%2Fmsg04114.html&amp;data=02%7C01%7Cshahafs%40mellanox.co
>> m%7C147684bb9b754e30781408d66b506965%7Ca652971c7d2e4d9ba6a4d149
>> 256f461b%7C0%7C0%7C636814390449088148&amp;sdata=f4W1YLftBtZ4zAQ3
>> %2Bv7LV5w%2FDhM5aWpROV2c2gHY8iU%3D&amp;reserved=0
>>
>>  drivers/net/virtio/virtio_ethdev.c |  2 ++  drivers/net/virtio/virtio_ethdev.h |  3
>> ++-
>>  drivers/net/virtio/virtio_pci.h    |  7 ++++++
>>  drivers/net/virtio/virtio_rxtx.c   | 16 ++++++------
>>  drivers/net/virtio/virtqueue.h     | 39 ++++++++++++++++++++++++------
>>  5 files changed, 51 insertions(+), 16 deletions(-)
> 
> [...]
> 
>>
>>  /*
>> - * Per virtio_config.h in Linux.
>> + * Per virtio_ring.h in Linux.
>>   *     For virtio_pci on SMP, we don't need to order with respect to MMIO
>>   *     accesses through relaxed memory I/O windows, so smp_mb() et al are
>>   *     sufficient.
>>   *
>> + *     For using virtio to talk to real devices (eg. vDPA) we do need real
>> + *     barriers.
>>   */
>> -#define virtio_mb()	rte_smp_mb()
>> -#define virtio_rmb()	rte_smp_rmb()
>> -#define virtio_wmb()	rte_smp_wmb()
>> +static inline void
>> +virtio_mb(uint8_t weak_barriers)
>> +{
>> +	if (weak_barriers)
>> +		rte_smp_mb();
>> +	else
>> +		rte_mb();
>> +}
>> +
>> +static inline void
>> +virtio_rmb(uint8_t weak_barriers)
>> +{
>> +	if (weak_barriers)
>> +		rte_smp_rmb();
>> +	else
>> +		rte_cio_rmb();
>> +}
>> +
>> +static inline void
>> +virtio_wmb(uint8_t weak_barriers)
>> +{
>> +	if (weak_barriers)
>> +		rte_smp_wmb();
>> +	else
>> +		rte_cio_wmb();
> 
> Just wondering if the cio barrier is enough here.
> This virtio_wmb will be called, for example in the following sequence for transmit (not of packed queue):
> if (likely(nb_tx)) {                                                 
>         vq_update_avail_idx(vq);                <--- virito_wmb()                     
>                                                                      
>         if (unlikely(virtqueue_kick_prepare(vq))) {                 <--- no barrier 
>                 virtqueue_notify(vq);                                
>                 PMD_TX_LOG(DEBUG, "Notified backend after xmit");    
>         }                                                            
> }                                                                    
> 
> Assuming the backhand has vDPA device. The notify will be most likely mapped to the device PCI bar as write combining.
> This means we need to keep ordering here between two memory domains - the PCI and the host local memory. We must make sure that when the notify reaches the device, the store to the host local memory is already written to memory (and not in the write buffer).
> Is complier barrier is enough for such purpose? Or we need something stronger like sfence? 
> 
> Note #1 - This issue (if exists) is not caused directly by your code, however you are making things right here w/ respect to memory ordering so worth taking care also on this one
> Note #2 - if indeed there is an issue here, for packed queue we are somewhat OK, since virtio_mb() will be called before the kick. I am not sure why we need such hard barrier and sfence is not enough. Do you know? 

Hmm. Thanks for spotting this.
The issue exists, but it's a bit different.
Looks like we have missed virtio_mb() for the split case inside the
virtqueue_kick_prepare(). It's required because we must ensure the
ordering between writing the data (updating avail->idx) and reading the
used->flags. Otherwise we could catch the case where data written, but
virtqueue wasn't notified and vhost waits for notification.
This is not the vDPA related issue. This could happen with kernel vhost.
Adding the virtio_mb() to virtqueue_kick_prepare() will solve the vDPA
case automatically. I'll prepare the patches.


Beside that, Jens, between v10 and v11 of the packed queues support
patch-set you added the virtio_mb() to kick_prepare function.
I guess, this could be the root cause of the performance drop you
spotted in compare with split queues. Maybe you could check and prove
that point?
I didn't found why you done that change (there are no such review
comments on the list), but it was right decision.

virtio_mb() is really heavy. I'd like to avoid it somehow, but I don't
know how to do this yet.

> 
> 
>> +}
>>
>>  #ifdef RTE_PMD_PACKET_PREFETCH
>>  #define rte_packet_prefetch(p)  rte_prefetch1(p) @@ -325,7 +350,7 @@
>> virtqueue_enable_intr_packed(struct virtqueue *vq)
>>
>>
>>  	if (vq->event_flags_shadow == RING_EVENT_FLAGS_DISABLE) {
>> -		virtio_wmb();
>> +		virtio_wmb(vq->hw->weak_barriers);
>>  		vq->event_flags_shadow = RING_EVENT_FLAGS_ENABLE;
>>  		*event_flags = vq->event_flags_shadow;
>>  	}
>> @@ -391,7 +416,7 @@ void vq_ring_free_inorder(struct virtqueue *vq,
>> uint16_t desc_idx,  static inline void  vq_update_avail_idx(struct virtqueue *vq)
>> {
>> -	virtio_wmb();
>> +	virtio_wmb(vq->hw->weak_barriers);
>>  	vq->vq_ring.avail->idx = vq->vq_avail_idx;  }
>>
>> @@ -423,7 +448,7 @@ virtqueue_kick_prepare_packed(struct virtqueue *vq)  {
>>  	uint16_t flags;
>>
>> -	virtio_mb();
>> +	virtio_mb(vq->hw->weak_barriers);
>>  	flags = vq->ring_packed.device_event->desc_event_flags;
>>
>>  	return flags != RING_EVENT_FLAGS_DISABLE;
>> --
>> 2.17.1
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM.
       [not found]       ` <CGME20190109145021eucas1p1bfe194ffafaaaa5df62243c92b2ed6cd@eucas1p1.samsung.com>
@ 2019-01-09 14:50         ` Ilya Maximets
       [not found]           ` <CGME20190109145027eucas1p2437215de0df4c691eb84d4e84bfc71e5@eucas1p2.samsung.com>
                             ` (3 more replies)
  0 siblings, 4 replies; 28+ messages in thread
From: Ilya Maximets @ 2019-01-09 14:50 UTC (permalink / raw)
  To: dev, Maxime Coquelin, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, Ilya Maximets

Version 3:
  * Added 2 patches with fixes for current virtio driver.
    Not directly connected with the new feature.

Version 2:
  * rebased on current master (packed rings).

RFC --> Version 1:
  * Dropped vendor-specific hack to determine if we need real barriers.
  * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.

Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
      to VIRTIO_F_ORDER_PLATFORM is not merged yet:
      https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg04114.html

Ilya Maximets (3):
  net/virtio: add missing barrier before reading the flags
  net/virtio: update memory ordering comment for vq notify
  net/virtio: add platform memory ordering feature support

 drivers/net/virtio/virtio_ethdev.c |  2 ++
 drivers/net/virtio/virtio_ethdev.h |  3 +-
 drivers/net/virtio/virtio_pci.h    |  7 ++++
 drivers/net/virtio/virtio_rxtx.c   | 16 ++++-----
 drivers/net/virtio/virtqueue.h     | 56 +++++++++++++++++++++++-------
 5 files changed, 63 insertions(+), 21 deletions(-)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [dpdk-dev] [PATCH v3 1/3] net/virtio: add missing barrier before reading the flags
       [not found]           ` <CGME20190109145027eucas1p2437215de0df4c691eb84d4e84bfc71e5@eucas1p2.samsung.com>
@ 2019-01-09 14:50             ` Ilya Maximets
  2019-01-10 14:31               ` Maxime Coquelin
  0 siblings, 1 reply; 28+ messages in thread
From: Ilya Maximets @ 2019-01-09 14:50 UTC (permalink / raw)
  To: dev, Maxime Coquelin, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, Ilya Maximets, stable

Reading the used->flags could be reordered with avail->idx update.
vhost in kernel disables notifications for the time of packets
receiving, like this:

    1. disable notify
    2. process packets
    3. enable notify
    4. has more packets ? goto 1

In case of reordering, virtio driver could read the flags on
step 2 while notifications disabled and update avail->idx after
the step 4, i.e. vhost will exit the loop on step 4 with
notifications enabled, but virtio will not notify.

Fixes: c1f86306a026 ("virtio: add new driver")
CC: stable@dpdk.org

Reported-by: Shahaf Shuler <shahafs@mellanox.com>
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 drivers/net/virtio/virtqueue.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index d8ae5cdec..dffa03669 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -415,6 +415,11 @@ vq_update_avail_ring(struct virtqueue *vq, uint16_t desc_idx)
 static inline int
 virtqueue_kick_prepare(struct virtqueue *vq)
 {
+	/*
+	 * Ensure updated avail->idx is visible to vhost before reading
+	 * the used->flags.
+	 */
+	virtio_mb();
 	return !(vq->vq_ring.used->flags & VRING_USED_F_NO_NOTIFY);
 }
 
@@ -423,6 +428,9 @@ virtqueue_kick_prepare_packed(struct virtqueue *vq)
 {
 	uint16_t flags;
 
+	/*
+	 * Ensure updated data is visible to vhost before reading the flags.
+	 */
 	virtio_mb();
 	flags = vq->ring_packed.device_event->desc_event_flags;
 
-- 
2.17.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify
       [not found]           ` <CGME20190109145034eucas1p2183e275e316b87917b96fa184fc7d7cb@eucas1p2.samsung.com>
@ 2019-01-09 14:50             ` Ilya Maximets
  2019-01-10  8:19               ` Gavin Hu (Arm Technology China)
  2019-01-10 14:31               ` Maxime Coquelin
  0 siblings, 2 replies; 28+ messages in thread
From: Ilya Maximets @ 2019-01-09 14:50 UTC (permalink / raw)
  To: dev, Maxime Coquelin, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, Ilya Maximets

We're not using io ports in case of modern device even on IA.
Also, this comment useless for other architectures.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 drivers/net/virtio/virtqueue.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index dffa03669..53aeac238 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -437,14 +437,13 @@ virtqueue_kick_prepare_packed(struct virtqueue *vq)
 	return flags != RING_EVENT_FLAGS_DISABLE;
 }
 
+/*
+ * virtqueue_kick_prepare*() or the virtio_wmb() should be called
+ * before this function to be sure that all the data is visible to vhost.
+ */
 static inline void
 virtqueue_notify(struct virtqueue *vq)
 {
-	/*
-	 * Ensure updated avail->idx is visible to host.
-	 * For virtio on IA, the notificaiton is through io port operation
-	 * which is a serialization instruction itself.
-	 */
 	VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
 }
 
-- 
2.17.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [dpdk-dev] [PATCH v3 3/3] net/virtio: add platform memory ordering feature support
       [not found]           ` <CGME20190109145040eucas1p2d9afc678ef94986544bde07b77373e6f@eucas1p2.samsung.com>
@ 2019-01-09 14:50             ` Ilya Maximets
  2019-01-10 14:31               ` Maxime Coquelin
  0 siblings, 1 reply; 28+ messages in thread
From: Ilya Maximets @ 2019-01-09 14:50 UTC (permalink / raw)
  To: dev, Maxime Coquelin, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, Ilya Maximets

VIRTIO_F_ORDER_PLATFORM is required to use proper memory barriers
in case of HW vhost implementations like vDPA.

DMA barriers (rte_cio_*) are sufficent for that purpose.

Previously known as VIRTIO_F_IO_BARRIER.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 drivers/net/virtio/virtio_ethdev.c |  2 ++
 drivers/net/virtio/virtio_ethdev.h |  3 ++-
 drivers/net/virtio/virtio_pci.h    |  7 +++++
 drivers/net/virtio/virtio_rxtx.c   | 16 ++++++------
 drivers/net/virtio/virtqueue.h     | 41 ++++++++++++++++++++++++------
 5 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 446c338fc..6d461180c 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1613,6 +1613,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 	if (virtio_negotiate_features(hw, req_features) < 0)
 		return -1;
 
+	hw->weak_barriers = !vtpci_with_feature(hw, VIRTIO_F_ORDER_PLATFORM);
+
 	if (!hw->virtio_user_dev) {
 		pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
 		rte_eth_copy_pci_info(eth_dev, pci_dev);
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index f8d8a56ab..b8aab7da4 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -35,7 +35,8 @@
 	 1ULL << VIRTIO_F_VERSION_1       |	\
 	 1ULL << VIRTIO_F_IN_ORDER        |	\
 	 1ULL << VIRTIO_F_RING_PACKED	  |	\
-	 1ULL << VIRTIO_F_IOMMU_PLATFORM)
+	 1ULL << VIRTIO_F_IOMMU_PLATFORM  |	\
+	 1ULL << VIRTIO_F_ORDER_PLATFORM)
 
 #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES	\
 	(VIRTIO_PMD_DEFAULT_GUEST_FEATURES |	\
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index b22b62dad..38a0261da 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -129,6 +129,12 @@ struct virtnet_ctl;
  */
 #define VIRTIO_F_IN_ORDER 35
 
+/*
+ * This feature indicates that memory accesses by the driver and the device
+ * are ordered in a way described by the platform.
+ */
+#define VIRTIO_F_ORDER_PLATFORM 36
+
 /* The Guest publishes the used index for which it expects an interrupt
  * at the end of the avail ring. Host should ignore the avail->flags field. */
 /* The Host publishes the avail index for which it expects a kick
@@ -241,6 +247,7 @@ struct virtio_hw {
 	uint8_t     use_simple_rx;
 	uint8_t     use_inorder_rx;
 	uint8_t     use_inorder_tx;
+	uint8_t     weak_barriers;
 	bool        has_tx_offload;
 	bool        has_rx_offload;
 	uint16_t    port_id;
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 2309b71d6..ebb86ef70 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1152,7 +1152,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 
 	num = likely(nb_used <= nb_pkts) ? nb_used : nb_pkts;
 	if (unlikely(num > VIRTIO_MBUF_BURST_SZ))
@@ -1361,7 +1361,7 @@ virtio_recv_pkts_inorder(void *rx_queue,
 	nb_used = RTE_MIN(nb_used, nb_pkts);
 	nb_used = RTE_MIN(nb_used, VIRTIO_MBUF_BURST_SZ);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 
 	PMD_RX_LOG(DEBUG, "used:%d", nb_used);
 
@@ -1549,7 +1549,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 
 	PMD_RX_LOG(DEBUG, "used:%d", nb_used);
 
@@ -1940,7 +1940,7 @@ virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
 
 		/* Positive value indicates it need free vring descriptors */
 		if (unlikely(need > 0)) {
-			virtio_rmb();
+			virtio_rmb(hw->weak_barriers);
 			need = RTE_MIN(need, (int)nb_pkts);
 			virtio_xmit_cleanup_packed(vq, need);
 			need = slots - vq->vq_free_cnt;
@@ -1988,7 +1988,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 	if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh))
 		virtio_xmit_cleanup(vq, nb_used);
 
@@ -2030,7 +2030,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		/* Positive value indicates it need free vring descriptors */
 		if (unlikely(need > 0)) {
 			nb_used = VIRTQUEUE_NUSED(vq);
-			virtio_rmb();
+			virtio_rmb(hw->weak_barriers);
 			need = RTE_MIN(need, (int)nb_used);
 
 			virtio_xmit_cleanup(vq, need);
@@ -2086,7 +2086,7 @@ virtio_xmit_pkts_inorder(void *tx_queue,
 	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-	virtio_rmb();
+	virtio_rmb(hw->weak_barriers);
 	if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh))
 		virtio_xmit_cleanup_inorder(vq, nb_used);
 
@@ -2134,7 +2134,7 @@ virtio_xmit_pkts_inorder(void *tx_queue,
 		need = slots - vq->vq_free_cnt;
 		if (unlikely(need > 0)) {
 			nb_used = VIRTQUEUE_NUSED(vq);
-			virtio_rmb();
+			virtio_rmb(hw->weak_barriers);
 			need = RTE_MIN(need, (int)nb_used);
 
 			virtio_xmit_cleanup_inorder(vq, need);
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 53aeac238..123bec34f 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -19,15 +19,40 @@
 struct rte_mbuf;
 
 /*
- * Per virtio_config.h in Linux.
+ * Per virtio_ring.h in Linux.
  *     For virtio_pci on SMP, we don't need to order with respect to MMIO
  *     accesses through relaxed memory I/O windows, so smp_mb() et al are
  *     sufficient.
  *
+ *     For using virtio to talk to real devices (eg. vDPA) we do need real
+ *     barriers.
  */
-#define virtio_mb()	rte_smp_mb()
-#define virtio_rmb()	rte_smp_rmb()
-#define virtio_wmb()	rte_smp_wmb()
+static inline void
+virtio_mb(uint8_t weak_barriers)
+{
+	if (weak_barriers)
+		rte_smp_mb();
+	else
+		rte_mb();
+}
+
+static inline void
+virtio_rmb(uint8_t weak_barriers)
+{
+	if (weak_barriers)
+		rte_smp_rmb();
+	else
+		rte_cio_rmb();
+}
+
+static inline void
+virtio_wmb(uint8_t weak_barriers)
+{
+	if (weak_barriers)
+		rte_smp_wmb();
+	else
+		rte_cio_wmb();
+}
 
 #ifdef RTE_PMD_PACKET_PREFETCH
 #define rte_packet_prefetch(p)  rte_prefetch1(p)
@@ -325,7 +350,7 @@ virtqueue_enable_intr_packed(struct virtqueue *vq)
 
 
 	if (vq->event_flags_shadow == RING_EVENT_FLAGS_DISABLE) {
-		virtio_wmb();
+		virtio_wmb(vq->hw->weak_barriers);
 		vq->event_flags_shadow = RING_EVENT_FLAGS_ENABLE;
 		*event_flags = vq->event_flags_shadow;
 	}
@@ -391,7 +416,7 @@ void vq_ring_free_inorder(struct virtqueue *vq, uint16_t desc_idx,
 static inline void
 vq_update_avail_idx(struct virtqueue *vq)
 {
-	virtio_wmb();
+	virtio_wmb(vq->hw->weak_barriers);
 	vq->vq_ring.avail->idx = vq->vq_avail_idx;
 }
 
@@ -419,7 +444,7 @@ virtqueue_kick_prepare(struct virtqueue *vq)
 	 * Ensure updated avail->idx is visible to vhost before reading
 	 * the used->flags.
 	 */
-	virtio_mb();
+	virtio_mb(vq->hw->weak_barriers);
 	return !(vq->vq_ring.used->flags & VRING_USED_F_NO_NOTIFY);
 }
 
@@ -431,7 +456,7 @@ virtqueue_kick_prepare_packed(struct virtqueue *vq)
 	/*
 	 * Ensure updated data is visible to vhost before reading the flags.
 	 */
-	virtio_mb();
+	virtio_mb(vq->hw->weak_barriers);
 	flags = vq->ring_packed.device_event->desc_event_flags;
 
 	return flags != RING_EVENT_FLAGS_DISABLE;
-- 
2.17.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM.
  2019-01-09 14:50         ` [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM Ilya Maximets
                             ` (2 preceding siblings ...)
       [not found]           ` <CGME20190109145040eucas1p2d9afc678ef94986544bde07b77373e6f@eucas1p2.samsung.com>
@ 2019-01-09 14:55           ` Michael S. Tsirkin
  2019-01-09 15:24             ` Ilya Maximets
  2019-01-10 15:19             ` Maxime Coquelin
  3 siblings, 2 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2019-01-09 14:55 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: dev, Maxime Coquelin, Xiao Wang, Tiwei Bie, Zhihong Wang,
	jfreimann, Jason Wang, xiaolong.ye, alejandro.lucero

On Wed, Jan 09, 2019 at 05:50:12PM +0300, Ilya Maximets wrote:
> Version 3:
>   * Added 2 patches with fixes for current virtio driver.
>     Not directly connected with the new feature.

New version shouldn't be reply-to old one really :).
But the patches are good I think.

Acked-by: Michael S. Tsirkin <mst@redhat.com>



> Version 2:
>   * rebased on current master (packed rings).
> 
> RFC --> Version 1:
>   * Dropped vendor-specific hack to determine if we need real barriers.
>   * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.
> 
> Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
>       to VIRTIO_F_ORDER_PLATFORM is not merged yet:
>       https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg04114.html
> 
> Ilya Maximets (3):
>   net/virtio: add missing barrier before reading the flags
>   net/virtio: update memory ordering comment for vq notify
>   net/virtio: add platform memory ordering feature support
> 
>  drivers/net/virtio/virtio_ethdev.c |  2 ++
>  drivers/net/virtio/virtio_ethdev.h |  3 +-
>  drivers/net/virtio/virtio_pci.h    |  7 ++++
>  drivers/net/virtio/virtio_rxtx.c   | 16 ++++-----
>  drivers/net/virtio/virtqueue.h     | 56 +++++++++++++++++++++++-------
>  5 files changed, 63 insertions(+), 21 deletions(-)
> 
> -- 
> 2.17.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM.
  2019-01-09 14:55           ` [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM Michael S. Tsirkin
@ 2019-01-09 15:24             ` Ilya Maximets
  2019-01-09 16:53               ` Ferruh Yigit
  2019-01-10 15:19             ` Maxime Coquelin
  1 sibling, 1 reply; 28+ messages in thread
From: Ilya Maximets @ 2019-01-09 15:24 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: dev, Maxime Coquelin, Xiao Wang, Tiwei Bie, Zhihong Wang,
	jfreimann, Jason Wang, xiaolong.ye, alejandro.lucero

On 09.01.2019 17:55, Michael S. Tsirkin wrote:
> On Wed, Jan 09, 2019 at 05:50:12PM +0300, Ilya Maximets wrote:
>> Version 3:
>>   * Added 2 patches with fixes for current virtio driver.
>>     Not directly connected with the new feature.
> 
> New version shouldn't be reply-to old one really :).

It's kind of a common practice and a recommended thing for the
dpdk mail-list. At least it was.
I'm not doing that on other lists. =)

> But the patches are good I think.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks.

> 
> 
> 
>> Version 2:
>>   * rebased on current master (packed rings).
>>
>> RFC --> Version 1:
>>   * Dropped vendor-specific hack to determine if we need real barriers.
>>   * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.
>>
>> Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
>>       to VIRTIO_F_ORDER_PLATFORM is not merged yet:
>>       https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg04114.html
>>
>> Ilya Maximets (3):
>>   net/virtio: add missing barrier before reading the flags
>>   net/virtio: update memory ordering comment for vq notify
>>   net/virtio: add platform memory ordering feature support
>>
>>  drivers/net/virtio/virtio_ethdev.c |  2 ++
>>  drivers/net/virtio/virtio_ethdev.h |  3 +-
>>  drivers/net/virtio/virtio_pci.h    |  7 ++++
>>  drivers/net/virtio/virtio_rxtx.c   | 16 ++++-----
>>  drivers/net/virtio/virtqueue.h     | 56 +++++++++++++++++++++++-------
>>  5 files changed, 63 insertions(+), 21 deletions(-)
>>
>> -- 
>> 2.17.1
> 
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support
  2019-01-09 14:34         ` Ilya Maximets
@ 2019-01-09 15:50           ` Michael S. Tsirkin
  2019-01-10 20:36             ` Shahaf Shuler
  0 siblings, 1 reply; 28+ messages in thread
From: Michael S. Tsirkin @ 2019-01-09 15:50 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: Shahaf Shuler, dev, Maxime Coquelin, Xiao Wang, jfreimann,
	Tiwei Bie, Zhihong Wang, Jason Wang, xiaolong.ye,
	alejandro.lucero, Daniel Marcovitch

On Wed, Jan 09, 2019 at 05:34:38PM +0300, Ilya Maximets wrote:
> virtio_mb() is really heavy. I'd like to avoid it somehow, but I don't
> know how to do this yet.

Linux driver doesn't avoid it either.

-- 
MST

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM.
  2019-01-09 15:24             ` Ilya Maximets
@ 2019-01-09 16:53               ` Ferruh Yigit
  0 siblings, 0 replies; 28+ messages in thread
From: Ferruh Yigit @ 2019-01-09 16:53 UTC (permalink / raw)
  To: Ilya Maximets, Michael S. Tsirkin
  Cc: dev, Maxime Coquelin, Xiao Wang, Tiwei Bie, Zhihong Wang,
	jfreimann, Jason Wang, xiaolong.ye, alejandro.lucero

On 1/9/2019 3:24 PM, Ilya Maximets wrote:
> On 09.01.2019 17:55, Michael S. Tsirkin wrote:
>> On Wed, Jan 09, 2019 at 05:50:12PM +0300, Ilya Maximets wrote:
>>> Version 3:
>>>   * Added 2 patches with fixes for current virtio driver.
>>>     Not directly connected with the new feature.
>>
>> New version shouldn't be reply-to old one really :).
> 
> It's kind of a common practice and a recommended thing for the
> dpdk mail-list. At least it was.
> I'm not doing that on other lists. =)

Yes this is something recommended to do. It helps a lot to be able to see all
versions and comments in same email thread.

> 
>> But the patches are good I think.
>>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Thanks.
> 
>>
>>
>>
>>> Version 2:
>>>   * rebased on current master (packed rings).
>>>
>>> RFC --> Version 1:
>>>   * Dropped vendor-specific hack to determine if we need real barriers.
>>>   * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.
>>>
>>> Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
>>>       to VIRTIO_F_ORDER_PLATFORM is not merged yet:
>>>       https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg04114.html
>>>
>>> Ilya Maximets (3):
>>>   net/virtio: add missing barrier before reading the flags
>>>   net/virtio: update memory ordering comment for vq notify
>>>   net/virtio: add platform memory ordering feature support
>>>
>>>  drivers/net/virtio/virtio_ethdev.c |  2 ++
>>>  drivers/net/virtio/virtio_ethdev.h |  3 +-
>>>  drivers/net/virtio/virtio_pci.h    |  7 ++++
>>>  drivers/net/virtio/virtio_rxtx.c   | 16 ++++-----
>>>  drivers/net/virtio/virtqueue.h     | 56 +++++++++++++++++++++++-------
>>>  5 files changed, 63 insertions(+), 21 deletions(-)
>>>
>>> -- 
>>> 2.17.1
>>
>>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify
  2019-01-09 14:50             ` [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify Ilya Maximets
@ 2019-01-10  8:19               ` Gavin Hu (Arm Technology China)
  2019-01-10  9:18                 ` Maxime Coquelin
  2019-01-10 14:31               ` Maxime Coquelin
  1 sibling, 1 reply; 28+ messages in thread
From: Gavin Hu (Arm Technology China) @ 2019-01-10  8:19 UTC (permalink / raw)
  To: Ilya Maximets, dev, Maxime Coquelin, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, Honnappa Nagarahalli, jerinj, nd



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ilya Maximets
> Sent: Wednesday, January 9, 2019 10:50 PM
> To: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
> Michael S . Tsirkin <mst@redhat.com>; Xiao Wang
> <xiao.w.wang@intel.com>
> Cc: Tiwei Bie <tiwei.bie@intel.com>; Zhihong Wang
> <zhihong.wang@intel.com>; jfreimann@redhat.com; Jason Wang
> <jasowang@redhat.com>; xiaolong.ye@intel.com;
> alejandro.lucero@netronome.com; Ilya Maximets
> <i.maximets@samsung.com>
> Subject: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering
> comment for vq notify
> 
> We're not using io ports in case of modern device even on IA.
> Also, this comment useless for other architectures.

Agree, it should be architecture neutral. 

> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  drivers/net/virtio/virtqueue.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index dffa03669..53aeac238 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -437,14 +437,13 @@ virtqueue_kick_prepare_packed(struct virtqueue
> *vq)
>  	return flags != RING_EVENT_FLAGS_DISABLE;
>  }
> 
> +/*
> + * virtqueue_kick_prepare*() or the virtio_wmb() should be called
> + * before this function to be sure that all the data is visible to vhost.
> + */

C11 _atomic APIs are preferred for new code, other than wmb or rmb,  could you work on that? 

>  static inline void
>  virtqueue_notify(struct virtqueue *vq)
>  {
> -	/*
> -	 * Ensure updated avail->idx is visible to host.
> -	 * For virtio on IA, the notificaiton is through io port operation
> -	 * which is a serialization instruction itself.
> -	 */
>  	VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
>  }
> 
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify
  2019-01-10  8:19               ` Gavin Hu (Arm Technology China)
@ 2019-01-10  9:18                 ` Maxime Coquelin
  2019-01-10  9:55                   ` Ilya Maximets
  0 siblings, 1 reply; 28+ messages in thread
From: Maxime Coquelin @ 2019-01-10  9:18 UTC (permalink / raw)
  To: Gavin Hu (Arm Technology China),
	Ilya Maximets, dev, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, Honnappa Nagarahalli, jerinj, nd

Hi Gavin,

On 1/10/19 9:19 AM, Gavin Hu (Arm Technology China) wrote:
> 
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ilya Maximets
>> Sent: Wednesday, January 9, 2019 10:50 PM
>> To: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
>> Michael S . Tsirkin <mst@redhat.com>; Xiao Wang
>> <xiao.w.wang@intel.com>
>> Cc: Tiwei Bie <tiwei.bie@intel.com>; Zhihong Wang
>> <zhihong.wang@intel.com>; jfreimann@redhat.com; Jason Wang
>> <jasowang@redhat.com>; xiaolong.ye@intel.com;
>> alejandro.lucero@netronome.com; Ilya Maximets
>> <i.maximets@samsung.com>
>> Subject: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering
>> comment for vq notify
>>
>> We're not using io ports in case of modern device even on IA.
>> Also, this comment useless for other architectures.
> 
> Agree, it should be architecture neutral.
> 
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>>   drivers/net/virtio/virtqueue.h | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>> index dffa03669..53aeac238 100644
>> --- a/drivers/net/virtio/virtqueue.h
>> +++ b/drivers/net/virtio/virtqueue.h
>> @@ -437,14 +437,13 @@ virtqueue_kick_prepare_packed(struct virtqueue
>> *vq)
>>   	return flags != RING_EVENT_FLAGS_DISABLE;
>>   }
>>
>> +/*
>> + * virtqueue_kick_prepare*() or the virtio_wmb() should be called
>> + * before this function to be sure that all the data is visible to vhost.
>> + */
> 
> C11 _atomic APIs are preferred for new code, other than wmb or rmb,  could you work on that?

Thanks for the review.
-rc2 deadline is today, so I may apply this series as is if no reply
from Ilya today. If a change is agreed, it could be made on top for
-rc3.

Regards,
Maxime
>>   static inline void
>>   virtqueue_notify(struct virtqueue *vq)
>>   {
>> -	/*
>> -	 * Ensure updated avail->idx is visible to host.
>> -	 * For virtio on IA, the notificaiton is through io port operation
>> -	 * which is a serialization instruction itself.
>> -	 */
>>   	VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
>>   }
>>
>> --
>> 2.17.1
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify
  2019-01-10  9:18                 ` Maxime Coquelin
@ 2019-01-10  9:55                   ` Ilya Maximets
  2019-01-10 14:56                     ` Michael S. Tsirkin
  0 siblings, 1 reply; 28+ messages in thread
From: Ilya Maximets @ 2019-01-10  9:55 UTC (permalink / raw)
  To: Maxime Coquelin, Gavin Hu (Arm Technology China),
	dev, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, Honnappa Nagarahalli, jerinj, nd

On 10.01.2019 12:18, Maxime Coquelin wrote:
> Hi Gavin,
> 
> On 1/10/19 9:19 AM, Gavin Hu (Arm Technology China) wrote:
>>
>>
>>> -----Original Message-----
>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ilya Maximets
>>> Sent: Wednesday, January 9, 2019 10:50 PM
>>> To: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
>>> Michael S . Tsirkin <mst@redhat.com>; Xiao Wang
>>> <xiao.w.wang@intel.com>
>>> Cc: Tiwei Bie <tiwei.bie@intel.com>; Zhihong Wang
>>> <zhihong.wang@intel.com>; jfreimann@redhat.com; Jason Wang
>>> <jasowang@redhat.com>; xiaolong.ye@intel.com;
>>> alejandro.lucero@netronome.com; Ilya Maximets
>>> <i.maximets@samsung.com>
>>> Subject: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering
>>> comment for vq notify
>>>
>>> We're not using io ports in case of modern device even on IA.
>>> Also, this comment useless for other architectures.
>>
>> Agree, it should be architecture neutral.
>>
>>>
>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>> ---
>>>   drivers/net/virtio/virtqueue.h | 9 ++++-----
>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>>> index dffa03669..53aeac238 100644
>>> --- a/drivers/net/virtio/virtqueue.h
>>> +++ b/drivers/net/virtio/virtqueue.h
>>> @@ -437,14 +437,13 @@ virtqueue_kick_prepare_packed(struct virtqueue
>>> *vq)
>>>       return flags != RING_EVENT_FLAGS_DISABLE;
>>>   }
>>>
>>> +/*
>>> + * virtqueue_kick_prepare*() or the virtio_wmb() should be called
>>> + * before this function to be sure that all the data is visible to vhost.
>>> + */
>>
>> C11 _atomic APIs are preferred for new code, other than wmb or rmb,  could you work on that?
> 
> Thanks for the review.
> -rc2 deadline is today, so I may apply this series as is if no reply
> from Ilya today. If a change is agreed, it could be made on top for
> -rc3.

IMHO, If we'll decide to move to C11 atomics, we'll need to rewrite all
the significant memory accesses in virtio driver at once. We can't do
this partially. This will require significant amount of work to understand
how to do that and will require a lot of testing. So, it's definitely not
for current release. Also, there are possible performance concerns about
such solution.

But I have even more significant concern: C11 atomics are designed for
inter-thread synchronization on multi-core systems. But in case of vDPA we
have real hardware and I'm not sure if we can use C11 atomics for
cross-domain synchronizations. Do you know if some of the memory ordering
types in C11 provide outer domain sync on ARMv8, for example ?

> 
> Regards,
> Maxime
>>>   static inline void
>>>   virtqueue_notify(struct virtqueue *vq)
>>>   {
>>> -    /*
>>> -     * Ensure updated avail->idx is visible to host.
>>> -     * For virtio on IA, the notificaiton is through io port operation
>>> -     * which is a serialization instruction itself.
>>> -     */
>>>       VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
>>>   }
>>>
>>> -- 
>>> 2.17.1
>>
> 
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/3] net/virtio: add missing barrier before reading the flags
  2019-01-09 14:50             ` [dpdk-dev] [PATCH v3 1/3] net/virtio: add missing barrier before reading the flags Ilya Maximets
@ 2019-01-10 14:31               ` Maxime Coquelin
  0 siblings, 0 replies; 28+ messages in thread
From: Maxime Coquelin @ 2019-01-10 14:31 UTC (permalink / raw)
  To: Ilya Maximets, dev, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero, stable



On 1/9/19 3:50 PM, Ilya Maximets wrote:
> Reading the used->flags could be reordered with avail->idx update.
> vhost in kernel disables notifications for the time of packets
> receiving, like this:
> 
>      1. disable notify
>      2. process packets
>      3. enable notify
>      4. has more packets ? goto 1
> 
> In case of reordering, virtio driver could read the flags on
> step 2 while notifications disabled and update avail->idx after
> the step 4, i.e. vhost will exit the loop on step 4 with
> notifications enabled, but virtio will not notify.
> 
> Fixes: c1f86306a026 ("virtio: add new driver")
> CC: stable@dpdk.org
> 
> Reported-by: Shahaf Shuler <shahafs@mellanox.com>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>   drivers/net/virtio/virtqueue.h | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify
  2019-01-09 14:50             ` [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify Ilya Maximets
  2019-01-10  8:19               ` Gavin Hu (Arm Technology China)
@ 2019-01-10 14:31               ` Maxime Coquelin
  1 sibling, 0 replies; 28+ messages in thread
From: Maxime Coquelin @ 2019-01-10 14:31 UTC (permalink / raw)
  To: Ilya Maximets, dev, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero



On 1/9/19 3:50 PM, Ilya Maximets wrote:
> We're not using io ports in case of modern device even on IA.
> Also, this comment useless for other architectures.
> 
> Signed-off-by: Ilya Maximets<i.maximets@samsung.com>
> ---
>   drivers/net/virtio/virtqueue.h | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)


Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 3/3] net/virtio: add platform memory ordering feature support
  2019-01-09 14:50             ` [dpdk-dev] [PATCH v3 3/3] net/virtio: add platform memory ordering feature support Ilya Maximets
@ 2019-01-10 14:31               ` Maxime Coquelin
  0 siblings, 0 replies; 28+ messages in thread
From: Maxime Coquelin @ 2019-01-10 14:31 UTC (permalink / raw)
  To: Ilya Maximets, dev, Michael S . Tsirkin, Xiao Wang
  Cc: Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang, xiaolong.ye,
	alejandro.lucero



On 1/9/19 3:50 PM, Ilya Maximets wrote:
> VIRTIO_F_ORDER_PLATFORM is required to use proper memory barriers
> in case of HW vhost implementations like vDPA.
> 
> DMA barriers (rte_cio_*) are sufficent for that purpose.
> 
> Previously known as VIRTIO_F_IO_BARRIER.
> 
> Signed-off-by: Ilya Maximets<i.maximets@samsung.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c |  2 ++
>   drivers/net/virtio/virtio_ethdev.h |  3 ++-
>   drivers/net/virtio/virtio_pci.h    |  7 +++++
>   drivers/net/virtio/virtio_rxtx.c   | 16 ++++++------
>   drivers/net/virtio/virtqueue.h     | 41 ++++++++++++++++++++++++------
>   5 files changed, 52 insertions(+), 17 deletions(-)



Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify
  2019-01-10  9:55                   ` Ilya Maximets
@ 2019-01-10 14:56                     ` Michael S. Tsirkin
  0 siblings, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2019-01-10 14:56 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: Maxime Coquelin, Gavin Hu (Arm Technology China),
	dev, Xiao Wang, Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang,
	xiaolong.ye, alejandro.lucero, Honnappa Nagarahalli, jerinj, nd

On Thu, Jan 10, 2019 at 12:55:19PM +0300, Ilya Maximets wrote:
> On 10.01.2019 12:18, Maxime Coquelin wrote:
> > Hi Gavin,
> > 
> > On 1/10/19 9:19 AM, Gavin Hu (Arm Technology China) wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ilya Maximets
> >>> Sent: Wednesday, January 9, 2019 10:50 PM
> >>> To: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
> >>> Michael S . Tsirkin <mst@redhat.com>; Xiao Wang
> >>> <xiao.w.wang@intel.com>
> >>> Cc: Tiwei Bie <tiwei.bie@intel.com>; Zhihong Wang
> >>> <zhihong.wang@intel.com>; jfreimann@redhat.com; Jason Wang
> >>> <jasowang@redhat.com>; xiaolong.ye@intel.com;
> >>> alejandro.lucero@netronome.com; Ilya Maximets
> >>> <i.maximets@samsung.com>
> >>> Subject: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering
> >>> comment for vq notify
> >>>
> >>> We're not using io ports in case of modern device even on IA.
> >>> Also, this comment useless for other architectures.
> >>
> >> Agree, it should be architecture neutral.
> >>
> >>>
> >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >>> ---
> >>>   drivers/net/virtio/virtqueue.h | 9 ++++-----
> >>>   1 file changed, 4 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> >>> index dffa03669..53aeac238 100644
> >>> --- a/drivers/net/virtio/virtqueue.h
> >>> +++ b/drivers/net/virtio/virtqueue.h
> >>> @@ -437,14 +437,13 @@ virtqueue_kick_prepare_packed(struct virtqueue
> >>> *vq)
> >>>       return flags != RING_EVENT_FLAGS_DISABLE;
> >>>   }
> >>>
> >>> +/*
> >>> + * virtqueue_kick_prepare*() or the virtio_wmb() should be called
> >>> + * before this function to be sure that all the data is visible to vhost.
> >>> + */
> >>
> >> C11 _atomic APIs are preferred for new code, other than wmb or rmb,  could you work on that?
> > 
> > Thanks for the review.
> > -rc2 deadline is today, so I may apply this series as is if no reply
> > from Ilya today. If a change is agreed, it could be made on top for
> > -rc3.
> 
> IMHO, If we'll decide to move to C11 atomics, we'll need to rewrite all
> the significant memory accesses in virtio driver at once. We can't do
> this partially. This will require significant amount of work to understand
> how to do that and will require a lot of testing. So, it's definitely not
> for current release. Also, there are possible performance concerns about
> such solution.
> 
> But I have even more significant concern: C11 atomics are designed for
> inter-thread synchronization on multi-core systems. But in case of vDPA we
> have real hardware and I'm not sure if we can use C11 atomics for
> cross-domain synchronizations. Do you know if some of the memory ordering
> types in C11 provide outer domain sync on ARMv8, for example ?

I would add to that - compiler support might not work well in all
versions which can be used to build dpdk.
E.g. this article https://lwn.net/Articles/691128/
says that "there will be some seriously suboptimal code production
before gcc-7.1".

HTH

> > 
> > Regards,
> > Maxime
> >>>   static inline void
> >>>   virtqueue_notify(struct virtqueue *vq)
> >>>   {
> >>> -    /*
> >>> -     * Ensure updated avail->idx is visible to host.
> >>> -     * For virtio on IA, the notificaiton is through io port operation
> >>> -     * which is a serialization instruction itself.
> >>> -     */
> >>>       VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
> >>>   }
> >>>
> >>> -- 
> >>> 2.17.1
> >>
> > 
> > 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM.
  2019-01-09 14:55           ` [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM Michael S. Tsirkin
  2019-01-09 15:24             ` Ilya Maximets
@ 2019-01-10 15:19             ` Maxime Coquelin
  1 sibling, 0 replies; 28+ messages in thread
From: Maxime Coquelin @ 2019-01-10 15:19 UTC (permalink / raw)
  To: Michael S. Tsirkin, Ilya Maximets
  Cc: dev, Xiao Wang, Tiwei Bie, Zhihong Wang, jfreimann, Jason Wang,
	xiaolong.ye, alejandro.lucero



On 1/9/19 3:55 PM, Michael S. Tsirkin wrote:
> On Wed, Jan 09, 2019 at 05:50:12PM +0300, Ilya Maximets wrote:
>> Version 3:
>>    * Added 2 patches with fixes for current virtio driver.
>>      Not directly connected with the new feature.
> 
> New version shouldn't be reply-to old one really :).
> But the patches are good I think.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> 
>> Version 2:
>>    * rebased on current master (packed rings).
>>
>> RFC --> Version 1:
>>    * Dropped vendor-specific hack to determine if we need real barriers.
>>    * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.
>>
>> Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
>>        to VIRTIO_F_ORDER_PLATFORM is not merged yet:
>>        https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg04114.html
>>
>> Ilya Maximets (3):
>>    net/virtio: add missing barrier before reading the flags
>>    net/virtio: update memory ordering comment for vq notify
>>    net/virtio: add platform memory ordering feature support
>>
>>   drivers/net/virtio/virtio_ethdev.c |  2 ++
>>   drivers/net/virtio/virtio_ethdev.h |  3 +-
>>   drivers/net/virtio/virtio_pci.h    |  7 ++++
>>   drivers/net/virtio/virtio_rxtx.c   | 16 ++++-----
>>   drivers/net/virtio/virtqueue.h     | 56 +++++++++++++++++++++++-------
>>   5 files changed, 63 insertions(+), 21 deletions(-)
>>
>> -- 
>> 2.17.1

Applied to dpdk-next-virtio/master.

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support
  2019-01-09 15:50           ` Michael S. Tsirkin
@ 2019-01-10 20:36             ` Shahaf Shuler
  2019-01-15  6:33               ` Shahaf Shuler
  0 siblings, 1 reply; 28+ messages in thread
From: Shahaf Shuler @ 2019-01-10 20:36 UTC (permalink / raw)
  To: Michael S. Tsirkin, Ilya Maximets
  Cc: dev, Maxime Coquelin, Xiao Wang, jfreimann, Tiwei Bie,
	Zhihong Wang, Jason Wang, xiaolong.ye, alejandro.lucero,
	Daniel Marcovitch

Wednesday, January 9, 2019 5:50 PM, Michael S. Tsirkin:
> alejandro.lucero@netronome.com; Daniel Marcovitch
> <danielm@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
> ordering feature support
> 
> On Wed, Jan 09, 2019 at 05:34:38PM +0300, Ilya Maximets wrote:
> > virtio_mb() is really heavy. I'd like to avoid it somehow, but I don't
> > know how to do this yet.
> 
> Linux driver doesn't avoid it either.

I understand v3 was merged but still would like to continue the discuss and make sure all is clear and agreed.

Form patch [1] description it is very clear why we need the rte_smp_mb() barrier.
However I am not sure why this barrier is interoperate into rte_mb in case of vDPA.  In vDPA case, both read of the user ring and write of the avail index are for local cached memory. 
The only write which is to uncachable memory (device memory) is the notify itself. 

As I mentioned, there is a need to have a store fence before doing the notify, but from different reasons. So vDPA use case and need Is a bit different than what presented in [1].

[1]
https://patches.dpdk.org/patch/49545/

> 
> --
> MST

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support
  2019-01-10 20:36             ` Shahaf Shuler
@ 2019-01-15  6:33               ` Shahaf Shuler
  2019-01-15  8:29                 ` Ilya Maximets
  0 siblings, 1 reply; 28+ messages in thread
From: Shahaf Shuler @ 2019-01-15  6:33 UTC (permalink / raw)
  To: Michael S. Tsirkin, Ilya Maximets
  Cc: dev, Maxime Coquelin, Xiao Wang, jfreimann, Tiwei Bie,
	Zhihong Wang, Jason Wang, xiaolong.ye, alejandro.lucero,
	Daniel Marcovitch

Thursday, January 10, 2019 10:37 PM, Shahaf Shuler:
> Subject: RE: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
> ordering feature support
> 
> Wednesday, January 9, 2019 5:50 PM, Michael S. Tsirkin:
> > alejandro.lucero@netronome.com; Daniel Marcovitch
> > <danielm@mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
> > ordering feature support
> >
> > On Wed, Jan 09, 2019 at 05:34:38PM +0300, Ilya Maximets wrote:
> > > virtio_mb() is really heavy. I'd like to avoid it somehow, but I
> > > don't know how to do this yet.
> >
> > Linux driver doesn't avoid it either.
> 
> I understand v3 was merged but still would like to continue the discuss and
> make sure all is clear and agreed.
> 
> Form patch [1] description it is very clear why we need the rte_smp_mb()
> barrier.
> However I am not sure why this barrier is interoperate into rte_mb in case of
> vDPA.  In vDPA case, both read of the user ring and write of the avail index
> are for local cached memory.
> The only write which is to uncachable memory (device memory) is the notify
> itself.
> 
> As I mentioned, there is a need to have a store fence before doing the
> notify, but from different reasons. So vDPA use case and need Is a bit
> different than what presented in [1].

Any answer?
It is pity if we add redundant barriers which will degrade the driver performance. 

> 
> [1]
> https://patches.dpdk.org/patch/49545/
> 
> >
> > --
> > MST

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support
  2019-01-15  6:33               ` Shahaf Shuler
@ 2019-01-15  8:29                 ` Ilya Maximets
  2019-01-15  8:55                   ` Shahaf Shuler
  0 siblings, 1 reply; 28+ messages in thread
From: Ilya Maximets @ 2019-01-15  8:29 UTC (permalink / raw)
  To: Shahaf Shuler, Michael S. Tsirkin
  Cc: dev, Maxime Coquelin, Xiao Wang, jfreimann, Tiwei Bie,
	Zhihong Wang, Jason Wang, xiaolong.ye, alejandro.lucero,
	Daniel Marcovitch

On 15.01.2019 9:33, Shahaf Shuler wrote:
> Thursday, January 10, 2019 10:37 PM, Shahaf Shuler:
>> Subject: RE: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
>> ordering feature support
>>
>> Wednesday, January 9, 2019 5:50 PM, Michael S. Tsirkin:
>>> alejandro.lucero@netronome.com; Daniel Marcovitch
>>> <danielm@mellanox.com>
>>> Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
>>> ordering feature support
>>>
>>> On Wed, Jan 09, 2019 at 05:34:38PM +0300, Ilya Maximets wrote:
>>>> virtio_mb() is really heavy. I'd like to avoid it somehow, but I
>>>> don't know how to do this yet.
>>>
>>> Linux driver doesn't avoid it either.
>>
>> I understand v3 was merged but still would like to continue the discuss and
>> make sure all is clear and agreed.
>>
>> Form patch [1] description it is very clear why we need the rte_smp_mb()
>> barrier.
>> However I am not sure why this barrier is interoperate into rte_mb in case of
>> vDPA.  In vDPA case, both read of the user ring and write of the avail index
>> are for local cached memory.
>> The only write which is to uncachable memory (device memory) is the notify
>> itself.
>>
>> As I mentioned, there is a need to have a store fence before doing the
>> notify, but from different reasons. So vDPA use case and need Is a bit
>> different than what presented in [1].
> 
> Any answer?
> It is pity if we add redundant barriers which will degrade the driver performance.

Sorry for late responses. Have a lot of work with OVS right now.

Regarding your question.
Current code looks like this:

 1. Update ring.
 2. virtio_wmb()
 3. Update idx.
 4. virtio_mb()
 5. read flags.
 6. notify.

virtio_mb() is here to avoid reordering of steps 3 and 5.
i.e. we need a full barrier to ensure the order between store (idx update)
and load (reading the flags). Otherwise we could miss the notification.
We can't avoid the barrier here, because even x86 does not guarantee
the ordering of the local load with earlier local store.

> 
>>
>> [1]
>> https://patches.dpdk.org/patch/49545/
>>
>>>
>>> --
>>> MST
> 
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support
  2019-01-15  8:29                 ` Ilya Maximets
@ 2019-01-15  8:55                   ` Shahaf Shuler
  2019-01-15 10:23                     ` Ilya Maximets
  2019-02-12 17:50                     ` Michael S. Tsirkin
  0 siblings, 2 replies; 28+ messages in thread
From: Shahaf Shuler @ 2019-01-15  8:55 UTC (permalink / raw)
  To: Ilya Maximets, Michael S. Tsirkin
  Cc: dev, Maxime Coquelin, Xiao Wang, jfreimann, Tiwei Bie,
	Zhihong Wang, Jason Wang, xiaolong.ye, alejandro.lucero,
	Daniel Marcovitch

Tuesday, January 15, 2019 10:29 AM, Ilya Maximets:
> Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
> ordering feature support
> 
> On 15.01.2019 9:33, Shahaf Shuler wrote:
> > Thursday, January 10, 2019 10:37 PM, Shahaf Shuler:
> >> Subject: RE: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
> >> ordering feature support
> >>
> >> Wednesday, January 9, 2019 5:50 PM, Michael S. Tsirkin:
> >>> alejandro.lucero@netronome.com; Daniel Marcovitch
> >>> <danielm@mellanox.com>
> >>> Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
> >>> ordering feature support
> >>>
> >>> On Wed, Jan 09, 2019 at 05:34:38PM +0300, Ilya Maximets wrote:
> >>>> virtio_mb() is really heavy. I'd like to avoid it somehow, but I
> >>>> don't know how to do this yet.
> >>>
> >>> Linux driver doesn't avoid it either.
> >>
> >> I understand v3 was merged but still would like to continue the
> >> discuss and make sure all is clear and agreed.
> >>
> >> Form patch [1] description it is very clear why we need the
> >> rte_smp_mb() barrier.
> >> However I am not sure why this barrier is interoperate into rte_mb in
> >> case of vDPA.  In vDPA case, both read of the user ring and write of
> >> the avail index are for local cached memory.
> >> The only write which is to uncachable memory (device memory) is the
> >> notify itself.
> >>
> >> As I mentioned, there is a need to have a store fence before doing
> >> the notify, but from different reasons. So vDPA use case and need Is
> >> a bit different than what presented in [1].
> >
> > Any answer?
> > It is pity if we add redundant barriers which will degrade the driver
> performance.
> 
> Sorry for late responses. Have a lot of work with OVS right now.
> 
> Regarding your question.
> Current code looks like this:
> 
>  1. Update ring.
>  2. virtio_wmb()
>  3. Update idx.
>  4. virtio_mb()
>  5. read flags.
>  6. notify.
> 
> virtio_mb() is here to avoid reordering of steps 3 and 5.
> i.e. we need a full barrier to ensure the order between store (idx update)
> and load (reading the flags). Otherwise we could miss the notification.
> We can't avoid the barrier here, because even x86 does not guarantee the
> ordering of the local load with earlier local store.

This is clear. You need the rte_smp_mb() here. My question is why you need the rte_mb() in case of vDPA? 
As you said, all accesses are local. 

Pasting you commit code:
/*                                                                        
 * Per virtio_ring.h in Linux.                                            
 *     For virtio_pci on SMP, we don't need to order with respect to MMIO 
 *     accesses through relaxed memory I/O windows, so smp_mb() et al are 
 *     sufficient.                                                        
 *                                                                        
 *     For using virtio to talk to real devices (eg. vDPA) we do need real
 *     barriers.                                                          
 */                                                                       
static inline void                                                        
virtio_mb(uint8_t weak_barriers)                                          
{                                                                         
        if (weak_barriers)                                                
                rte_smp_mb();                                             
        else                                                              
                rte_mb();                                                 
}                                                                         

> 
> >
> >>
> >> [1]
> >>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> >>
> tches.dpdk.org%2Fpatch%2F49545%2F&amp;data=02%7C01%7Cshahafs%40
> mellan
> >>
> ox.com%7C01907f1b2e0e4002cb7508d67ac38a98%7Ca652971c7d2e4d9ba6a4
> d1492
> >>
> 56f461b%7C0%7C0%7C636831377591864200&amp;sdata=TSpc%2Fzyq2aq0N3
> %2Bh4o
> >> ro4std8ut%2FQU6%2BOeMDeeaQdsM%3D&amp;reserved=0
> >>
> >>>
> >>> --
> >>> MST
> >
> >

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support
  2019-01-15  8:55                   ` Shahaf Shuler
@ 2019-01-15 10:23                     ` Ilya Maximets
  2019-02-12 17:50                     ` Michael S. Tsirkin
  1 sibling, 0 replies; 28+ messages in thread
From: Ilya Maximets @ 2019-01-15 10:23 UTC (permalink / raw)
  To: Shahaf Shuler, Michael S. Tsirkin
  Cc: dev, Maxime Coquelin, Xiao Wang, jfreimann, Tiwei Bie,
	Zhihong Wang, Jason Wang, xiaolong.ye, alejandro.lucero,
	Daniel Marcovitch

On 15.01.2019 11:55, Shahaf Shuler wrote:
> Tuesday, January 15, 2019 10:29 AM, Ilya Maximets:
>> Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
>> ordering feature support
>>
>> On 15.01.2019 9:33, Shahaf Shuler wrote:
>>> Thursday, January 10, 2019 10:37 PM, Shahaf Shuler:
>>>> Subject: RE: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
>>>> ordering feature support
>>>>
>>>> Wednesday, January 9, 2019 5:50 PM, Michael S. Tsirkin:
>>>>> alejandro.lucero@netronome.com; Daniel Marcovitch
>>>>> <danielm@mellanox.com>
>>>>> Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
>>>>> ordering feature support
>>>>>
>>>>> On Wed, Jan 09, 2019 at 05:34:38PM +0300, Ilya Maximets wrote:
>>>>>> virtio_mb() is really heavy. I'd like to avoid it somehow, but I
>>>>>> don't know how to do this yet.
>>>>>
>>>>> Linux driver doesn't avoid it either.
>>>>
>>>> I understand v3 was merged but still would like to continue the
>>>> discuss and make sure all is clear and agreed.
>>>>
>>>> Form patch [1] description it is very clear why we need the
>>>> rte_smp_mb() barrier.
>>>> However I am not sure why this barrier is interoperate into rte_mb in
>>>> case of vDPA.  In vDPA case, both read of the user ring and write of
>>>> the avail index are for local cached memory.
>>>> The only write which is to uncachable memory (device memory) is the
>>>> notify itself.
>>>>
>>>> As I mentioned, there is a need to have a store fence before doing
>>>> the notify, but from different reasons. So vDPA use case and need Is
>>>> a bit different than what presented in [1].
>>>
>>> Any answer?
>>> It is pity if we add redundant barriers which will degrade the driver
>> performance.
>>
>> Sorry for late responses. Have a lot of work with OVS right now.
>>
>> Regarding your question.
>> Current code looks like this:
>>
>>  1. Update ring.
>>  2. virtio_wmb()
>>  3. Update idx.
>>  4. virtio_mb()
>>  5. read flags.
>>  6. notify.
>>
>> virtio_mb() is here to avoid reordering of steps 3 and 5.
>> i.e. we need a full barrier to ensure the order between store (idx update)
>> and load (reading the flags). Otherwise we could miss the notification.
>> We can't avoid the barrier here, because even x86 does not guarantee the
>> ordering of the local load with earlier local store.
> 
> This is clear. You need the rte_smp_mb() here. My question is why you need the rte_mb() in case of vDPA? 
> As you said, all accesses are local.

Sorry, I misunderstood your question.
Memory accesses are not local here. We want the ring data/idx be visible
to vDPA HW before reading the flags.

> 
> Pasting you commit code:
> /*                                                                        
>  * Per virtio_ring.h in Linux.                                            
>  *     For virtio_pci on SMP, we don't need to order with respect to MMIO 
>  *     accesses through relaxed memory I/O windows, so smp_mb() et al are 
>  *     sufficient.                                                        
>  *                                                                        
>  *     For using virtio to talk to real devices (eg. vDPA) we do need real
>  *     barriers.                                                          
>  */                                                                       
> static inline void                                                        
> virtio_mb(uint8_t weak_barriers)                                          
> {                                                                         
>         if (weak_barriers)                                                
>                 rte_smp_mb();                                             
>         else                                                              
>                 rte_mb();                                                 
> }                                                                         
> 
>>
>>>
>>>>
>>>> [1]
>>>>
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
>>>>
>> tches.dpdk.org%2Fpatch%2F49545%2F&amp;data=02%7C01%7Cshahafs%40
>> mellan
>>>>
>> ox.com%7C01907f1b2e0e4002cb7508d67ac38a98%7Ca652971c7d2e4d9ba6a4
>> d1492
>>>>
>> 56f461b%7C0%7C0%7C636831377591864200&amp;sdata=TSpc%2Fzyq2aq0N3
>> %2Bh4o
>>>> ro4std8ut%2FQU6%2BOeMDeeaQdsM%3D&amp;reserved=0
>>>>
>>>>>
>>>>> --
>>>>> MST
>>>
>>>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support
  2019-01-15  8:55                   ` Shahaf Shuler
  2019-01-15 10:23                     ` Ilya Maximets
@ 2019-02-12 17:50                     ` Michael S. Tsirkin
  1 sibling, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2019-02-12 17:50 UTC (permalink / raw)
  To: Shahaf Shuler
  Cc: Ilya Maximets, dev, Maxime Coquelin, Xiao Wang, jfreimann,
	Tiwei Bie, Zhihong Wang, Jason Wang, xiaolong.ye,
	alejandro.lucero, Daniel Marcovitch

On Tue, Jan 15, 2019 at 08:55:50AM +0000, Shahaf Shuler wrote:
> Tuesday, January 15, 2019 10:29 AM, Ilya Maximets:
> > Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
> > ordering feature support
> > 
> > On 15.01.2019 9:33, Shahaf Shuler wrote:
> > > Thursday, January 10, 2019 10:37 PM, Shahaf Shuler:
> > >> Subject: RE: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
> > >> ordering feature support
> > >>
> > >> Wednesday, January 9, 2019 5:50 PM, Michael S. Tsirkin:
> > >>> alejandro.lucero@netronome.com; Daniel Marcovitch
> > >>> <danielm@mellanox.com>
> > >>> Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory
> > >>> ordering feature support
> > >>>
> > >>> On Wed, Jan 09, 2019 at 05:34:38PM +0300, Ilya Maximets wrote:
> > >>>> virtio_mb() is really heavy. I'd like to avoid it somehow, but I
> > >>>> don't know how to do this yet.
> > >>>
> > >>> Linux driver doesn't avoid it either.
> > >>
> > >> I understand v3 was merged but still would like to continue the
> > >> discuss and make sure all is clear and agreed.
> > >>
> > >> Form patch [1] description it is very clear why we need the
> > >> rte_smp_mb() barrier.
> > >> However I am not sure why this barrier is interoperate into rte_mb in
> > >> case of vDPA.  In vDPA case, both read of the user ring and write of
> > >> the avail index are for local cached memory.
> > >> The only write which is to uncachable memory (device memory) is the
> > >> notify itself.
> > >>
> > >> As I mentioned, there is a need to have a store fence before doing
> > >> the notify, but from different reasons. So vDPA use case and need Is
> > >> a bit different than what presented in [1].
> > >
> > > Any answer?
> > > It is pity if we add redundant barriers which will degrade the driver
> > performance.
> > 
> > Sorry for late responses. Have a lot of work with OVS right now.
> > 
> > Regarding your question.
> > Current code looks like this:
> > 
> >  1. Update ring.
> >  2. virtio_wmb()
> >  3. Update idx.
> >  4. virtio_mb()
> >  5. read flags.
> >  6. notify.
> > 
> > virtio_mb() is here to avoid reordering of steps 3 and 5.
> > i.e. we need a full barrier to ensure the order between store (idx update)
> > and load (reading the flags). Otherwise we could miss the notification.
> > We can't avoid the barrier here, because even x86 does not guarantee the
> > ordering of the local load with earlier local store.
> 
> This is clear. You need the rte_smp_mb() here. My question is why you need the rte_mb() in case of vDPA? 
> As you said, all accesses are local. 

Are you asking why the different barriers? Or as you asking why is a barrier
needed at all?

The barriers themselves are clearly needed.

But in my opinion some dpdk barrier implementations are sub-optimal and too
strong.  For example on intel: the big question is whether anyone does
any non-temporals. In absence of these, and with non-cacheable mappings
on x86 wmb and rmb should be a nop, and mb should be a locked instruction.
It might make sense to add rte_dma_rmb/wmb/mb.




> Pasting you commit code:
> /*                                                                        
>  * Per virtio_ring.h in Linux.                                            
>  *     For virtio_pci on SMP, we don't need to order with respect to MMIO 
>  *     accesses through relaxed memory I/O windows, so smp_mb() et al are 
>  *     sufficient.                                                        
>  *                                                                        
>  *     For using virtio to talk to real devices (eg. vDPA) we do need real
>  *     barriers.                                                          
>  */                                                                       
> static inline void                                                        
> virtio_mb(uint8_t weak_barriers)                                          
> {                                                                         
>         if (weak_barriers)                                                
>                 rte_smp_mb();                                             
>         else                                                              
>                 rte_mb();                                                 
> }                                                                         
> 
> > 
> > >
> > >>
> > >> [1]
> > >>
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > >>
> > tches.dpdk.org%2Fpatch%2F49545%2F&amp;data=02%7C01%7Cshahafs%40
> > mellan
> > >>
> > ox.com%7C01907f1b2e0e4002cb7508d67ac38a98%7Ca652971c7d2e4d9ba6a4
> > d1492
> > >>
> > 56f461b%7C0%7C0%7C636831377591864200&amp;sdata=TSpc%2Fzyq2aq0N3
> > %2Bh4o
> > >> ro4std8ut%2FQU6%2BOeMDeeaQdsM%3D&amp;reserved=0
> > >>
> > >>>
> > >>> --
> > >>> MST
> > >
> > >

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2019-02-12 17:51 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20181214153817eucas1p19a41cdd791879252e1f3a5d77c427845@eucas1p1.samsung.com>
2018-12-14 15:38 ` [dpdk-dev] [PATCH] net/virtio: add platform memory ordering feature support Ilya Maximets
2018-12-14 17:00   ` Michael S. Tsirkin
2018-12-14 17:23     ` Ilya Maximets
     [not found]   ` <CGME20181226163717eucas1p15276eb45e35abe2c9cf3e7c1e0050823@eucas1p1.samsung.com>
2018-12-26 16:37     ` [dpdk-dev] [PATCH v2] " Ilya Maximets
2018-12-27 10:07       ` Shahaf Shuler
2019-01-09 14:34         ` Ilya Maximets
2019-01-09 15:50           ` Michael S. Tsirkin
2019-01-10 20:36             ` Shahaf Shuler
2019-01-15  6:33               ` Shahaf Shuler
2019-01-15  8:29                 ` Ilya Maximets
2019-01-15  8:55                   ` Shahaf Shuler
2019-01-15 10:23                     ` Ilya Maximets
2019-02-12 17:50                     ` Michael S. Tsirkin
     [not found]       ` <CGME20190109145021eucas1p1bfe194ffafaaaa5df62243c92b2ed6cd@eucas1p1.samsung.com>
2019-01-09 14:50         ` [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM Ilya Maximets
     [not found]           ` <CGME20190109145027eucas1p2437215de0df4c691eb84d4e84bfc71e5@eucas1p2.samsung.com>
2019-01-09 14:50             ` [dpdk-dev] [PATCH v3 1/3] net/virtio: add missing barrier before reading the flags Ilya Maximets
2019-01-10 14:31               ` Maxime Coquelin
     [not found]           ` <CGME20190109145034eucas1p2183e275e316b87917b96fa184fc7d7cb@eucas1p2.samsung.com>
2019-01-09 14:50             ` [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify Ilya Maximets
2019-01-10  8:19               ` Gavin Hu (Arm Technology China)
2019-01-10  9:18                 ` Maxime Coquelin
2019-01-10  9:55                   ` Ilya Maximets
2019-01-10 14:56                     ` Michael S. Tsirkin
2019-01-10 14:31               ` Maxime Coquelin
     [not found]           ` <CGME20190109145040eucas1p2d9afc678ef94986544bde07b77373e6f@eucas1p2.samsung.com>
2019-01-09 14:50             ` [dpdk-dev] [PATCH v3 3/3] net/virtio: add platform memory ordering feature support Ilya Maximets
2019-01-10 14:31               ` Maxime Coquelin
2019-01-09 14:55           ` [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM Michael S. Tsirkin
2019-01-09 15:24             ` Ilya Maximets
2019-01-09 16:53               ` Ferruh Yigit
2019-01-10 15:19             ` Maxime Coquelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).