[PATCH] examples/vhost: Fix retry logic on Rx

[PATCH] examples/vhost: Fix retry logic on Rx

[Yuan Wang]

drain_eth_rx() uses rte_vhost_avail_entries() to calculate
the available entries to determine if a retry is required.
However, this function only works with split rings, and
calculating packed rings will return the wrong value and cause
unnecessary retries resulting in a significant performance penalty.

This patch uses the difference between tx burst and rx burst
as a retry condition, and introduces enqueue_pkts() to reduce
code duplication.

Fixes: 4ecf22e356 ("vhost: export device id as the interface to applications")
Cc: stable@dpdk.org

Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
---
 examples/vhost/main.c | 78 ++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 46 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index c4d46de1c5..198bf9dc4a 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -599,7 +599,7 @@ us_vhost_usage(const char *prgname)
 {
 	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p PORTMASK\n"
 	"		--vm2vm [0|1|2]\n"
-	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
+	"		--rx-retry [0|1] --mergeable [0|1] --stats [0-N]\n"
 	"		--socket-file <path>\n"
 	"		--nb-devices ND\n"
 	"		-p PORTMASK: Set mask for ports to be used by application\n"
@@ -1021,31 +1021,43 @@ sync_virtio_xmit(struct vhost_dev *dst_vdev, struct vhost_dev *src_vdev,
 	}
 }
 
-static __rte_always_inline void
-drain_vhost(struct vhost_dev *vdev)
+static __rte_always_inline uint16_t
+enqueue_pkts(struct vhost_dev *vdev, struct rte_mbuf **pkts, uint16_t rx_count)
 {
-	uint16_t ret;
-	uint32_t buff_idx = rte_lcore_id() * RTE_MAX_VHOST_DEVICE + vdev->vid;
-	uint16_t nr_xmit = vhost_txbuff[buff_idx]->len;
-	struct rte_mbuf **m = vhost_txbuff[buff_idx]->m_table;
+	uint16_t enqueue_count;
 
 	if (builtin_net_driver) {
-		ret = vs_enqueue_pkts(vdev, VIRTIO_RXQ, m, nr_xmit);
+		enqueue_count = vs_enqueue_pkts(vdev, VIRTIO_RXQ, pkts, rx_count);
 	} else if (dma_bind[vdev->vid].dmas[VIRTIO_RXQ].async_enabled) {
 		uint16_t enqueue_fail = 0;
 		int16_t dma_id = dma_bind[vdev->vid].dmas[VIRTIO_RXQ].dev_id;
 
 		complete_async_pkts(vdev);
-		ret = rte_vhost_submit_enqueue_burst(vdev->vid, VIRTIO_RXQ, m, nr_xmit, dma_id, 0);
+		enqueue_count = rte_vhost_submit_enqueue_burst(vdev->vid,
+					VIRTIO_RXQ, pkts, rx_count, dma_id, 0);
 
-		enqueue_fail = nr_xmit - ret;
+		enqueue_fail = rx_count - enqueue_count;
 		if (enqueue_fail)
-			free_pkts(&m[ret], nr_xmit - ret);
+			free_pkts(&pkts[enqueue_count], enqueue_fail);
+
 	} else {
-		ret = rte_vhost_enqueue_burst(vdev->vid, VIRTIO_RXQ,
-						m, nr_xmit);
+		enqueue_count = rte_vhost_enqueue_burst(vdev->vid, VIRTIO_RXQ,
+						pkts, rx_count);
 	}
 
+	return enqueue_count;
+}
+
+static __rte_always_inline void
+drain_vhost(struct vhost_dev *vdev)
+{
+	uint16_t ret;
+	uint32_t buff_idx = rte_lcore_id() * RTE_MAX_VHOST_DEVICE + vdev->vid;
+	uint16_t nr_xmit = vhost_txbuff[buff_idx]->len;
+	struct rte_mbuf **m = vhost_txbuff[buff_idx]->m_table;
+
+	ret = enqueue_pkts(vdev, m, nr_xmit);
+
 	if (enable_stats) {
 		__atomic_add_fetch(&vdev->stats.rx_total_atomic, nr_xmit,
 				__ATOMIC_SEQ_CST);
@@ -1337,44 +1349,18 @@ drain_eth_rx(struct vhost_dev *vdev)
 	if (!rx_count)
 		return;
 
-	/*
-	 * When "enable_retry" is set, here we wait and retry when there
-	 * is no enough free slots in the queue to hold @rx_count packets,
-	 * to diminish packet loss.
-	 */
-	if (enable_retry &&
-	    unlikely(rx_count > rte_vhost_avail_entries(vdev->vid,
-			VIRTIO_RXQ))) {
-		uint32_t retry;
+	enqueue_count = enqueue_pkts(vdev, pkts, rx_count);
 
-		for (retry = 0; retry < burst_rx_retry_num; retry++) {
+	/* Retry if necessary */
+	if (unlikely(enqueue_count < rx_count) && enable_retry) {
+		uint32_t retry = 0;
+
+		while (enqueue_count < rx_count && retry++ < burst_rx_retry_num) {
 			rte_delay_us(burst_rx_delay_time);
-			if (rx_count <= rte_vhost_avail_entries(vdev->vid,
-					VIRTIO_RXQ))
-				break;
+			enqueue_count += enqueue_pkts(vdev, pkts, rx_count - enqueue_count);
 		}
 	}
 
-	if (builtin_net_driver) {
-		enqueue_count = vs_enqueue_pkts(vdev, VIRTIO_RXQ,
-						pkts, rx_count);
-	} else if (dma_bind[vdev->vid].dmas[VIRTIO_RXQ].async_enabled) {
-		uint16_t enqueue_fail = 0;
-		int16_t dma_id = dma_bind[vdev->vid].dmas[VIRTIO_RXQ].dev_id;
-
-		complete_async_pkts(vdev);
-		enqueue_count = rte_vhost_submit_enqueue_burst(vdev->vid,
-					VIRTIO_RXQ, pkts, rx_count, dma_id, 0);
-
-		enqueue_fail = rx_count - enqueue_count;
-		if (enqueue_fail)
-			free_pkts(&pkts[enqueue_count], enqueue_fail);
-
-	} else {
-		enqueue_count = rte_vhost_enqueue_burst(vdev->vid, VIRTIO_RXQ,
-						pkts, rx_count);
-	}
-
 	if (enable_stats) {
 		__atomic_add_fetch(&vdev->stats.rx_total_atomic, rx_count,
 				__ATOMIC_SEQ_CST);
-- 
2.25.1

RE: [PATCH] examples/vhost: Fix retry logic on Rx

[Ling, WeiX]

> -----Original Message-----
> From: Wang, YuanX <yuanx.wang@intel.com>
> Sent: Thursday, May 19, 2022 12:25 AM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> <xingguang.he@intel.com>; Ling, WeiX <weix.ling@intel.com>; Wang, YuanX
> <yuanx.wang@intel.com>; stable@dpdk.org
> Subject: [PATCH] examples/vhost: Fix retry logic on Rx
> 
> drain_eth_rx() uses rte_vhost_avail_entries() to calculate the available
> entries to determine if a retry is required.
> However, this function only works with split rings, and calculating packed
> rings will return the wrong value and cause unnecessary retries resulting in a
> significant performance penalty.
> 
> This patch uses the difference between tx burst and rx burst as a retry
> condition, and introduces enqueue_pkts() to reduce code duplication.
> 
> Fixes: 4ecf22e356 ("vhost: export device id as the interface to applications")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> ---

Tested-by: Wei Ling <weix.ling@intel.com>

[PATCH v2] examples/vhost: fix retry logic on eth rx path

[Yuan Wang]

drain_eth_rx() uses rte_vhost_avail_entries() to calculate
the available entries to determine if a retry is required.
However, this function only works with split rings, and
calculating packed rings will return the wrong value and cause
unnecessary retries resulting in a significant performance penalty.

This patch fix that by using the difference between tx/rx burst
as the retry condition.

Fixes: 4ecf22e356de ("vhost: export device id as the interface to applications")
Cc: stable@dpdk.org

Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
Tested-by: Wei Ling <weix.ling@intel.com>
---
V2: Rebase to 22.07 rc1.
---
 examples/vhost/main.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index e7fee5aa1b..e7a84333ed 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -634,7 +634,7 @@ us_vhost_usage(const char *prgname)
 {
 	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p PORTMASK\n"
 	"		--vm2vm [0|1|2]\n"
-	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
+	"		--rx-retry [0|1] --mergeable [0|1] --stats [0-N]\n"
 	"		--socket-file <path>\n"
 	"		--nb-devices ND\n"
 	"		-p PORTMASK: Set mask for ports to be used by application\n"
@@ -1383,27 +1383,20 @@ drain_eth_rx(struct vhost_dev *vdev)
 	if (!rx_count)
 		return;
 
-	/*
-	 * When "enable_retry" is set, here we wait and retry when there
-	 * is no enough free slots in the queue to hold @rx_count packets,
-	 * to diminish packet loss.
-	 */
-	if (enable_retry &&
-	    unlikely(rx_count > rte_vhost_avail_entries(vdev->vid,
-			VIRTIO_RXQ))) {
-		uint32_t retry;
+	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
+						VIRTIO_RXQ, pkts, rx_count);
 
-		for (retry = 0; retry < burst_rx_retry_num; retry++) {
+	/* Retry if necessary */
+	if (enable_retry && unlikely(enqueue_count < rx_count)) {
+		uint32_t retry = 0;
+
+		while (enqueue_count < rx_count && retry++ < burst_rx_retry_num) {
 			rte_delay_us(burst_rx_delay_time);
-			if (rx_count <= rte_vhost_avail_entries(vdev->vid,
-					VIRTIO_RXQ))
-				break;
+			enqueue_count += vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
+								VIRTIO_RXQ, pkts, rx_count);
 		}
 	}
 
-	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
-					VIRTIO_RXQ, pkts, rx_count);
-
 	if (enable_stats) {
 		__atomic_add_fetch(&vdev->stats.rx_total_atomic, rx_count,
 				__ATOMIC_SEQ_CST);
-- 
2.25.1

RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path

[Xia, Chenbo]

> -----Original Message-----
> From: Wang, YuanX <yuanx.wang@intel.com>
> Sent: Friday, June 17, 2022 3:02 PM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> dev@dpdk.org
> Cc: Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> Wang, YuanX <yuanx.wang@intel.com>; stable@dpdk.org; Ling, WeiX
> <weix.ling@intel.com>
> Subject: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> 
> drain_eth_rx() uses rte_vhost_avail_entries() to calculate
> the available entries to determine if a retry is required.
> However, this function only works with split rings, and
> calculating packed rings will return the wrong value and cause
> unnecessary retries resulting in a significant performance penalty.
> 
> This patch fix that by using the difference between tx/rx burst
> as the retry condition.

Does it mean we don't need the API rte_vhost_avail_entries() anymore?

Jiayu/Yuan/Maxime, what do you think?

Thanks,
Chenbo

> 
> Fixes: 4ecf22e356de ("vhost: export device id as the interface to
> applications")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> Tested-by: Wei Ling <weix.ling@intel.com>
> ---
> V2: Rebase to 22.07 rc1.
> ---
>  examples/vhost/main.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index e7fee5aa1b..e7a84333ed 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -634,7 +634,7 @@ us_vhost_usage(const char *prgname)
>  {
>  	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p PORTMASK\n"
>  	"		--vm2vm [0|1|2]\n"
> -	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
> +	"		--rx-retry [0|1] --mergeable [0|1] --stats [0-N]\n"
>  	"		--socket-file <path>\n"
>  	"		--nb-devices ND\n"
>  	"		-p PORTMASK: Set mask for ports to be used by
> application\n"
> @@ -1383,27 +1383,20 @@ drain_eth_rx(struct vhost_dev *vdev)
>  	if (!rx_count)
>  		return;
> 
> -	/*
> -	 * When "enable_retry" is set, here we wait and retry when there
> -	 * is no enough free slots in the queue to hold @rx_count packets,
> -	 * to diminish packet loss.
> -	 */
> -	if (enable_retry &&
> -	    unlikely(rx_count > rte_vhost_avail_entries(vdev->vid,
> -			VIRTIO_RXQ))) {
> -		uint32_t retry;
> +	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
> +						VIRTIO_RXQ, pkts, rx_count);
> 
> -		for (retry = 0; retry < burst_rx_retry_num; retry++) {
> +	/* Retry if necessary */
> +	if (enable_retry && unlikely(enqueue_count < rx_count)) {
> +		uint32_t retry = 0;
> +
> +		while (enqueue_count < rx_count && retry++ <
> burst_rx_retry_num) {
>  			rte_delay_us(burst_rx_delay_time);
> -			if (rx_count <= rte_vhost_avail_entries(vdev->vid,
> -					VIRTIO_RXQ))
> -				break;
> +			enqueue_count += vdev_queue_ops[vdev-
> >vid].enqueue_pkt_burst(vdev,
> +								VIRTIO_RXQ, pkts,
> rx_count);
>  		}
>  	}
> 
> -	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
> -					VIRTIO_RXQ, pkts, rx_count);
> -
>  	if (enable_stats) {
>  		__atomic_add_fetch(&vdev->stats.rx_total_atomic, rx_count,
>  				__ATOMIC_SEQ_CST);
> --
> 2.25.1

Re: [PATCH v2] examples/vhost: fix retry logic on eth rx path

[David Marchand]

On Mon, Jun 20, 2022 at 5:20 AM Xia, Chenbo <chenbo.xia@intel.com> wrote:
> > drain_eth_rx() uses rte_vhost_avail_entries() to calculate
> > the available entries to determine if a retry is required.
> > However, this function only works with split rings, and
> > calculating packed rings will return the wrong value and cause
> > unnecessary retries resulting in a significant performance penalty.
> >
> > This patch fix that by using the difference between tx/rx burst
> > as the retry condition.
>
> Does it mean we don't need the API rte_vhost_avail_entries() anymore?
>
> Jiayu/Yuan/Maxime, what do you think?

FWIW, I still see a user:
virtio-forwarder/virtio_vhostuser.c:     * This check ensures that we
do not call rte_vhost_avail_entries
virtio-forwarder/virtio_worker.c:        try_rcv =
rte_vhost_avail_entries((int)relay->vio.vio_dev,

Cc'd a few Corigine guys.


-- 
David Marchand

RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path

[Xia, Chenbo]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, June 20, 2022 3:36 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; maxime.coquelin@redhat.com
> Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>; jin.liu@corigine.com;
> louis.peens@corigine.com; peng.zhang@corigine.com; Heinrich Kuhn
> <heinrich.kuhn@corigine.com>
> Subject: Re: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> 
> On Mon, Jun 20, 2022 at 5:20 AM Xia, Chenbo <chenbo.xia@intel.com> wrote:
> > > drain_eth_rx() uses rte_vhost_avail_entries() to calculate
> > > the available entries to determine if a retry is required.
> > > However, this function only works with split rings, and
> > > calculating packed rings will return the wrong value and cause
> > > unnecessary retries resulting in a significant performance penalty.
> > >
> > > This patch fix that by using the difference between tx/rx burst
> > > as the retry condition.
> >
> > Does it mean we don't need the API rte_vhost_avail_entries() anymore?
> >
> > Jiayu/Yuan/Maxime, what do you think?
> 
> FWIW, I still see a user:
> virtio-forwarder/virtio_vhostuser.c:     * This check ensures that we
> do not call rte_vhost_avail_entries
> virtio-forwarder/virtio_worker.c:        try_rcv =
> rte_vhost_avail_entries((int)relay->vio.vio_dev,
> 
> Cc'd a few Corigine guys.

Thanks David for this info! Then I guess only split ring is used in this use case?
If we want to keep it, then this API should also be fixed as it's not supporting
packed ring.

Thanks,
Chenbo

> 
> 
> --
> David Marchand

RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path

[Hu, Jiayu]

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Monday, June 20, 2022 3:49 PM
> To: David Marchand <david.marchand@redhat.com>;
> maxime.coquelin@redhat.com
> Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>; jin.liu@corigine.com;
> louis.peens@corigine.com; peng.zhang@corigine.com; Heinrich Kuhn
> <heinrich.kuhn@corigine.com>
> Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> 
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Monday, June 20, 2022 3:36 PM
> > To: Xia, Chenbo <chenbo.xia@intel.com>; maxime.coquelin@redhat.com
> > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> > <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> > stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>;
> > jin.liu@corigine.com; louis.peens@corigine.com;
> > peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> > Subject: Re: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> >
> > On Mon, Jun 20, 2022 at 5:20 AM Xia, Chenbo <chenbo.xia@intel.com>
> wrote:
> > > > drain_eth_rx() uses rte_vhost_avail_entries() to calculate the
> > > > available entries to determine if a retry is required.
> > > > However, this function only works with split rings, and
> > > > calculating packed rings will return the wrong value and cause
> > > > unnecessary retries resulting in a significant performance penalty.
> > > >
> > > > This patch fix that by using the difference between tx/rx burst as
> > > > the retry condition.
> > >
> > > Does it mean we don't need the API rte_vhost_avail_entries() anymore?
> > >
> > > Jiayu/Yuan/Maxime, what do you think?
> >
> > FWIW, I still see a user:
> > virtio-forwarder/virtio_vhostuser.c:     * This check ensures that we
> > do not call rte_vhost_avail_entries
> > virtio-forwarder/virtio_worker.c:        try_rcv =
> > rte_vhost_avail_entries((int)relay->vio.vio_dev,
> >
> > Cc'd a few Corigine guys.
> 
> Thanks David for this info! Then I guess only split ring is used in this use case?
> If we want to keep it, then this API should also be fixed as it's not supporting
> packed ring.

Same issue for rte_vhost_rx_queue_count(), and it is used in OVS.

But if look into the implementation of rte_vhost_avail_entries(), it calculates
the number of available descriptors by " vq->avail->idx - vq->last_used_idx".
This logic looks strange. Anyone knows the reason of this implementation?

Thanks,
Jiayu

> 
> Thanks,
> Chenbo
> 
> >
> >
> > --
> > David Marchand
> 

RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path

[Xia, Chenbo]

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: Monday, June 20, 2022 4:59 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; David Marchand
> <david.marchand@redhat.com>; maxime.coquelin@redhat.com
> Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; He, Xingguang
> <xingguang.he@intel.com>; stable@dpdk.org; Ling, WeiX
> <weix.ling@intel.com>; jin.liu@corigine.com; louis.peens@corigine.com;
> peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> 
> 
> 
> > -----Original Message-----
> > From: Xia, Chenbo <chenbo.xia@intel.com>
> > Sent: Monday, June 20, 2022 3:49 PM
> > To: David Marchand <david.marchand@redhat.com>;
> > maxime.coquelin@redhat.com
> > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> > <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> > stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>; jin.liu@corigine.com;
> > louis.peens@corigine.com; peng.zhang@corigine.com; Heinrich Kuhn
> > <heinrich.kuhn@corigine.com>
> > Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com>
> > > Sent: Monday, June 20, 2022 3:36 PM
> > > To: Xia, Chenbo <chenbo.xia@intel.com>; maxime.coquelin@redhat.com
> > > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> > > <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> > > stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>;
> > > jin.liu@corigine.com; louis.peens@corigine.com;
> > > peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> > > Subject: Re: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> > >
> > > On Mon, Jun 20, 2022 at 5:20 AM Xia, Chenbo <chenbo.xia@intel.com>
> > wrote:
> > > > > drain_eth_rx() uses rte_vhost_avail_entries() to calculate the
> > > > > available entries to determine if a retry is required.
> > > > > However, this function only works with split rings, and
> > > > > calculating packed rings will return the wrong value and cause
> > > > > unnecessary retries resulting in a significant performance penalty.
> > > > >
> > > > > This patch fix that by using the difference between tx/rx burst as
> > > > > the retry condition.
> > > >
> > > > Does it mean we don't need the API rte_vhost_avail_entries() anymore?
> > > >
> > > > Jiayu/Yuan/Maxime, what do you think?
> > >
> > > FWIW, I still see a user:
> > > virtio-forwarder/virtio_vhostuser.c:     * This check ensures that we
> > > do not call rte_vhost_avail_entries
> > > virtio-forwarder/virtio_worker.c:        try_rcv =
> > > rte_vhost_avail_entries((int)relay->vio.vio_dev,
> > >
> > > Cc'd a few Corigine guys.
> >
> > Thanks David for this info! Then I guess only split ring is used in this
> use case?
> > If we want to keep it, then this API should also be fixed as it's not
> supporting
> > packed ring.
> 
> Same issue for rte_vhost_rx_queue_count(), and it is used in OVS.
> 
> But if look into the implementation of rte_vhost_avail_entries(), it
> calculates
> the number of available descriptors by " vq->avail->idx - vq-
> >last_used_idx".
> This logic looks strange. Anyone knows the reason of this implementation?

I was not in the history, but as I checked the git log. Seems it's because in this commit,
this API was not improved (This API is introduced before the commit).

commit f6be82d7259ee35683721092d61283d99a47aff1
Author: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Date:   Sun Oct 9 15:27:56 2016 +0800

    vhost: introduce last available index for dequeue

    So far, we retrieve both the used ring and avail ring idx by the var
    last_used_idx; it won't be a problem because the used ring is updated
    immediately after those avail entries are consumed.

    But that's not true when dequeue zero copy is enabled, that used ring is
    updated only when the mbuf is consumed. Thus, we need use another var to
    note the last avail ring idx we have consumed.

    Therefore, last_avail_idx is introduced.

    Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
    Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
    Tested-by: Qian Xu <qian.q.xu@intel.com>

Thanks,
Chenbo

> 
> Thanks,
> Jiayu
> 
> >
> > Thanks,
> > Chenbo
> >
> > >
> > >
> > > --
> > > David Marchand
> >
> 

RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path

[Wang, YuanX]

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Monday, June 20, 2022 5:10 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; David Marchand
> <david.marchand@redhat.com>; maxime.coquelin@redhat.com
> Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; He, Xingguang
> <xingguang.he@intel.com>; stable@dpdk.org; Ling, WeiX
> <weix.ling@intel.com>; jin.liu@corigine.com; louis.peens@corigine.com;
> peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> 
> > -----Original Message-----
> > From: Hu, Jiayu <jiayu.hu@intel.com>
> > Sent: Monday, June 20, 2022 4:59 PM
> > To: Xia, Chenbo <chenbo.xia@intel.com>; David Marchand
> > <david.marchand@redhat.com>; maxime.coquelin@redhat.com
> > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; He, Xingguang
> > <xingguang.he@intel.com>; stable@dpdk.org; Ling, WeiX
> > <weix.ling@intel.com>; jin.liu@corigine.com; louis.peens@corigine.com;
> > peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> > Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> >
> >
> >
> > > -----Original Message-----
> > > From: Xia, Chenbo <chenbo.xia@intel.com>
> > > Sent: Monday, June 20, 2022 3:49 PM
> > > To: David Marchand <david.marchand@redhat.com>;
> > > maxime.coquelin@redhat.com
> > > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> > > <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> > > stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>;
> > > jin.liu@corigine.com; louis.peens@corigine.com;
> > > peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> > > Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx
> > > path
> > >
> > > > -----Original Message-----
> > > > From: David Marchand <david.marchand@redhat.com>
> > > > Sent: Monday, June 20, 2022 3:36 PM
> > > > To: Xia, Chenbo <chenbo.xia@intel.com>;
> maxime.coquelin@redhat.com
> > > > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> > > > <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> > > > stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>;
> > > > jin.liu@corigine.com; louis.peens@corigine.com;
> > > > peng.zhang@corigine.com; Heinrich Kuhn
> > > > <heinrich.kuhn@corigine.com>
> > > > Subject: Re: [PATCH v2] examples/vhost: fix retry logic on eth rx
> > > > path
> > > >
> > > > On Mon, Jun 20, 2022 at 5:20 AM Xia, Chenbo <chenbo.xia@intel.com>
> > > wrote:
> > > > > > drain_eth_rx() uses rte_vhost_avail_entries() to calculate the
> > > > > > available entries to determine if a retry is required.
> > > > > > However, this function only works with split rings, and
> > > > > > calculating packed rings will return the wrong value and cause
> > > > > > unnecessary retries resulting in a significant performance penalty.
> > > > > >
> > > > > > This patch fix that by using the difference between tx/rx
> > > > > > burst as the retry condition.
> > > > >
> > > > > Does it mean we don't need the API rte_vhost_avail_entries()
> anymore?
> > > > >
> > > > > Jiayu/Yuan/Maxime, what do you think?
> > > >
> > > > FWIW, I still see a user:
> > > > virtio-forwarder/virtio_vhostuser.c:     * This check ensures that we
> > > > do not call rte_vhost_avail_entries
> > > > virtio-forwarder/virtio_worker.c:        try_rcv =
> > > > rte_vhost_avail_entries((int)relay->vio.vio_dev,
> > > >
> > > > Cc'd a few Corigine guys.
> > >
> > > Thanks David for this info! Then I guess only split ring is used in
> > > this
> > use case?
> > > If we want to keep it, then this API should also be fixed as it's
> > > not
> > supporting
> > > packed ring.
> >
> > Same issue for rte_vhost_rx_queue_count(), and it is used in OVS.
> >
> > But if look into the implementation of rte_vhost_avail_entries(), it
> > calculates the number of available descriptors by " vq->avail->idx -
> > vq-
> > >last_used_idx".
> > This logic looks strange. Anyone knows the reason of this implementation?
> 
> I was not in the history, but as I checked the git log. Seems it's because in this
> commit, this API was not improved (This API is introduced before the
> commit).
> 
> commit f6be82d7259ee35683721092d61283d99a47aff1
> Author: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Date:   Sun Oct 9 15:27:56 2016 +0800
> 
>     vhost: introduce last available index for dequeue
> 
>     So far, we retrieve both the used ring and avail ring idx by the var
>     last_used_idx; it won't be a problem because the used ring is updated
>     immediately after those avail entries are consumed.
> 
>     But that's not true when dequeue zero copy is enabled, that used ring is
>     updated only when the mbuf is consumed. Thus, we need use another var
> to
>     note the last avail ring idx we have consumed.
> 
>     Therefore, last_avail_idx is introduced.
> 
>     Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>     Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>     Tested-by: Qian Xu <qian.q.xu@intel.com>
> 

It was introduced by this commit.

commit 7202b0a8240158b317665c20525f81d55f16f602
Author: Huawei Xie <huawei.xie@intel.com>
Date:   Thu Oct 9 02:54:51 2014 +0800

    vhost: get available vring entries

    Signed-off-by: Huawei Xie <huawei.xie@intel.com>
    Acked-by: Changchun Ouyang <changchun.ouyang@intel.com>
    [Thomas: split patch]

Check for the define of VQ, it is obvious for split ring. 

struct vhost_virtqueue {
	struct vring_desc	*desc;			/**< Virtqueue descriptor ring. */
	struct vring_avail	*avail;			/**< Virtqueue available ring. */
	struct vring_used	*used;			/**< Virtqueue used ring. */
	uint32_t		size;			/**< Size of descriptor ring. */
	int			backend;		/**< Backend value to determine if device should started/stopped. */
	uint16_t		vhost_hlen;		/**< Vhost header length (varies depending on RX merge buffers. */
	volatile uint16_t	last_used_idx;		/**< Last index used on the available ring */
	volatile uint16_t	last_used_idx_res;	/**< Used for multiple devices reserving buffers. */
#define VIRTIO_INVALID_EVENTFD		(-1)
#define VIRTIO_UNINITIALIZED_EVENTFD	(-2)
	int			callfd;			/**< Used to notify the guest (trigger interrupt). */
	int			kickfd;			/**< Currently unused as polling mode is enabled. */
	int			enabled;
	uint64_t		log_guest_addr;		/**< Physical address of used ring, for logging */
	uint64_t		reserved[15];		/**< Reserve some spaces for future extension. */
	struct buf_vector	buf_vec[BUF_VECTOR_MAX];	/**< for scatter RX. */
} __rte_cache_aligned;

Thanks,
Yuan

> Thanks,
> Chenbo
> 
> >
> > Thanks,
> > Jiayu
> >
> > >
> > > Thanks,
> > > Chenbo
> > >
> > > >
> > > >
> > > > --
> > > > David Marchand
> > >
> >

RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path

[Xia, Chenbo]

> -----Original Message-----
> From: Wang, YuanX <yuanx.wang@intel.com>
> Sent: Monday, June 20, 2022 5:19 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>;
> David Marchand <david.marchand@redhat.com>; maxime.coquelin@redhat.com
> Cc: dev@dpdk.org; He, Xingguang <xingguang.he@intel.com>; stable@dpdk.org;
> Ling, WeiX <weix.ling@intel.com>; jin.liu@corigine.com;
> louis.peens@corigine.com; peng.zhang@corigine.com; Heinrich Kuhn
> <heinrich.kuhn@corigine.com>
> Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> 
> 
> 
> > -----Original Message-----
> > From: Xia, Chenbo <chenbo.xia@intel.com>
> > Sent: Monday, June 20, 2022 5:10 PM
> > To: Hu, Jiayu <jiayu.hu@intel.com>; David Marchand
> > <david.marchand@redhat.com>; maxime.coquelin@redhat.com
> > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; He, Xingguang
> > <xingguang.he@intel.com>; stable@dpdk.org; Ling, WeiX
> > <weix.ling@intel.com>; jin.liu@corigine.com; louis.peens@corigine.com;
> > peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> > Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> >
> > > -----Original Message-----
> > > From: Hu, Jiayu <jiayu.hu@intel.com>
> > > Sent: Monday, June 20, 2022 4:59 PM
> > > To: Xia, Chenbo <chenbo.xia@intel.com>; David Marchand
> > > <david.marchand@redhat.com>; maxime.coquelin@redhat.com
> > > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; He, Xingguang
> > > <xingguang.he@intel.com>; stable@dpdk.org; Ling, WeiX
> > > <weix.ling@intel.com>; jin.liu@corigine.com; louis.peens@corigine.com;
> > > peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> > > Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Xia, Chenbo <chenbo.xia@intel.com>
> > > > Sent: Monday, June 20, 2022 3:49 PM
> > > > To: David Marchand <david.marchand@redhat.com>;
> > > > maxime.coquelin@redhat.com
> > > > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> > > > <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> > > > stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>;
> > > > jin.liu@corigine.com; louis.peens@corigine.com;
> > > > peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> > > > Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx
> > > > path
> > > >
> > > > > -----Original Message-----
> > > > > From: David Marchand <david.marchand@redhat.com>
> > > > > Sent: Monday, June 20, 2022 3:36 PM
> > > > > To: Xia, Chenbo <chenbo.xia@intel.com>;
> > maxime.coquelin@redhat.com
> > > > > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> > > > > <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> > > > > stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>;
> > > > > jin.liu@corigine.com; louis.peens@corigine.com;
> > > > > peng.zhang@corigine.com; Heinrich Kuhn
> > > > > <heinrich.kuhn@corigine.com>
> > > > > Subject: Re: [PATCH v2] examples/vhost: fix retry logic on eth rx
> > > > > path
> > > > >
> > > > > On Mon, Jun 20, 2022 at 5:20 AM Xia, Chenbo <chenbo.xia@intel.com>
> > > > wrote:
> > > > > > > drain_eth_rx() uses rte_vhost_avail_entries() to calculate the
> > > > > > > available entries to determine if a retry is required.
> > > > > > > However, this function only works with split rings, and
> > > > > > > calculating packed rings will return the wrong value and cause
> > > > > > > unnecessary retries resulting in a significant performance
> penalty.
> > > > > > >
> > > > > > > This patch fix that by using the difference between tx/rx
> > > > > > > burst as the retry condition.
> > > > > >
> > > > > > Does it mean we don't need the API rte_vhost_avail_entries()
> > anymore?
> > > > > >
> > > > > > Jiayu/Yuan/Maxime, what do you think?
> > > > >
> > > > > FWIW, I still see a user:
> > > > > virtio-forwarder/virtio_vhostuser.c:     * This check ensures that
> we
> > > > > do not call rte_vhost_avail_entries
> > > > > virtio-forwarder/virtio_worker.c:        try_rcv =
> > > > > rte_vhost_avail_entries((int)relay->vio.vio_dev,
> > > > >
> > > > > Cc'd a few Corigine guys.
> > > >
> > > > Thanks David for this info! Then I guess only split ring is used in
> > > > this
> > > use case?
> > > > If we want to keep it, then this API should also be fixed as it's
> > > > not
> > > supporting
> > > > packed ring.
> > >
> > > Same issue for rte_vhost_rx_queue_count(), and it is used in OVS.
> > >
> > > But if look into the implementation of rte_vhost_avail_entries(), it
> > > calculates the number of available descriptors by " vq->avail->idx -
> > > vq-
> > > >last_used_idx".
> > > This logic looks strange. Anyone knows the reason of this
> implementation?
> >
> > I was not in the history, but as I checked the git log. Seems it's
> because in this
> > commit, this API was not improved (This API is introduced before the
> > commit).
> >
> > commit f6be82d7259ee35683721092d61283d99a47aff1
> > Author: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > Date:   Sun Oct 9 15:27:56 2016 +0800
> >
> >     vhost: introduce last available index for dequeue
> >
> >     So far, we retrieve both the used ring and avail ring idx by the var
> >     last_used_idx; it won't be a problem because the used ring is
> updated
> >     immediately after those avail entries are consumed.
> >
> >     But that's not true when dequeue zero copy is enabled, that used
> ring is
> >     updated only when the mbuf is consumed. Thus, we need use another
> var
> > to
> >     note the last avail ring idx we have consumed.
> >
> >     Therefore, last_avail_idx is introduced.
> >
> >     Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >     Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >     Tested-by: Qian Xu <qian.q.xu@intel.com>
> >
> 
> It was introduced by this commit.

Yes, of course both last_XXX_idx are introduced for split ring. I was saying
the story seems to be:

At first, vhost usage is very trivial, get available and set used, so one idx
is enough. Then dequeue_zero_copy comes (later removed) and you may not update
used after you get avail in one func call. Then last_avail_idx is introduced but
rte_vhost_avail_entries() is not updated. And when we introduced packed ring,
this API is also not updated.

Correct me if I misunderstand the story.

Thanks,
Chenbo

> 
> commit 7202b0a8240158b317665c20525f81d55f16f602
> Author: Huawei Xie <huawei.xie@intel.com>
> Date:   Thu Oct 9 02:54:51 2014 +0800
> 
>     vhost: get available vring entries
> 
>     Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>     Acked-by: Changchun Ouyang <changchun.ouyang@intel.com>
>     [Thomas: split patch]
> 
> Check for the define of VQ, it is obvious for split ring.
> 
> struct vhost_virtqueue {
> 	struct vring_desc	*desc;			/**< Virtqueue descriptor
> ring. */
> 	struct vring_avail	*avail;			/**< Virtqueue
> available ring. */
> 	struct vring_used	*used;			/**< Virtqueue used ring.
> */
> 	uint32_t		size;			/**< Size of descriptor ring. */
> 	int			backend;		/**< Backend value to determine
> if device should started/stopped. */
> 	uint16_t		vhost_hlen;		/**< Vhost header length (varies
> depending on RX merge buffers. */
> 	volatile uint16_t	last_used_idx;		/**< Last index used on
> the available ring */
> 	volatile uint16_t	last_used_idx_res;	/**< Used for multiple
> devices reserving buffers. */
> #define VIRTIO_INVALID_EVENTFD		(-1)
> #define VIRTIO_UNINITIALIZED_EVENTFD	(-2)
> 	int			callfd;			/**< Used to notify the
> guest (trigger interrupt). */
> 	int			kickfd;			/**< Currently unused as
> polling mode is enabled. */
> 	int			enabled;
> 	uint64_t		log_guest_addr;		/**< Physical address of
> used ring, for logging */
> 	uint64_t		reserved[15];		/**< Reserve some spaces
> for future extension. */
> 	struct buf_vector	buf_vec[BUF_VECTOR_MAX];	/**< for scatter RX.
> */
> } __rte_cache_aligned;
> 
> Thanks,
> Yuan
> 
> > Thanks,
> > Chenbo
> >
> > >
> > > Thanks,
> > > Jiayu
> > >
> > > >
> > > > Thanks,
> > > > Chenbo
> > > >
> > > > >
> > > > >
> > > > > --
> > > > > David Marchand
> > > >
> > >

RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path

[Hu, Jiayu]

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Monday, June 20, 2022 5:10 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; David Marchand
> <david.marchand@redhat.com>; maxime.coquelin@redhat.com
> Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; He, Xingguang
> <xingguang.he@intel.com>; stable@dpdk.org; Ling, WeiX
> <weix.ling@intel.com>; jin.liu@corigine.com; louis.peens@corigine.com;
> peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> 
> > -----Original Message-----
> > From: Hu, Jiayu <jiayu.hu@intel.com>
> > Sent: Monday, June 20, 2022 4:59 PM
> > To: Xia, Chenbo <chenbo.xia@intel.com>; David Marchand
> > <david.marchand@redhat.com>; maxime.coquelin@redhat.com
> > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; He, Xingguang
> > <xingguang.he@intel.com>; stable@dpdk.org; Ling, WeiX
> > <weix.ling@intel.com>; jin.liu@corigine.com; louis.peens@corigine.com;
> > peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> > Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> >
> >
> >
> > > -----Original Message-----
> > > From: Xia, Chenbo <chenbo.xia@intel.com>
> > > Sent: Monday, June 20, 2022 3:49 PM
> > > To: David Marchand <david.marchand@redhat.com>;
> > > maxime.coquelin@redhat.com
> > > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> > > <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> > > stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>;
> > > jin.liu@corigine.com; louis.peens@corigine.com;
> > > peng.zhang@corigine.com; Heinrich Kuhn <heinrich.kuhn@corigine.com>
> > > Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx
> > > path
> > >
> > > > -----Original Message-----
> > > > From: David Marchand <david.marchand@redhat.com>
> > > > Sent: Monday, June 20, 2022 3:36 PM
> > > > To: Xia, Chenbo <chenbo.xia@intel.com>;
> maxime.coquelin@redhat.com
> > > > Cc: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org; Hu, Jiayu
> > > > <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> > > > stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>;
> > > > jin.liu@corigine.com; louis.peens@corigine.com;
> > > > peng.zhang@corigine.com; Heinrich Kuhn
> > > > <heinrich.kuhn@corigine.com>
> > > > Subject: Re: [PATCH v2] examples/vhost: fix retry logic on eth rx
> > > > path
> > > >
> > > > On Mon, Jun 20, 2022 at 5:20 AM Xia, Chenbo <chenbo.xia@intel.com>
> > > wrote:
> > > > > > drain_eth_rx() uses rte_vhost_avail_entries() to calculate the
> > > > > > available entries to determine if a retry is required.
> > > > > > However, this function only works with split rings, and
> > > > > > calculating packed rings will return the wrong value and cause
> > > > > > unnecessary retries resulting in a significant performance penalty.
> > > > > >
> > > > > > This patch fix that by using the difference between tx/rx
> > > > > > burst as the retry condition.
> > > > >
> > > > > Does it mean we don't need the API rte_vhost_avail_entries()
> anymore?
> > > > >
> > > > > Jiayu/Yuan/Maxime, what do you think?
> > > >
> > > > FWIW, I still see a user:
> > > > virtio-forwarder/virtio_vhostuser.c:     * This check ensures that we
> > > > do not call rte_vhost_avail_entries
> > > > virtio-forwarder/virtio_worker.c:        try_rcv =
> > > > rte_vhost_avail_entries((int)relay->vio.vio_dev,
> > > >
> > > > Cc'd a few Corigine guys.
> > >
> > > Thanks David for this info! Then I guess only split ring is used in
> > > this
> > use case?
> > > If we want to keep it, then this API should also be fixed as it's
> > > not
> > supporting
> > > packed ring.
> >
> > Same issue for rte_vhost_rx_queue_count(), and it is used in OVS.
> >
> > But if look into the implementation of rte_vhost_avail_entries(), it
> > calculates the number of available descriptors by " vq->avail->idx -
> > vq-
> > >last_used_idx".
> > This logic looks strange. Anyone knows the reason of this implementation?
> 
> I was not in the history, but as I checked the git log. Seems it's because in this
> commit, this API was not improved (This API is introduced before the
> commit).

Agree. Need a bug fix for this API too.

Thanks,
Jiayu

> 
> commit f6be82d7259ee35683721092d61283d99a47aff1
> Author: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Date:   Sun Oct 9 15:27:56 2016 +0800
> 
>     vhost: introduce last available index for dequeue
> 
>     So far, we retrieve both the used ring and avail ring idx by the var
>     last_used_idx; it won't be a problem because the used ring is updated
>     immediately after those avail entries are consumed.
> 
>     But that's not true when dequeue zero copy is enabled, that used ring is
>     updated only when the mbuf is consumed. Thus, we need use another var
> to
>     note the last avail ring idx we have consumed.
> 
>     Therefore, last_avail_idx is introduced.
> 
>     Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>     Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>     Tested-by: Qian Xu <qian.q.xu@intel.com>
> 
> Thanks,
> Chenbo
> 
> >
> > Thanks,
> > Jiayu
> >
> > >
> > > Thanks,
> > > Chenbo
> > >
> > > >
> > > >
> > > > --
> > > > David Marchand
> > >
> >
> 

RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path

[Xia, Chenbo]

Hi Yuan,

> -----Original Message-----
> From: Wang, YuanX <yuanx.wang@intel.com>
> Sent: Friday, June 17, 2022 3:02 PM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> dev@dpdk.org
> Cc: Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> Wang, YuanX <yuanx.wang@intel.com>; stable@dpdk.org; Ling, WeiX
> <weix.ling@intel.com>
> Subject: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> 
> drain_eth_rx() uses rte_vhost_avail_entries() to calculate
> the available entries to determine if a retry is required.
> However, this function only works with split rings, and
> calculating packed rings will return the wrong value and cause
> unnecessary retries resulting in a significant performance penalty.
> 
> This patch fix that by using the difference between tx/rx burst
> as the retry condition.
> 
> Fixes: 4ecf22e356de ("vhost: export device id as the interface to
> applications")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> Tested-by: Wei Ling <weix.ling@intel.com>
> ---
> V2: Rebase to 22.07 rc1.
> ---
>  examples/vhost/main.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index e7fee5aa1b..e7a84333ed 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -634,7 +634,7 @@ us_vhost_usage(const char *prgname)
>  {
>  	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p PORTMASK\n"
>  	"		--vm2vm [0|1|2]\n"
> -	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
> +	"		--rx-retry [0|1] --mergeable [0|1] --stats [0-N]\n"
>  	"		--socket-file <path>\n"
>  	"		--nb-devices ND\n"
>  	"		-p PORTMASK: Set mask for ports to be used by
> application\n"
> @@ -1383,27 +1383,20 @@ drain_eth_rx(struct vhost_dev *vdev)
>  	if (!rx_count)
>  		return;
> 
> -	/*
> -	 * When "enable_retry" is set, here we wait and retry when there
> -	 * is no enough free slots in the queue to hold @rx_count packets,
> -	 * to diminish packet loss.
> -	 */
> -	if (enable_retry &&
> -	    unlikely(rx_count > rte_vhost_avail_entries(vdev->vid,
> -			VIRTIO_RXQ))) {
> -		uint32_t retry;
> +	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
> +						VIRTIO_RXQ, pkts, rx_count);
> 
> -		for (retry = 0; retry < burst_rx_retry_num; retry++) {
> +	/* Retry if necessary */
> +	if (enable_retry && unlikely(enqueue_count < rx_count)) {
> +		uint32_t retry = 0;
> +
> +		while (enqueue_count < rx_count && retry++ <
> burst_rx_retry_num) {
>  			rte_delay_us(burst_rx_delay_time);
> -			if (rx_count <= rte_vhost_avail_entries(vdev->vid,
> -					VIRTIO_RXQ))
> -				break;
> +			enqueue_count += vdev_queue_ops[vdev-
> >vid].enqueue_pkt_burst(vdev,
> +								VIRTIO_RXQ, pkts,
> rx_count);

Looking at the code again, it seems current logic will enqueue more than rx_count
packets and same packet may be sent multiple times as you are enqueue multiple times
but using the same mbuf pointer and rx_count.

Thanks,
Chenbo 

>  		}
>  	}
> 
> -	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
> -					VIRTIO_RXQ, pkts, rx_count);
> -
>  	if (enable_stats) {
>  		__atomic_add_fetch(&vdev->stats.rx_total_atomic, rx_count,
>  				__ATOMIC_SEQ_CST);
> --
> 2.25.1

RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path

[Wang, YuanX]

Hi Chenbo,

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Tuesday, June 21, 2022 9:35 PM
> To: Wang, YuanX <yuanx.wang@intel.com>; maxime.coquelin@redhat.com;
> dev@dpdk.org
> Cc: Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> <xingguang.he@intel.com>; stable@dpdk.org; Ling, WeiX
> <weix.ling@intel.com>
> Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> 
> Hi Yuan,
> 
> > -----Original Message-----
> > From: Wang, YuanX <yuanx.wang@intel.com>
> > Sent: Friday, June 17, 2022 3:02 PM
> > To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> > dev@dpdk.org
> > Cc: Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> > <xingguang.he@intel.com>; Wang, YuanX <yuanx.wang@intel.com>;
> > stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>
> > Subject: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> >
> > drain_eth_rx() uses rte_vhost_avail_entries() to calculate the
> > available entries to determine if a retry is required.
> > However, this function only works with split rings, and calculating
> > packed rings will return the wrong value and cause unnecessary retries
> > resulting in a significant performance penalty.
> >
> > This patch fix that by using the difference between tx/rx burst as the
> > retry condition.
> >
> > Fixes: 4ecf22e356de ("vhost: export device id as the interface to
> > applications")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> > Tested-by: Wei Ling <weix.ling@intel.com>
> > ---
> > V2: Rebase to 22.07 rc1.
> > ---
> >  examples/vhost/main.c | 27 ++++++++++-----------------
> >  1 file changed, 10 insertions(+), 17 deletions(-)
> >
> > diff --git a/examples/vhost/main.c b/examples/vhost/main.c index
> > e7fee5aa1b..e7a84333ed 100644
> > --- a/examples/vhost/main.c
> > +++ b/examples/vhost/main.c
> > @@ -634,7 +634,7 @@ us_vhost_usage(const char *prgname)  {
> >  	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p
> PORTMASK\n"
> >  	"		--vm2vm [0|1|2]\n"
> > -	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
> > +	"		--rx-retry [0|1] --mergeable [0|1] --stats [0-N]\n"
> >  	"		--socket-file <path>\n"
> >  	"		--nb-devices ND\n"
> >  	"		-p PORTMASK: Set mask for ports to be used by
> > application\n"
> > @@ -1383,27 +1383,20 @@ drain_eth_rx(struct vhost_dev *vdev)
> >  	if (!rx_count)
> >  		return;
> >
> > -	/*
> > -	 * When "enable_retry" is set, here we wait and retry when there
> > -	 * is no enough free slots in the queue to hold @rx_count packets,
> > -	 * to diminish packet loss.
> > -	 */
> > -	if (enable_retry &&
> > -	    unlikely(rx_count > rte_vhost_avail_entries(vdev->vid,
> > -			VIRTIO_RXQ))) {
> > -		uint32_t retry;
> > +	enqueue_count = vdev_queue_ops[vdev-
> >vid].enqueue_pkt_burst(vdev,
> > +						VIRTIO_RXQ, pkts, rx_count);
> >
> > -		for (retry = 0; retry < burst_rx_retry_num; retry++) {
> > +	/* Retry if necessary */
> > +	if (enable_retry && unlikely(enqueue_count < rx_count)) {
> > +		uint32_t retry = 0;
> > +
> > +		while (enqueue_count < rx_count && retry++ <
> > burst_rx_retry_num) {
> >  			rte_delay_us(burst_rx_delay_time);
> > -			if (rx_count <= rte_vhost_avail_entries(vdev->vid,
> > -					VIRTIO_RXQ))
> > -				break;
> > +			enqueue_count += vdev_queue_ops[vdev-
> > >vid].enqueue_pkt_burst(vdev,
> > +								VIRTIO_RXQ,
> pkts,
> > rx_count);
> 
> Looking at the code again, it seems current logic will enqueue more than
> rx_count packets and same packet may be sent multiple times as you are
> enqueue multiple times but using the same mbuf pointer and rx_count.
> 

Thanks for pointing that out, I did miss the packet index.
Will fix in the next version.

Thanks,
Yuan

> Thanks,
> Chenbo
> 
> >  		}
> >  	}
> >
> > -	enqueue_count = vdev_queue_ops[vdev-
> >vid].enqueue_pkt_burst(vdev,
> > -					VIRTIO_RXQ, pkts, rx_count);
> > -
> >  	if (enable_stats) {
> >  		__atomic_add_fetch(&vdev->stats.rx_total_atomic,
> rx_count,
> >  				__ATOMIC_SEQ_CST);
> > --
> > 2.25.1

[PATCH v3] examples/vhost: fix retry logic on eth rx path

[Yuan Wang]

drain_eth_rx() uses rte_vhost_avail_entries() to calculate
the available entries to determine if a retry is required.
However, this function only works with split rings, and
calculating packed rings will return the wrong value and cause
unnecessary retries resulting in a significant performance penalty.

This patch fix that by using the difference between tx/rx burst
as the retry condition.

Fixes: 4ecf22e356de ("vhost: export device id as the interface to applications")
Cc: stable@dpdk.org

Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
Tested-by: Wei Ling <weix.ling@intel.com>
---
V3: Fix mbuf index.
V2: Rebase to 22.07 rc1.
---
 examples/vhost/main.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index e7fee5aa1b..0fa6c096c8 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -634,7 +634,7 @@ us_vhost_usage(const char *prgname)
 {
 	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p PORTMASK\n"
 	"		--vm2vm [0|1|2]\n"
-	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
+	"		--rx-retry [0|1] --mergeable [0|1] --stats [0-N]\n"
 	"		--socket-file <path>\n"
 	"		--nb-devices ND\n"
 	"		-p PORTMASK: Set mask for ports to be used by application\n"
@@ -1383,27 +1383,21 @@ drain_eth_rx(struct vhost_dev *vdev)
 	if (!rx_count)
 		return;
 
-	/*
-	 * When "enable_retry" is set, here we wait and retry when there
-	 * is no enough free slots in the queue to hold @rx_count packets,
-	 * to diminish packet loss.
-	 */
-	if (enable_retry &&
-	    unlikely(rx_count > rte_vhost_avail_entries(vdev->vid,
-			VIRTIO_RXQ))) {
-		uint32_t retry;
+	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
+						VIRTIO_RXQ, pkts, rx_count);
 
-		for (retry = 0; retry < burst_rx_retry_num; retry++) {
+	/* Retry if necessary */
+	if (enable_retry && unlikely(enqueue_count < rx_count)) {
+		uint32_t retry = 0;
+
+		while (enqueue_count < rx_count && retry++ < burst_rx_retry_num) {
 			rte_delay_us(burst_rx_delay_time);
-			if (rx_count <= rte_vhost_avail_entries(vdev->vid,
-					VIRTIO_RXQ))
-				break;
+			enqueue_count += vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
+							VIRTIO_RXQ, &pkts[enqueue_count],
+							rx_count - enqueue_count);
 		}
 	}
 
-	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
-					VIRTIO_RXQ, pkts, rx_count);
-
 	if (enable_stats) {
 		__atomic_add_fetch(&vdev->stats.rx_total_atomic, rx_count,
 				__ATOMIC_SEQ_CST);
-- 
2.25.1

Re: [PATCH v3] examples/vhost: fix retry logic on eth rx path

[Maxime Coquelin]

On 6/22/22 08:27, Yuan Wang wrote:
> drain_eth_rx() uses rte_vhost_avail_entries() to calculate
> the available entries to determine if a retry is required.
> However, this function only works with split rings, and
> calculating packed rings will return the wrong value and cause
> unnecessary retries resulting in a significant performance penalty.
> 
> This patch fix that by using the difference between tx/rx burst
> as the retry condition.
> 
> Fixes: 4ecf22e356de ("vhost: export device id as the interface to applications")

Are you sure about the Fixes tag?
It seems to be unrelated to the issue this patch is addressing.

> Cc: stable@dpdk.org
> 
> Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> Tested-by: Wei Ling <weix.ling@intel.com>
> ---
> V3: Fix mbuf index.
> V2: Rebase to 22.07 rc1.
> ---
>   examples/vhost/main.c | 28 +++++++++++-----------------
>   1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index e7fee5aa1b..0fa6c096c8 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -634,7 +634,7 @@ us_vhost_usage(const char *prgname)
>   {
>   	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p PORTMASK\n"
>   	"		--vm2vm [0|1|2]\n"
> -	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
> +	"		--rx-retry [0|1] --mergeable [0|1] --stats [0-N]\n"
>   	"		--socket-file <path>\n"
>   	"		--nb-devices ND\n"
>   	"		-p PORTMASK: Set mask for ports to be used by application\n"
> @@ -1383,27 +1383,21 @@ drain_eth_rx(struct vhost_dev *vdev)
>   	if (!rx_count)
>   		return;
>   
> -	/*
> -	 * When "enable_retry" is set, here we wait and retry when there
> -	 * is no enough free slots in the queue to hold @rx_count packets,
> -	 * to diminish packet loss.
> -	 */
> -	if (enable_retry &&
> -	    unlikely(rx_count > rte_vhost_avail_entries(vdev->vid,
> -			VIRTIO_RXQ))) {
> -		uint32_t retry;
> +	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
> +						VIRTIO_RXQ, pkts, rx_count);
>   
> -		for (retry = 0; retry < burst_rx_retry_num; retry++) {
> +	/* Retry if necessary */
> +	if (enable_retry && unlikely(enqueue_count < rx_count)) {
> +		uint32_t retry = 0;
> +
> +		while (enqueue_count < rx_count && retry++ < burst_rx_retry_num) {
>   			rte_delay_us(burst_rx_delay_time);
> -			if (rx_count <= rte_vhost_avail_entries(vdev->vid,
> -					VIRTIO_RXQ))
> -				break;
> +			enqueue_count += vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
> +							VIRTIO_RXQ, &pkts[enqueue_count],
> +							rx_count - enqueue_count);
>   		}
>   	}
>   
> -	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
> -					VIRTIO_RXQ, pkts, rx_count);
> -
>   	if (enable_stats) {
>   		__atomic_add_fetch(&vdev->stats.rx_total_atomic, rx_count,
>   				__ATOMIC_SEQ_CST);

RE: [PATCH v3] examples/vhost: fix retry logic on eth rx path

[Wang, YuanX]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, June 22, 2022 3:24 PM
> To: Wang, YuanX <yuanx.wang@intel.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> <xingguang.he@intel.com>; stable@dpdk.org; Ling, WeiX
> <weix.ling@intel.com>
> Subject: Re: [PATCH v3] examples/vhost: fix retry logic on eth rx path
> 
> 
> 
> On 6/22/22 08:27, Yuan Wang wrote:
> > drain_eth_rx() uses rte_vhost_avail_entries() to calculate the
> > available entries to determine if a retry is required.
> > However, this function only works with split rings, and calculating
> > packed rings will return the wrong value and cause unnecessary retries
> > resulting in a significant performance penalty.
> >
> > This patch fix that by using the difference between tx/rx burst as the
> > retry condition.
> >
> > Fixes: 4ecf22e356de ("vhost: export device id as the interface to
> > applications")
> 
> Are you sure about the Fixes tag?
> It seems to be unrelated to the issue this patch is addressing.

Yes, it looks like the wrong tag, Thanks for pointing out.
Will fix at next version.

Thanks,
Yuan

> 
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> > Tested-by: Wei Ling <weix.ling@intel.com>
> > ---
> > V3: Fix mbuf index.
> > V2: Rebase to 22.07 rc1.
> > ---
> >   examples/vhost/main.c | 28 +++++++++++-----------------
> >   1 file changed, 11 insertions(+), 17 deletions(-)
> >
> > diff --git a/examples/vhost/main.c b/examples/vhost/main.c index
> > e7fee5aa1b..0fa6c096c8 100644
> > --- a/examples/vhost/main.c
> > +++ b/examples/vhost/main.c
> > @@ -634,7 +634,7 @@ us_vhost_usage(const char *prgname)
> >   {
> >   	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p
> PORTMASK\n"
> >   	"		--vm2vm [0|1|2]\n"
> > -	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
> > +	"		--rx-retry [0|1] --mergeable [0|1] --stats [0-N]\n"
> >   	"		--socket-file <path>\n"
> >   	"		--nb-devices ND\n"
> >   	"		-p PORTMASK: Set mask for ports to be used by
> application\n"
> > @@ -1383,27 +1383,21 @@ drain_eth_rx(struct vhost_dev *vdev)
> >   	if (!rx_count)
> >   		return;
> >
> > -	/*
> > -	 * When "enable_retry" is set, here we wait and retry when there
> > -	 * is no enough free slots in the queue to hold @rx_count packets,
> > -	 * to diminish packet loss.
> > -	 */
> > -	if (enable_retry &&
> > -	    unlikely(rx_count > rte_vhost_avail_entries(vdev->vid,
> > -			VIRTIO_RXQ))) {
> > -		uint32_t retry;
> > +	enqueue_count = vdev_queue_ops[vdev-
> >vid].enqueue_pkt_burst(vdev,
> > +						VIRTIO_RXQ, pkts, rx_count);
> >
> > -		for (retry = 0; retry < burst_rx_retry_num; retry++) {
> > +	/* Retry if necessary */
> > +	if (enable_retry && unlikely(enqueue_count < rx_count)) {
> > +		uint32_t retry = 0;
> > +
> > +		while (enqueue_count < rx_count && retry++ <
> burst_rx_retry_num) {
> >   			rte_delay_us(burst_rx_delay_time);
> > -			if (rx_count <= rte_vhost_avail_entries(vdev->vid,
> > -					VIRTIO_RXQ))
> > -				break;
> > +			enqueue_count += vdev_queue_ops[vdev-
> >vid].enqueue_pkt_burst(vdev,
> > +							VIRTIO_RXQ,
> &pkts[enqueue_count],
> > +							rx_count -
> enqueue_count);
> >   		}
> >   	}
> >
> > -	enqueue_count = vdev_queue_ops[vdev-
> >vid].enqueue_pkt_burst(vdev,
> > -					VIRTIO_RXQ, pkts, rx_count);
> > -
> >   	if (enable_stats) {
> >   		__atomic_add_fetch(&vdev->stats.rx_total_atomic,
> rx_count,
> >   				__ATOMIC_SEQ_CST);

[PATCH v4] examples/vhost: fix retry logic on eth rx path

[Yuan Wang]

drain_eth_rx() uses rte_vhost_avail_entries() to calculate
the available entries to determine if a retry is required.
However, this function only works with split rings, and
calculating packed rings will return the wrong value and cause
unnecessary retries resulting in a significant performance penalty.

This patch fix that by using the difference between tx/rx burst
as the retry condition.

Fixes: be800696c26e ("examples/vhost: use burst enqueue and dequeue from lib")
Cc: stable@dpdk.org

Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
Tested-by: Wei Ling <weix.ling@intel.com>
---
V4: Fix fiexs tag.
V3: Fix mbuf index.
V2: Rebase to 22.07 rc1.
---
 examples/vhost/main.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index e7fee5aa1b..0fa6c096c8 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -634,7 +634,7 @@ us_vhost_usage(const char *prgname)
 {
 	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p PORTMASK\n"
 	"		--vm2vm [0|1|2]\n"
-	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
+	"		--rx-retry [0|1] --mergeable [0|1] --stats [0-N]\n"
 	"		--socket-file <path>\n"
 	"		--nb-devices ND\n"
 	"		-p PORTMASK: Set mask for ports to be used by application\n"
@@ -1383,27 +1383,21 @@ drain_eth_rx(struct vhost_dev *vdev)
 	if (!rx_count)
 		return;
 
-	/*
-	 * When "enable_retry" is set, here we wait and retry when there
-	 * is no enough free slots in the queue to hold @rx_count packets,
-	 * to diminish packet loss.
-	 */
-	if (enable_retry &&
-	    unlikely(rx_count > rte_vhost_avail_entries(vdev->vid,
-			VIRTIO_RXQ))) {
-		uint32_t retry;
+	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
+						VIRTIO_RXQ, pkts, rx_count);
 
-		for (retry = 0; retry < burst_rx_retry_num; retry++) {
+	/* Retry if necessary */
+	if (enable_retry && unlikely(enqueue_count < rx_count)) {
+		uint32_t retry = 0;
+
+		while (enqueue_count < rx_count && retry++ < burst_rx_retry_num) {
 			rte_delay_us(burst_rx_delay_time);
-			if (rx_count <= rte_vhost_avail_entries(vdev->vid,
-					VIRTIO_RXQ))
-				break;
+			enqueue_count += vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
+							VIRTIO_RXQ, &pkts[enqueue_count],
+							rx_count - enqueue_count);
 		}
 	}
 
-	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
-					VIRTIO_RXQ, pkts, rx_count);
-
 	if (enable_stats) {
 		__atomic_add_fetch(&vdev->stats.rx_total_atomic, rx_count,
 				__ATOMIC_SEQ_CST);
-- 
2.25.1

RE: [PATCH v4] examples/vhost: fix retry logic on eth rx path

[Xia, Chenbo]

> -----Original Message-----
> From: Wang, YuanX <yuanx.wang@intel.com>
> Sent: Wednesday, June 22, 2022 5:26 PM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> dev@dpdk.org
> Cc: Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang <xingguang.he@intel.com>;
> Jiang, Cheng1 <cheng1.jiang@intel.com>; Wang, YuanX <yuanx.wang@intel.com>;
> stable@dpdk.org; Ling, WeiX <weix.ling@intel.com>
> Subject: [PATCH v4] examples/vhost: fix retry logic on eth rx path
> 
> drain_eth_rx() uses rte_vhost_avail_entries() to calculate
> the available entries to determine if a retry is required.
> However, this function only works with split rings, and
> calculating packed rings will return the wrong value and cause
> unnecessary retries resulting in a significant performance penalty.
> 
> This patch fix that by using the difference between tx/rx burst
> as the retry condition.
> 
> Fixes: be800696c26e ("examples/vhost: use burst enqueue and dequeue from
> lib")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> Tested-by: Wei Ling <weix.ling@intel.com>

Yuan & Wei

Please make sure it's re-tested. Patch seems good to me.

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

> ---
> V4: Fix fiexs tag.
> V3: Fix mbuf index.
> V2: Rebase to 22.07 rc1.
> ---
>  examples/vhost/main.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index e7fee5aa1b..0fa6c096c8 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -634,7 +634,7 @@ us_vhost_usage(const char *prgname)
>  {
>  	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p PORTMASK\n"
>  	"		--vm2vm [0|1|2]\n"
> -	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
> +	"		--rx-retry [0|1] --mergeable [0|1] --stats [0-N]\n"
>  	"		--socket-file <path>\n"
>  	"		--nb-devices ND\n"
>  	"		-p PORTMASK: Set mask for ports to be used by
> application\n"
> @@ -1383,27 +1383,21 @@ drain_eth_rx(struct vhost_dev *vdev)
>  	if (!rx_count)
>  		return;
> 
> -	/*
> -	 * When "enable_retry" is set, here we wait and retry when there
> -	 * is no enough free slots in the queue to hold @rx_count packets,
> -	 * to diminish packet loss.
> -	 */
> -	if (enable_retry &&
> -	    unlikely(rx_count > rte_vhost_avail_entries(vdev->vid,
> -			VIRTIO_RXQ))) {
> -		uint32_t retry;
> +	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
> +						VIRTIO_RXQ, pkts, rx_count);
> 
> -		for (retry = 0; retry < burst_rx_retry_num; retry++) {
> +	/* Retry if necessary */
> +	if (enable_retry && unlikely(enqueue_count < rx_count)) {
> +		uint32_t retry = 0;
> +
> +		while (enqueue_count < rx_count && retry++ <
> burst_rx_retry_num) {
>  			rte_delay_us(burst_rx_delay_time);
> -			if (rx_count <= rte_vhost_avail_entries(vdev->vid,
> -					VIRTIO_RXQ))
> -				break;
> +			enqueue_count += vdev_queue_ops[vdev-
> >vid].enqueue_pkt_burst(vdev,
> +							VIRTIO_RXQ, &pkts[enqueue_count],
> +							rx_count - enqueue_count);
>  		}
>  	}
> 
> -	enqueue_count = vdev_queue_ops[vdev->vid].enqueue_pkt_burst(vdev,
> -					VIRTIO_RXQ, pkts, rx_count);
> -
>  	if (enable_stats) {
>  		__atomic_add_fetch(&vdev->stats.rx_total_atomic, rx_count,
>  				__ATOMIC_SEQ_CST);
> --
> 2.25.1

RE: [PATCH v4] examples/vhost: fix retry logic on eth rx path

[Ling, WeiX]

> -----Original Message-----
> From: Wang, YuanX <yuanx.wang@intel.com>
> Sent: Wednesday, June 22, 2022 5:26 PM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> dev@dpdk.org
> Cc: Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> <xingguang.he@intel.com>; Jiang, Cheng1 <cheng1.jiang@intel.com>; Wang,
> YuanX <yuanx.wang@intel.com>; stable@dpdk.org; Ling, WeiX
> <weix.ling@intel.com>
> Subject: [PATCH v4] examples/vhost: fix retry logic on eth rx path
> 
> drain_eth_rx() uses rte_vhost_avail_entries() to calculate the available
> entries to determine if a retry is required.
> However, this function only works with split rings, and calculating packed
> rings will return the wrong value and cause unnecessary retries resulting in a
> significant performance penalty.
> 
> This patch fix that by using the difference between tx/rx burst as the retry
> condition.
> 
> Fixes: be800696c26e ("examples/vhost: use burst enqueue and dequeue
> from lib")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> Tested-by: Wei Ling <weix.ling@intel.com>
> ---
> V4: Fix fiexs tag.
> V3: Fix mbuf index.
> V2: Rebase to 22.07 rc1.
> ---

Tested-by: Wei Ling <weix.ling@intel.com>

Re: [PATCH v4] examples/vhost: fix retry logic on eth rx path

[Maxime Coquelin]

On 6/22/22 11:25, Yuan Wang wrote:
> drain_eth_rx() uses rte_vhost_avail_entries() to calculate
> the available entries to determine if a retry is required.
> However, this function only works with split rings, and
> calculating packed rings will return the wrong value and cause
> unnecessary retries resulting in a significant performance penalty.
> 
> This patch fix that by using the difference between tx/rx burst
> as the retry condition.
> 
> Fixes: be800696c26e ("examples/vhost: use burst enqueue and dequeue from lib")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> Tested-by: Wei Ling <weix.ling@intel.com>
> ---
> V4: Fix fiexs tag.
> V3: Fix mbuf index.
> V2: Rebase to 22.07 rc1.
> ---
>   examples/vhost/main.c | 28 +++++++++++-----------------
>   1 file changed, 11 insertions(+), 17 deletions(-)


Applied to dpdk-next-virtio/main.

Thanks,
Maxime
		__ATOMIC_SEQ_CST);