Re: [dpdk-dev] [PATCH v1 08/14] vhost: improve IO vector logic

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

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, October 25, 2021 6:03 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org; Xia, Chenbo
> <chenbo.xia@intel.com>; Wang, YuanX <yuanx.wang@intel.com>; Ma,
> WenwuX <wenwux.ma@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; david.marchand@redhat.com
> Subject: Re: [PATCH v1 08/14] vhost: improve IO vector logic
> 
> Hi Jiayu,
> 
> On 10/25/21 09:22, Hu, Jiayu wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Monday, October 18, 2021 9:02 PM
> >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu
> >> <jiayu.hu@intel.com>; Wang, YuanX <yuanx.wang@intel.com>; Ma,
> WenwuX
> >> <wenwux.ma@intel.com>; Richardson, Bruce
> >> <bruce.richardson@intel.com>; Mcnamara, John
> >> <john.mcnamara@intel.com>; david.marchand@redhat.com
> >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Subject: [PATCH v1 08/14] vhost: improve IO vector logic
> >>
> >> IO vectors and their iterators arrays were part of the async metadata
> >> but not their indexes.
> >>
> >> In order to makes this more consistent, the patch adds the indexes to
> >> the async metadata. Doing that, we can avoid triggering DMA transfer
> >> within the loop as it IO vector index overflow is now prevented in
> >> the
> >> async_mbuf_to_desc() function.
> >>
> >> Note that previous detection mechanism was broken since the overflow
> >> already happened when detected, so OOB memory access would already
> >> have happened.
> >>
> >> With this changes done, virtio_dev_rx_async_submit_split()
> >> and virtio_dev_rx_async_submit_packed() can be further simplified.
> >>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> ---
> >>   lib/vhost/vhost.h      |   2 +
> >>   lib/vhost/virtio_net.c | 291 ++++++++++++++++++-----------------------
> >>   2 files changed, 131 insertions(+), 162 deletions(-)
> >>
> >> diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h index
> >> dae9a1ac2d..812d4c55a5 100644
> >> --- a/lib/vhost/vhost.h
> >> +++ b/lib/vhost/vhost.h
> >> @@ -134,6 +134,8 @@ struct vhost_async {
> >>
> >>   	struct rte_vhost_iov_iter iov_iter[VHOST_MAX_ASYNC_IT];
> >>   	struct rte_vhost_iovec iovec[VHOST_MAX_ASYNC_VEC];
> >> +	uint16_t iter_idx;
> >> +	uint16_t iovec_idx;
> >>
> >>   	/* data transfer status */
> >>   	struct async_inflight_info *pkts_info; diff --git
> >> a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c index
> >> ae7dded979..c80823a8de 100644
> >> --- a/lib/vhost/virtio_net.c
> >> +++ b/lib/vhost/virtio_net.c
> >> @@ -924,33 +924,86 @@ copy_mbuf_to_desc(struct virtio_net *dev,
> >> struct vhost_virtqueue *vq,
> >>   	return error;
> >>   }
> >>
> >> +static __rte_always_inline int
> >> +async_iter_initialize(struct vhost_async *async) {
> >> +	struct rte_vhost_iov_iter *iter;
> >> +
> >> +	if (unlikely(async->iovec_idx >= VHOST_MAX_ASYNC_VEC)) {
> >> +		VHOST_LOG_DATA(ERR, "no more async iovec available\n");
> >> +		return -1;
> >> +	}
> >> +
> >> +	iter = async->iov_iter + async->iter_idx;
> >> +	iter->iov = async->iovec + async->iovec_idx;
> >> +	iter->nr_segs = 0;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static __rte_always_inline int
> >> +async_iter_add_iovec(struct vhost_async *async, void *src, void
> >> +*dst, size_t len) {
> >> +	struct rte_vhost_iov_iter *iter;
> >> +	struct rte_vhost_iovec *iovec;
> >> +
> >> +	if (unlikely(async->iovec_idx >= VHOST_MAX_ASYNC_VEC)) {
> >> +		VHOST_LOG_DATA(ERR, "no more async iovec available\n");
> >> +		return -1;
> >> +	}
> >
> > For large packets, like 64KB in iperf test, async_iter_add_iovec()
> > frequently reports the log above, as we run out of iovecs. I think
> > it's better to change the log from ERR to DEBUG.
> 
> I think it is better to keep it as an error, we want to see it if it happens
> without having the user to enable debug.
> 
> But maybe we can only print it once, not to flood the logs.

OK.

> 
> > In addition, the size of iovec is too small. For burst 32 and 64KB
> > pkts, it's easy to run out of iovecs and we will drop the pkts to
> > enqueue if it happens, which hurts performance. Enlarging the array is
> > a choice to mitigate the issue, but another solution is to reallocate
> > iovec once we run out of it. How do you think?
> 
> I would prefer we enlarge the array, reallocating the array when the issue
> happens sounds like over-engineering to me.
> 
> Any idea what size it should be based on your experiments?

2048 is enough for iperf and 64KB pkts.

Thanks,
Jiayu
> 
> Thanks,
> Maxime
> 
> > Thanks,
> > Jiayu
> >> +
> >> +	iter = async->iov_iter + async->iter_idx;
> >> +	iovec = async->iovec + async->iovec_idx;
> >> +
> >> +	iovec->src_addr = src;
> >> +	iovec->dst_addr = dst;
> >> +	iovec->len = len;
> >> +
> >> +	iter->nr_segs++;
> >> +	async->iovec_idx++;
> >> +
> >> +	return 0;
> >> +}
> >