diff mbox series

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

Message ID 20220617070144.710487-1-yuanx.wang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series [v2] examples/vhost: fix retry logic on eth rx path |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Wang, YuanX June 17, 2022, 7:01 a.m. UTC 
  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(-)

Comments

Chenbo Xia June 20, 2022, 3:20 a.m. UTC | #1 
> -----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
David Marchand June 20, 2022, 7:36 a.m. UTC | #2 
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.
Chenbo Xia June 20, 2022, 7:49 a.m. UTC | #3 
> -----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
Hu, Jiayu June 20, 2022, 8:59 a.m. UTC | #4 
> -----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
>
Chenbo Xia June 20, 2022, 9:09 a.m. UTC | #5 
> -----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
> >
>
Wang, YuanX June 20, 2022, 9:19 a.m. UTC | #6 
> -----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
> > >
> >
Chenbo Xia June 20, 2022, 9:33 a.m. UTC | #7 
> -----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
> > > >
> > >
Hu, Jiayu June 20, 2022, 9:42 a.m. UTC | #8 
> -----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
> > >
> >
>
Chenbo Xia June 21, 2022, 1:34 p.m. UTC | #9 
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
Wang, YuanX June 22, 2022, 2:26 a.m. UTC | #10 
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
diff mbox series

Patch

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);