[dpdk-dev] [PATCH 0/3] net/virtio: missing/wrong read barriers.

[dpdk-dev] [PATCH 0/3] net/virtio: missing/wrong read barriers.

[Ilya Maximets]

Ilya Maximets (3):
  net/virtio: fix improper read barriers on packed Tx cleanup
  net/virtio: add barriers for extra descriptors on Rx split
  net/virtio: add missing read barrier for packed dequeue

 drivers/net/virtio/virtio_rxtx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/3] net/virtio: fix improper read barriers on packed Tx cleanup

[Ilya Maximets]

Read barrier must be implied between reading descriptor flags
and descriptor id. Otherwise, in case of reordering, we could
read wrong descriptor id.

For the reference, similar barrier for split rings is the read
barrier between VIRTQUEUE_NUSED (reading the used->idx) and
the call to the virtio_xmit_cleanup().

Additionally removed double update of 'used_idx'. It's enough
to set it in the end of the loop.

Fixes: 892dc798fa9c ("net/virtio: implement Tx path for packed queues")
Cc: stable@dpdk.org

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 drivers/net/virtio/virtio_rxtx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index cc476b898..63e4370e4 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -234,7 +234,7 @@ virtio_xmit_cleanup_packed(struct virtqueue *vq, int num)
 
 	used_idx = vq->vq_used_cons_idx;
 	while (num-- && desc_is_used(&desc[used_idx], vq)) {
-		used_idx = vq->vq_used_cons_idx;
+		virtio_rmb(vq->hw->weak_barriers);
 		id = desc[used_idx].id;
 		dxp = &vq->vq_descx[id];
 		vq->vq_used_cons_idx += dxp->ndescs;
@@ -1940,7 +1940,6 @@ 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(hw->weak_barriers);
 			need = RTE_MIN(need, (int)nb_pkts);
 			virtio_xmit_cleanup_packed(vq, need);
 			need = slots - vq->vq_free_cnt;
-- 
2.17.1

[dpdk-dev] [PATCH 2/3] net/virtio: add barriers for extra descriptors on Rx split

[Ilya Maximets]

There should be read barrier between checking VIRTQUEUE_NUSED (reading
the used->idx) and reading these descriptors. It's done for the first
checks at the beginning of these functions but missed while checking
for extra required descriptors.

Fixes: e5f456a98d3c ("net/virtio: support in-order Rx and Tx")
Fixes: 13ce5e7eb94f ("virtio: mergeable buffers")
Cc: stable@dpdk.org

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 drivers/net/virtio/virtio_rxtx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 63e4370e4..5ffed6a51 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1456,6 +1456,7 @@ virtio_recv_pkts_inorder(void *rx_queue,
 
 		prev = rcv_pkts[nb_rx];
 		if (likely(VIRTQUEUE_NUSED(vq) >= rcv_cnt)) {
+			virtio_rmb(hw->weak_barriers);
 			num = virtqueue_dequeue_rx_inorder(vq, rcv_pkts, len,
 							   rcv_cnt);
 			uint16_t extra_idx = 0;
@@ -1642,6 +1643,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 
 		prev = rcv_pkts[nb_rx];
 		if (likely(VIRTQUEUE_NUSED(vq) >= rcv_cnt)) {
+			virtio_rmb(hw->weak_barriers);
 			num = virtqueue_dequeue_burst_rx(vq, rcv_pkts, len,
 							   rcv_cnt);
 			uint16_t extra_idx = 0;
-- 
2.17.1

[dpdk-dev] [PATCH 3/3] net/virtio: add missing read barrier for packed dequeue

[Ilya Maximets]

Read barrier is required between reading the flags (desc_is_used)
and the content of descriptor to ensure the ordering.
Otherwise, speculative read of desc.id could be reordered with
reading of the desc.flags.

Fixes: a76290c8f1cf ("net/virtio: implement Rx path for packed queues")
Cc: stable@dpdk.org

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 drivers/net/virtio/virtio_rxtx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 5ffed6a51..4c701c514 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -124,6 +124,7 @@ virtqueue_dequeue_burst_rx_packed(struct virtqueue *vq,
 		used_idx = vq->vq_used_cons_idx;
 		if (!desc_is_used(&desc[used_idx], vq))
 			return i;
+		virtio_rmb(vq->hw->weak_barriers);
 		len[i] = desc[used_idx].len;
 		id = desc[used_idx].id;
 		cookie = (struct rte_mbuf *)vq->vq_descx[id].cookie;
-- 
2.17.1

Re: [dpdk-dev] [PATCH 0/3] net/virtio: missing/wrong read barriers.

[Tiwei Bie]

On Thu, Jan 24, 2019 at 07:58:59PM +0300, Ilya Maximets wrote:
> Ilya Maximets (3):
>   net/virtio: fix improper read barriers on packed Tx cleanup
>   net/virtio: add barriers for extra descriptors on Rx split
>   net/virtio: add missing read barrier for packed dequeue
> 
>  drivers/net/virtio/virtio_rxtx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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

Re: [dpdk-dev] [PATCH 1/3] net/virtio: fix improper read barriers on packed Tx cleanup

[Jens Freimann]

On Thu, Jan 24, 2019 at 07:59:00PM +0300, Ilya Maximets wrote:
>Read barrier must be implied between reading descriptor flags
>and descriptor id. Otherwise, in case of reordering, we could
>read wrong descriptor id.
>
>For the reference, similar barrier for split rings is the read
>barrier between VIRTQUEUE_NUSED (reading the used->idx) and
>the call to the virtio_xmit_cleanup().
>
>Additionally removed double update of 'used_idx'. It's enough
>to set it in the end of the loop.
>
>Fixes: 892dc798fa9c ("net/virtio: implement Tx path for packed queues")
>Cc: stable@dpdk.org
>
>Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>---
> drivers/net/virtio/virtio_rxtx.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>

Reviewed-by: Jens Freimann <jfreimann@redhat.com>

Re: [dpdk-dev] [PATCH 3/3] net/virtio: add missing read barrier for packed dequeue

[Jens Freimann]

On Thu, Jan 24, 2019 at 07:59:02PM +0300, Ilya Maximets wrote:
>Read barrier is required between reading the flags (desc_is_used)
>and the content of descriptor to ensure the ordering.
>Otherwise, speculative read of desc.id could be reordered with
>reading of the desc.flags.
>
>Fixes: a76290c8f1cf ("net/virtio: implement Rx path for packed queues")
>Cc: stable@dpdk.org
>
>Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>---
> drivers/net/virtio/virtio_rxtx.c | 1 +
> 1 file changed, 1 insertion(+)
>

Reviewed-by: Jens Freimann <jfreimann@redhat.com>

Re: [dpdk-dev] [PATCH 3/3] net/virtio: add missing read barrier for packed dequeue

[Jens Freimann]

On Thu, Jan 24, 2019 at 07:59:02PM +0300, Ilya Maximets wrote:
>Read barrier is required between reading the flags (desc_is_used)
>and the content of descriptor to ensure the ordering.
>Otherwise, speculative read of desc.id could be reordered with
>reading of the desc.flags.
>
>Fixes: a76290c8f1cf ("net/virtio: implement Rx path for packed queues")
>Cc: stable@dpdk.org

I don't thing cc stable is required as a76290c8f1cf is only in
v19.02-rc1 and newer.

regards,
Jens 

Re: [dpdk-dev] [PATCH 1/3] net/virtio: fix improper read barriers on packed Tx cleanup

[Jens Freimann]

On Thu, Jan 24, 2019 at 07:59:00PM +0300, Ilya Maximets wrote:
>Read barrier must be implied between reading descriptor flags
>and descriptor id. Otherwise, in case of reordering, we could
>read wrong descriptor id.
>
>For the reference, similar barrier for split rings is the read
>barrier between VIRTQUEUE_NUSED (reading the used->idx) and
>the call to the virtio_xmit_cleanup().
>
>Additionally removed double update of 'used_idx'. It's enough
>to set it in the end of the loop.
>
>Fixes: 892dc798fa9c ("net/virtio: implement Tx path for packed queues")
>Cc: stable@dpdk.org

I don't think cc stable is required as 892dc798fa9c is only in
v19.02-rc1 and newer.

regards,
Jens 

Re: [dpdk-dev] [PATCH 0/3] net/virtio: missing/wrong read barriers.

[Maxime Coquelin]

Hi Ilya,

On 1/24/19 5:58 PM, Ilya Maximets wrote:
> Ilya Maximets (3):
>    net/virtio: fix improper read barriers on packed Tx cleanup
>    net/virtio: add barriers for extra descriptors on Rx split
>    net/virtio: add missing read barrier for packed dequeue
> 
>   drivers/net/virtio/virtio_rxtx.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 


Thanks for the series, it is sadly too late for v19.02.
I'll pick it as soon as v19.05 merge windo opens.

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

Maxime

Re: [dpdk-dev] [PATCH 0/3] net/virtio: missing/wrong read barriers.

[Maxime Coquelin]

On 1/24/19 5:58 PM, Ilya Maximets wrote:
> Ilya Maximets (3):
>    net/virtio: fix improper read barriers on packed Tx cleanup
>    net/virtio: add barriers for extra descriptors on Rx split
>    net/virtio: add missing read barrier for packed dequeue
> 
>   drivers/net/virtio/virtio_rxtx.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 


Applied to dpdk-next-virtio/master.

Thanks,
Maxime