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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 10/26/21 09:07, Hu, Jiayu wrote:
> 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 for the insight, I will change to 2048 in next revision.

Maxime

> 
> 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;
>>>> +}
>>>
>