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

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

Hi,

On 10/12/21 08:05, Hu, Jiayu wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Friday, October 8, 2021 6:00 AM
>> 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>
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [RFC 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 | 296 +++++++++++++++++++----------------------
>>   2 files changed, 136 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..5ce4c14a73 100644
>> --- a/lib/vhost/virtio_net.c
>> +++ b/lib/vhost/virtio_net.c
>> @@ -924,33 +924,91 @@ 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->iter_idx >= VHOST_MAX_ASYNC_IT)) {
>> +		VHOST_LOG_DATA(ERR, "no more async iterators
>> available\n");
>> +		return -1;
>> +	}
> 
> async->iter_idx will not exceed VHOST_MAX_ASYNC_IT, as virtio_dev_rx
> makes sure the number of packets to enqueue is less than or equal to
> MAX_PKT_BURST and it is the same as VHOST_MAX_ASYNC_IT.

Agree, this may not be necessary.

>> +
>> +	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;
>> +	}
>> +
>> +	iter = async->iov_iter + async->iter_idx;
>> +	iovec = async->iovec + async->iovec_idx;
> 
> async->iovec_idx is never gotten increased.

Good catch!

Thanks,
Maxime

> Thanks,
> Jiayu
>