Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coque...@redhat.com>
> Sent: Monday, October 25, 2021 6:03 PM
> To: Hu, Jiayu <jiayu...@intel.com>; dev@dpdk.org; Xia, Chenbo
> <chenbo....@intel.com>; Wang, YuanX <yuanx.w...@intel.com>; Ma,
> WenwuX <wenwux...@intel.com>; Richardson, Bruce
> <bruce.richard...@intel.com>; Mcnamara, John
> <john.mcnam...@intel.com>; david.march...@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.coque...@redhat.com>
> >> Sent: Monday, October 18, 2021 9:02 PM
> >> To: dev@dpdk.org; Xia, Chenbo <chenbo....@intel.com>; Hu, Jiayu
> >> <jiayu...@intel.com>; Wang, YuanX <yuanx.w...@intel.com>; Ma,
> WenwuX
> >> <wenwux...@intel.com>; Richardson, Bruce
> >> <bruce.richard...@intel.com>; Mcnamara, John
> >> <john.mcnam...@intel.com>; david.march...@redhat.com
> >> Cc: Maxime Coquelin <maxime.coque...@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.coque...@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;
> >> +}
> >
