Re: [dpdk-dev] [PATCH 1/1] lib/vhost: support async dequeue for split ring

["Hu, Jiayu" <jiayu.hu@intel.com>]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <mcoqueli@redhat.com>
> Sent: Tuesday, June 8, 2021 12:17 AM
> To: Wang, YuanX <yuanx.wang@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> Jiang, Cheng1 <cheng1.jiang@intel.com>; Ma, WenwuX
> <wenwux.ma@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>
> Subject: Re: [PATCH 1/1] lib/vhost: support async dequeue for split ring
> 
> Hi Yuan,
> 
> This is a first review, I will certainly have more comments later.
> 
> On 6/2/21 10:31 AM, Yuan Wang wrote:
> > This patch implements asynchronous dequeue data path for split ring.
> > A new asynchronous dequeue function is introduced. With this function,
> > the application can try to receive packets from the guest with
> > offloading large copies to the DMA engine, thus saving precious CPU
> > cycles.
> 
> Do you have any number to share?

We cannot share the exact numbers without legal approval.
There are some relative values:
in PV cases, testpmd macfwd throughput of one core with async
dequeue enabled is around 1.25x of SW vhost; when enabling
both async enqueue and dequeue, one core macfwd throughput
is up to 2x of SW vhost.

> 
> > Signed-off-by: Wenwu Ma <wenwux.ma@intel.com>
> > Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> >  doc/guides/prog_guide/vhost_lib.rst |  10 +
> >  examples/vhost/ioat.c               |  30 +-
> >  examples/vhost/ioat.h               |   3 +
> >  examples/vhost/main.c               |  60 +--
> >  lib/vhost/rte_vhost_async.h         |  44 ++-
> >  lib/vhost/version.map               |   3 +
> >  lib/vhost/virtio_net.c              | 549 ++++++++++++++++++++++++++++
> >  7 files changed, 664 insertions(+), 35 deletions(-)
> 
> Please split the patch in multple parts.
> At least don't mix example and lib changes in the same patch.
> 
> > diff --git a/doc/guides/prog_guide/vhost_lib.rst
> b/doc/guides/prog_guide/vhost_lib.rst
> > index 6b7206bc1d..785ab0fb34 100644
> > --- a/doc/guides/prog_guide/vhost_lib.rst
> > +++ b/doc/guides/prog_guide/vhost_lib.rst
> > @@ -281,6 +281,16 @@ The following is an overview of some key Vhost
> API functions:
> >    Poll enqueue completion status from async data path. Completed packets
> >    are returned to applications through ``pkts``.
> >
> > +* ``rte_vhost_try_dequeue_burst(vid, queue_id, mbuf_pool, pkts, count,
> nr_inflight)``
> 
> The function should contain async in its name.
> 
> BTW, I think we should also rename below APIs while they are
> experimental to highlight it is async related:
> 
> rte_vhost_submit_enqueue_burst
> rte_vhost_poll_enqueue_completed

Yes, it's better to add "async" in related functions. How about:
rte_vhost_async_submit_enqueue_burst
rte_vhost_async_poll_enqueue_completed
rte_vhost_async_try_dequeue_burst

Any suggestions?

Thanks,
Jiayu