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

Message ID 20190124165902.24178-2-i.maximets@samsung.com
State Accepted, archived
Delegated to: Maxime Coquelin
Headers show
Series
  • net/virtio: missing/wrong read barriers.
Related show

Checks

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

Commit Message

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

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;