[dpdk-dev] [PATCH v2 0/4] vhost: add missing barriers, move prefetching

[dpdk-dev] [PATCH v2 0/4] vhost: add missing barriers, move prefetching

[Maxime Coquelin]

This series adds missing read barriers after reading avail index
for split ring and desc flags for packed ring.

Also, it turns out that some descriptors prefetching are either
badly placed, or useless, last part of the series fixes that.

With the series applied, I get between 0 and 4% gain depending
on the benchmark (testpmd txonly/rxonly/io).

Thanks to Jason for reporting the missing read barriers.

Changes since v1:
=================
- Drop volatile removal patch (Ilya)
- Improve commit messages for RMB patches (Ilya)

Maxime Coquelin (4):
  vhost: enforce avail index and desc read ordering
  vhost: enforce desc flags and content read ordering
  vhost: prefetch descriptor after the read barrier
  vhost: remove useless prefetch for packed ring descriptor

 lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v2 1/4] vhost: enforce avail index and desc read ordering

[Maxime Coquelin]

A read barrier is required to ensure the ordering between
available index and the descriptor reads is enforced.

1. read avail_head = avail->idx
2. read cur_idx = last_avail_idx
if (cur_idx != avail_head) {
    3. read idx = avail->ring[cur_idx]
    4. read desc[idx]
}

There is a control dependency between step 1 and steps 3 & 4,
3 could be speculatively executed before 1, which could result
in 'idx' to not being updated yet.

Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
Cc: stable@dpdk.org

Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Acked-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/librte_vhost/virtio_net.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 8c657a101..7f37bbbed 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -752,6 +752,12 @@ virtio_dev_rx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
 	avail_head = *((volatile uint16_t *)&vq->avail->idx);
 
+	/*
+	 * The ordering between avail index and
+	 * desc reads needs to be enforced.
+	 */
+	rte_smp_rmb();
+
 	for (pkt_idx = 0; pkt_idx < count; pkt_idx++) {
 		uint32_t pkt_len = pkts[pkt_idx]->pkt_len + dev->vhost_hlen;
 		uint16_t nr_vec = 0;
@@ -1334,6 +1340,12 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	if (free_entries == 0)
 		return 0;
 
+	/*
+	 * The ordering between avail index and
+	 * desc reads needs to be enforced.
+	 */
+	rte_smp_rmb();
+
 	VHOST_LOG_DEBUG(VHOST_DATA, "(%d) %s\n", dev->vid, __func__);
 
 	count = RTE_MIN(count, MAX_PKT_BURST);
-- 
2.17.2

[dpdk-dev] [PATCH v2 2/4] vhost: enforce desc flags and content read ordering

[Maxime Coquelin]

A read barrier is required to ensure that the ordering between
descriptor's flags and content reads is enforced.

1. read flags = desc->flags
if (flags & AVAIL_BIT)
2.   read desc->id

There is a control dependency between steps 1 and step 2.
2 could be speculatively executed before 1, which could result
in 'id' to not be updated yet.

Fixes: 2f3225a7d69b ("vhost: add vector filling support for packed ring")
Cc: stable@dpdk.org

Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Acked-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/librte_vhost/virtio_net.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 7f37bbbed..a9b1bc55f 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -481,6 +481,12 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	if (unlikely(!desc_is_avail(&descs[avail_idx], wrap_counter)))
 		return -1;
 
+	/*
+	 * The ordering between desc flags and desc
+	 * content reads need to be enforced.
+	 */
+	rte_smp_rmb();
+
 	*desc_count = 0;
 	*len = 0;
 
-- 
2.17.2

[dpdk-dev] [PATCH v2 3/4] vhost: prefetch descriptor after the read barrier

[Maxime Coquelin]

This patch moves the prefetch after the available index
is read to avoid prefetching a descriptor not available yet.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/virtio_net.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index a9b1bc55f..a9be633de 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -755,7 +755,6 @@ virtio_dev_rx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	struct buf_vector buf_vec[BUF_VECTOR_MAX];
 	uint16_t avail_head;
 
-	rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
 	avail_head = *((volatile uint16_t *)&vq->avail->idx);
 
 	/*
@@ -764,6 +763,8 @@ virtio_dev_rx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	 */
 	rte_smp_rmb();
 
+	rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
+
 	for (pkt_idx = 0; pkt_idx < count; pkt_idx++) {
 		uint32_t pkt_len = pkts[pkt_idx]->pkt_len + dev->vhost_hlen;
 		uint16_t nr_vec = 0;
@@ -1339,8 +1340,6 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		}
 	}
 
-	rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
-
 	free_entries = *((volatile uint16_t *)&vq->avail->idx) -
 			vq->last_avail_idx;
 	if (free_entries == 0)
@@ -1352,6 +1351,8 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	 */
 	rte_smp_rmb();
 
+	rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
+
 	VHOST_LOG_DEBUG(VHOST_DATA, "(%d) %s\n", dev->vid, __func__);
 
 	count = RTE_MIN(count, MAX_PKT_BURST);
-- 
2.17.2

[dpdk-dev] [PATCH v2 4/4] vhost: remove useless prefetch for packed ring descriptor

[Maxime Coquelin]

This prefetch does not show any performance improvement.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/virtio_net.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index a9be633de..4d201a3fd 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -1437,8 +1437,6 @@ virtio_dev_tx_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
 {
 	uint16_t i;
 
-	rte_prefetch0(&vq->desc_packed[vq->last_avail_idx]);
-
 	if (unlikely(dev->dequeue_zero_copy)) {
 		struct zcopy_mbuf *zmbuf, *next;
 
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 1/4] vhost: enforce avail index and desc read ordering

[Michael S. Tsirkin]

On Wed, Dec 19, 2018 at 09:21:10AM +0100, Maxime Coquelin wrote:
> A read barrier is required to ensure the ordering between
> available index and the descriptor reads is enforced.
> 
> 1. read avail_head = avail->idx
> 2. read cur_idx = last_avail_idx
> if (cur_idx != avail_head) {
>     3. read idx = avail->ring[cur_idx]
>     4. read desc[idx]
> }
> 
> There is a control dependency between step 1 and steps 3 & 4,
> 3 could be speculatively executed before 1, which could result
> in 'idx' to not being updated yet.
> 
> Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
> Cc: stable@dpdk.org
> 
> Reported-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Acked-by: Ilya Maximets <i.maximets@samsung.com>

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

BTW Ilya do you see a performance degradation from RMBs with these patches?


> ---
>  lib/librte_vhost/virtio_net.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index 8c657a101..7f37bbbed 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -752,6 +752,12 @@ virtio_dev_rx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  	rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
>  	avail_head = *((volatile uint16_t *)&vq->avail->idx);
>  
> +	/*
> +	 * The ordering between avail index and
> +	 * desc reads needs to be enforced.
> +	 */
> +	rte_smp_rmb();
> +

I'd guess you want to put the RMB before the prefetch. No?
Otherwise I think you are either stalling until that completes
or discard the prefetch ...


>  	for (pkt_idx = 0; pkt_idx < count; pkt_idx++) {
>  		uint32_t pkt_len = pkts[pkt_idx]->pkt_len + dev->vhost_hlen;
>  		uint16_t nr_vec = 0;
> @@ -1334,6 +1340,12 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  	if (free_entries == 0)
>  		return 0;
>  
> +	/*
> +	 * The ordering between avail index and
> +	 * desc reads needs to be enforced.
> +	 */
> +	rte_smp_rmb();
> +
>  	VHOST_LOG_DEBUG(VHOST_DATA, "(%d) %s\n", dev->vid, __func__);
>  
>  	count = RTE_MIN(count, MAX_PKT_BURST);
> -- 
> 2.17.2

Re: [dpdk-dev] [PATCH v2 4/4] vhost: remove useless prefetch for packed ring descriptor

[Michael S. Tsirkin]

On Wed, Dec 19, 2018 at 09:21:13AM +0100, Maxime Coquelin wrote:
> This prefetch does not show any performance improvement.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Likely because of the RMB.
Try prefetching the *next* descriptor maybe?

> ---
>  lib/librte_vhost/virtio_net.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index a9be633de..4d201a3fd 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -1437,8 +1437,6 @@ virtio_dev_tx_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  {
>  	uint16_t i;
>  
> -	rte_prefetch0(&vq->desc_packed[vq->last_avail_idx]);
> -
>  	if (unlikely(dev->dequeue_zero_copy)) {
>  		struct zcopy_mbuf *zmbuf, *next;
>  
> -- 
> 2.17.2

Re: [dpdk-dev] [PATCH v2 1/4] vhost: enforce avail index and desc read ordering

[Maxime Coquelin]

On 12/19/18 4:47 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 19, 2018 at 09:21:10AM +0100, Maxime Coquelin wrote:
>> A read barrier is required to ensure the ordering between
>> available index and the descriptor reads is enforced.
>>
>> 1. read avail_head = avail->idx
>> 2. read cur_idx = last_avail_idx
>> if (cur_idx != avail_head) {
>>      3. read idx = avail->ring[cur_idx]
>>      4. read desc[idx]
>> }
>>
>> There is a control dependency between step 1 and steps 3 & 4,
>> 3 could be speculatively executed before 1, which could result
>> in 'idx' to not being updated yet.
>>
>> Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
>> Cc: stable@dpdk.org
>>
>> Reported-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Acked-by: Ilya Maximets <i.maximets@samsung.com>
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> BTW Ilya do you see a performance degradation from RMBs with these patches?
> 
> 
>> ---
>>   lib/librte_vhost/virtio_net.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
>> index 8c657a101..7f37bbbed 100644
>> --- a/lib/librte_vhost/virtio_net.c
>> +++ b/lib/librte_vhost/virtio_net.c
>> @@ -752,6 +752,12 @@ virtio_dev_rx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>   	rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
>>   	avail_head = *((volatile uint16_t *)&vq->avail->idx);
>>   
>> +	/*
>> +	 * The ordering between avail index and
>> +	 * desc reads needs to be enforced.
>> +	 */
>> +	rte_smp_rmb();
>> +
> 
> I'd guess you want to put the RMB before the prefetch. No?
> Otherwise I think you are either stalling until that completes
> or discard the prefetch ...

Yeah, this is actually done in patch 3.

Thanks,
Maxime
>>   	for (pkt_idx = 0; pkt_idx < count; pkt_idx++) {
>>   		uint32_t pkt_len = pkts[pkt_idx]->pkt_len + dev->vhost_hlen;
>>   		uint16_t nr_vec = 0;
>> @@ -1334,6 +1340,12 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>   	if (free_entries == 0)
>>   		return 0;
>>   
>> +	/*
>> +	 * The ordering between avail index and
>> +	 * desc reads needs to be enforced.
>> +	 */
>> +	rte_smp_rmb();
>> +
>>   	VHOST_LOG_DEBUG(VHOST_DATA, "(%d) %s\n", dev->vid, __func__);
>>   
>>   	count = RTE_MIN(count, MAX_PKT_BURST);
>> -- 
>> 2.17.2

Re: [dpdk-dev] [PATCH v2 0/4] vhost: add missing barriers, move prefetching

[Michael S. Tsirkin]

On Wed, Dec 19, 2018 at 09:21:09AM +0100, Maxime Coquelin wrote:
> This series adds missing read barriers after reading avail index
> for split ring and desc flags for packed ring.
> 
> Also, it turns out that some descriptors prefetching are either
> badly placed, or useless, last part of the series fixes that.
> 
> With the series applied, I get between 0 and 4% gain depending
> on the benchmark (testpmd txonly/rxonly/io).
> 
> Thanks to Jason for reporting the missing read barriers.

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

But I wonder what effect this has on ARM where RMB isn't a NOP.
Ilya do you happen to have any data?

> Changes since v1:
> =================
> - Drop volatile removal patch (Ilya)
> - Improve commit messages for RMB patches (Ilya)
> 
> Maxime Coquelin (4):
>   vhost: enforce avail index and desc read ordering
>   vhost: enforce desc flags and content read ordering
>   vhost: prefetch descriptor after the read barrier
>   vhost: remove useless prefetch for packed ring descriptor
> 
>  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> -- 
> 2.17.2

Re: [dpdk-dev] [PATCH v2 0/4] vhost: add missing barriers, move prefetching

[Ilya Maximets]

On 19.12.2018 18:50, Michael S. Tsirkin wrote:
> On Wed, Dec 19, 2018 at 09:21:09AM +0100, Maxime Coquelin wrote:
>> This series adds missing read barriers after reading avail index
>> for split ring and desc flags for packed ring.
>>
>> Also, it turns out that some descriptors prefetching are either
>> badly placed, or useless, last part of the series fixes that.
>>
>> With the series applied, I get between 0 and 4% gain depending
>> on the benchmark (testpmd txonly/rxonly/io).
>>
>> Thanks to Jason for reporting the missing read barriers.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> But I wonder what effect this has on ARM where RMB isn't a NOP.
> Ilya do you happen to have any data?

Hi.
My rough testing shows no significant performance difference on ARMv8.

> 
>> Changes since v1:
>> =================
>> - Drop volatile removal patch (Ilya)
>> - Improve commit messages for RMB patches (Ilya)
>>
>> Maxime Coquelin (4):
>>   vhost: enforce avail index and desc read ordering
>>   vhost: enforce desc flags and content read ordering
>>   vhost: prefetch descriptor after the read barrier
>>   vhost: remove useless prefetch for packed ring descriptor
>>
>>  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> -- 
>> 2.17.2
> 
> 

Re: [dpdk-dev] [PATCH v2 0/4] vhost: add missing barriers, move prefetching

[Tiwei Bie]

On Wed, Dec 19, 2018 at 09:21:09AM +0100, Maxime Coquelin wrote:
> This series adds missing read barriers after reading avail index
> for split ring and desc flags for packed ring.
> 
> Also, it turns out that some descriptors prefetching are either
> badly placed, or useless, last part of the series fixes that.
> 
> With the series applied, I get between 0 and 4% gain depending
> on the benchmark (testpmd txonly/rxonly/io).
> 
> Thanks to Jason for reporting the missing read barriers.
> 
> Changes since v1:
> =================
> - Drop volatile removal patch (Ilya)
> - Improve commit messages for RMB patches (Ilya)
> 
> Maxime Coquelin (4):
>   vhost: enforce avail index and desc read ordering
>   vhost: enforce desc flags and content read ordering
>   vhost: prefetch descriptor after the read barrier
>   vhost: remove useless prefetch for packed ring descriptor
> 
>  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)

Acked-by: Tiwei Bie <tiwei.bie@intel.com>

Re: [dpdk-dev] [PATCH v2 0/4] vhost: add missing barriers, move prefetching

[Maxime Coquelin]

On 12/19/18 9:21 AM, Maxime Coquelin wrote:
> This series adds missing read barriers after reading avail index
> for split ring and desc flags for packed ring.
> 
> Also, it turns out that some descriptors prefetching are either
> badly placed, or useless, last part of the series fixes that.
> 
> With the series applied, I get between 0 and 4% gain depending
> on the benchmark (testpmd txonly/rxonly/io).
> 
> Thanks to Jason for reporting the missing read barriers.
> 
> Changes since v1:
> =================
> - Drop volatile removal patch (Ilya)
> - Improve commit messages for RMB patches (Ilya)
> 
> Maxime Coquelin (4):
>    vhost: enforce avail index and desc read ordering
>    vhost: enforce desc flags and content read ordering
>    vhost: prefetch descriptor after the read barrier
>    vhost: remove useless prefetch for packed ring descriptor
> 
>   lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
> 

Applied to dpdk-next-virtio.

Maxime