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

[Xia, Chenbo]

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu at intel.com>
> Sent: Monday, June 20, 2022 4:59 PM
> To: Xia, Chenbo <chenbo.xia at intel.com>; David Marchand
> <david.marchand at redhat.com>; maxime.coquelin at redhat.com
> Cc: Wang, YuanX <yuanx.wang at intel.com>; dev at dpdk.org; He, Xingguang
> <xingguang.he at intel.com>; stable at dpdk.org; Ling, WeiX
> <weix.ling at intel.com>; jin.liu at corigine.com; louis.peens at corigine.com;
> peng.zhang at corigine.com; Heinrich Kuhn <heinrich.kuhn at corigine.com>
> Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> 
> 
> 
> > -----Original Message-----
> > From: Xia, Chenbo <chenbo.xia at intel.com>
> > Sent: Monday, June 20, 2022 3:49 PM
> > To: David Marchand <david.marchand at redhat.com>;
> > maxime.coquelin at redhat.com
> > Cc: Wang, YuanX <yuanx.wang at intel.com>; dev at dpdk.org; Hu, Jiayu
> > <jiayu.hu at intel.com>; He, Xingguang <xingguang.he at intel.com>;
> > stable at dpdk.org; Ling, WeiX <weix.ling at intel.com>; jin.liu at corigine.com;
> > louis.peens at corigine.com; peng.zhang at corigine.com; Heinrich Kuhn
> > <heinrich.kuhn at corigine.com>
> > Subject: RE: [PATCH v2] examples/vhost: fix retry logic on eth rx path
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand at redhat.com>
> > > Sent: Monday, June 20, 2022 3:36 PM
> > > To: Xia, Chenbo <chenbo.xia at intel.com>; maxime.coquelin at redhat.com
> > > Cc: Wang, YuanX <yuanx.wang at intel.com>; dev at dpdk.org; Hu, Jiayu
> > > <jiayu.hu at intel.com>; He, Xingguang <xingguang.he at intel.com>;
> > > stable at dpdk.org; Ling, WeiX <weix.ling at intel.com>;
> > > jin.liu at corigine.com; louis.peens at corigine.com;
> > > peng.zhang at corigine.com; Heinrich Kuhn <heinrich.kuhn at 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 at 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 at 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 at linux.intel.com>
    Reviewed-by: Maxime Coquelin <maxime.coquelin at redhat.com>
    Tested-by: Qian Xu <qian.q.xu at intel.com>

Thanks,
Chenbo

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