[v2,3/4] vhost: rework async dequeue path error handling

Message ID 20250115125938.2699577-4-maxime.coquelin@redhat.com (mailing list archive)
State Superseded
Delegated to: Maxime Coquelin
Headers
Series vhost: fix and improve dequeue error path |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Maxime Coquelin Jan. 15, 2025, 12:59 p.m. UTC
This patch refactors the error handling in the Vhost async
dequeue path to ease its maintenance and readability.

Suggested-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/virtio_net.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)
  

Comments

David Marchand Jan. 15, 2025, 4:42 p.m. UTC | #1
On Wed, Jan 15, 2025 at 1:59 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch refactors the error handling in the Vhost async
> dequeue path to ease its maintenance and readability.
>
> Suggested-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Reviewed-by: David Marchand <david.marchand@redhat.com>
  
David Marchand Jan. 15, 2025, 4:49 p.m. UTC | #2
On Wed, Jan 15, 2025 at 5:42 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Wed, Jan 15, 2025 at 1:59 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
> >
> > This patch refactors the error handling in the Vhost async
> > dequeue path to ease its maintenance and readability.
> >
> > Suggested-by: David Marchand <david.marchand@redhat.com>
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> Reviewed-by: David Marchand <david.marchand@redhat.com>

Btw, I would squash patch 2 and 3 (especially as you updated both sync
and async code at the same time in patch 4).
  
Maxime Coquelin Jan. 16, 2025, 9:51 a.m. UTC | #3
On 1/15/25 5:49 PM, David Marchand wrote:
> On Wed, Jan 15, 2025 at 5:42 PM David Marchand
> <david.marchand@redhat.com> wrote:
>>
>> On Wed, Jan 15, 2025 at 1:59 PM Maxime Coquelin
>> <maxime.coquelin@redhat.com> wrote:
>>>
>>> This patch refactors the error handling in the Vhost async
>>> dequeue path to ease its maintenance and readability.
>>>
>>> Suggested-by: David Marchand <david.marchand@redhat.com>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> Btw, I would squash patch 2 and 3 (especially as you updated both sync
> and async code at the same time in patch 4).
> 
> 

Ack, I will squash both patches.

Thanks,
Maxime
  

Patch

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 3a4955fd30..59ea2d16a5 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -4197,52 +4197,51 @@  rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
 	struct rte_mbuf *rarp_mbuf = NULL;
 	struct vhost_virtqueue *vq;
 	int16_t success = 1;
+	uint16_t nb_rx = 0;
 
 	dev = get_device(vid);
 	if (!dev || !nr_inflight)
-		return 0;
+		goto out_no_unlock;
 
 	*nr_inflight = -1;
 
 	if (unlikely(!(dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET))) {
 		VHOST_DATA_LOG(dev->ifname, ERR, "%s: built-in vhost net backend is disabled.",
 			__func__);
-		return 0;
+		goto out_no_unlock;
 	}
 
 	if (unlikely(!is_valid_virt_queue_idx(queue_id, 1, dev->nr_vring))) {
 		VHOST_DATA_LOG(dev->ifname, ERR, "%s: invalid virtqueue idx %d.",
 			__func__, queue_id);
-		return 0;
+		goto out_no_unlock;
 	}
 
 	if (unlikely(dma_id < 0 || dma_id >= RTE_DMADEV_DEFAULT_MAX)) {
 		VHOST_DATA_LOG(dev->ifname, ERR, "%s: invalid dma id %d.",
 			__func__, dma_id);
-		return 0;
+		goto out_no_unlock;
 	}
 
 	if (unlikely(!dma_copy_track[dma_id].vchans ||
 				!dma_copy_track[dma_id].vchans[vchan_id].pkts_cmpl_flag_addr)) {
 		VHOST_DATA_LOG(dev->ifname, ERR, "%s: invalid channel %d:%u.",
 			__func__, dma_id, vchan_id);
-		return 0;
+		goto out_no_unlock;
 	}
 
 	vq = dev->virtqueue[queue_id];
 
 	if (unlikely(rte_rwlock_read_trylock(&vq->access_lock) != 0))
-		return 0;
+		goto out_no_unlock;
 
 	if (unlikely(vq->enabled == 0)) {
-		count = 0;
 		goto out_access_unlock;
 	}
 
 	if (unlikely(!vq->async)) {
 		VHOST_DATA_LOG(dev->ifname, ERR, "%s: async not registered for queue id %d.",
 			__func__, queue_id);
-		count = 0;
 		goto out_access_unlock;
 	}
 
@@ -4253,7 +4252,6 @@  rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
 		rte_rwlock_read_unlock(&vq->access_lock);
 
 		virtio_dev_vring_translate(dev, vq);
-		count = 0;
 		goto out_no_unlock;
 	}
 
@@ -4280,7 +4278,6 @@  rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
 		rarp_mbuf = rte_net_make_rarp_packet(mbuf_pool, &dev->mac);
 		if (rarp_mbuf == NULL) {
 			VHOST_DATA_LOG(dev->ifname, ERR, "failed to make RARP packet.");
-			count = 0;
 			goto out;
 		}
 		/*
@@ -4295,22 +4292,22 @@  rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
 
 	if (vq_is_packed(dev)) {
 		if (dev->flags & VIRTIO_DEV_LEGACY_OL_FLAGS)
-			count = virtio_dev_tx_async_packed_legacy(dev, vq, mbuf_pool,
+			nb_rx = virtio_dev_tx_async_packed_legacy(dev, vq, mbuf_pool,
 					pkts, count, dma_id, vchan_id);
 		else
-			count = virtio_dev_tx_async_packed_compliant(dev, vq, mbuf_pool,
+			nb_rx = virtio_dev_tx_async_packed_compliant(dev, vq, mbuf_pool,
 					pkts, count, dma_id, vchan_id);
 	} else {
 		if (dev->flags & VIRTIO_DEV_LEGACY_OL_FLAGS)
-			count = virtio_dev_tx_async_split_legacy(dev, vq, mbuf_pool,
+			nb_rx = virtio_dev_tx_async_split_legacy(dev, vq, mbuf_pool,
 					pkts, count, dma_id, vchan_id);
 		else
-			count = virtio_dev_tx_async_split_compliant(dev, vq, mbuf_pool,
+			nb_rx = virtio_dev_tx_async_split_compliant(dev, vq, mbuf_pool,
 					pkts, count, dma_id, vchan_id);
 	}
 
 	*nr_inflight = vq->async->pkts_inflight_n;
-	vhost_queue_stats_update(dev, vq, pkts, count);
+	vhost_queue_stats_update(dev, vq, pkts, nb_rx);
 
 out:
 	vhost_user_iotlb_rd_unlock(vq);
@@ -4319,8 +4316,8 @@  rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
 	rte_rwlock_read_unlock(&vq->access_lock);
 
 	if (unlikely(rarp_mbuf != NULL))
-		count += 1;
+		nb_rx += 1;
 
 out_no_unlock:
-	return count;
+	return nb_rx;
 }