[dpdk-dev] [PATCH] net/virtio: add platform memory ordering feature support

[dpdk-dev] [PATCH] net/virtio: add platform memory ordering feature support

[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

Re: [dpdk-dev] [PATCH] net/virtio: add platform memory ordering feature support

[Michael S. Tsirkin]

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

Re: [dpdk-dev] [PATCH] net/virtio: add platform memory ordering feature support

[Ilya Maximets]

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
> 
> 

[dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

[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

Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

[Shahaf Shuler]

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

Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

[Ilya Maximets]

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
> 

[dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM.

[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

[dpdk-dev] [PATCH v3 1/3] net/virtio: add missing barrier before reading the flags

[Ilya Maximets]

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

[dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify

[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

[dpdk-dev] [PATCH v3 3/3] net/virtio: add platform memory ordering feature support

[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

Re: [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM.

[Michael S. Tsirkin]

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

Re: [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM.

[Ilya Maximets]

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
> 
> 

Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

[Michael S. Tsirkin]

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

Re: [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM.

[Ferruh Yigit]

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
>>
>>

Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify

[Gavin Hu (Arm Technology China)]

> -----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

Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify

[Maxime Coquelin]

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
> 

Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify

[Ilya Maximets]

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
>>
> 
> 

Re: [dpdk-dev] [PATCH v3 1/3] net/virtio: add missing barrier before reading the flags

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH v3 3/3] net/virtio: add platform memory ordering feature support

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify

[Michael S. Tsirkin]

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
> >>
> > 
> > 

Re: [dpdk-dev] [PATCH v3 0/3] Missing barriers and VIRTIO_F_ORDER_PLATFORM.

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

[Shahaf Shuler]

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

Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

[Shahaf Shuler]

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

Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

[Ilya Maximets]

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
> 
> 

Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

[Shahaf Shuler]

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
> >
> >

Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

[Ilya Maximets]

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
>>>
>>>

Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

[Michael S. Tsirkin]

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
> > >
> > >