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

["Xia, Chenbo" <chenbo.xia@intel.com>]

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