diff mbox series

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

Message ID 20190124165902.24178-2-i.maximets@samsung.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: missing/wrong read barriers. |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS

Commit Message

Ilya Maximets Jan. 24, 2019, 4:59 p.m. UTC 
  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(-)

Comments

Jens Freimann Jan. 25, 2019, 12:43 p.m. UTC | #1 
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>
Jens Freimann Jan. 25, 2019, 12:49 p.m. UTC | #2 
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
diff mbox series

Patch

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;