Re: [dpdk-dev] [PATCH v4 1/2] vhost: support inflight share memory protocol feature

[Tiwei Bie <tiwei.bie@intel.com>]

On Thu, Aug 01, 2019 at 04:40:49AM +0800, JinYu wrote:
> This patch introduces two new messages VHOST_USER_GET_INFLIGHT_FD
> and VHOST_USER_SET_INFLIGHT_FD to support transferring a shared
> buffer between qemu and backend.
> 
> Firstly, qemu uses VHOST_USER_GET_INFLIGHT_FD to get the
> shared buffer from backend. Then qemu should send it back
> through VHOST_USER_SET_INFLIGHT_FD each time we start vhost-user.
> 
> This shared buffer is used to process inflight I/O when backend
> reconnect.
> 
> Signed-off-by: Lin Li <lilin24@baidu.com>
> Signed-off-by: Xun Ni <nixun@baidu.com>
> Signed-off-by: Yu Zhang <zhangyu31@baidu.com>
> Signed-off-by: Jin Yu <jin.yu@intel.com>
> ---
> v1 - specify the APIs are split-ring only
> v2 - fix APIs and judge split or packed
> v3 - Add rte_vhost_ prefix and fix one issue.
> v4 - add the packed ring support
> ---
>  lib/librte_vhost/rte_vhost.h           | 301 +++++++++++++++++-
>  lib/librte_vhost/rte_vhost_version.map |  12 +
>  lib/librte_vhost/vhost.c               | 399 ++++++++++++++++++++++-
>  lib/librte_vhost/vhost.h               |  54 ++--
>  lib/librte_vhost/vhost_user.c          | 423 ++++++++++++++++++++++++-
>  lib/librte_vhost/vhost_user.h          |  13 +-
>  6 files changed, 1173 insertions(+), 29 deletions(-)

There are some coding style issues reported by
devtools/checkpatches.sh, please get them fixed:

WARNING:LONG_LINE: line over 80 characters
#372: FILE: lib/librte_vhost/rte_vhost.h:952:
+		uint16_t queue_id, bool *avail_wrap_counter, bool *used_wrap_counter);

WARNING:LONG_LINE: line over 80 characters
#622: FILE: lib/librte_vhost/vhost.c:935:
+		vq->inflight_packed->desc[free_head].addr = vq->desc_packed[head].addr;

WARNING:LONG_LINE: line over 80 characters
#623: FILE: lib/librte_vhost/vhost.c:936:
+		vq->inflight_packed->desc[free_head].len = vq->desc_packed[head].len;

WARNING:LONG_LINE: line over 80 characters
#624: FILE: lib/librte_vhost/vhost.c:937:
+		vq->inflight_packed->desc[free_head].flags = vq->desc_packed[head].flags;

WARNING:LONG_LINE: line over 80 characters
#625: FILE: lib/librte_vhost/vhost.c:938:
+		vq->inflight_packed->desc[free_head].id = vq->desc_packed[head].id;

WARNING:LONG_LINE: line over 80 characters
#783: FILE: lib/librte_vhost/vhost.c:1096:
+		vq->inflight_packed->used_idx &= vq->inflight_packed->desc_num - 1;

WARNING:LONG_LINE: line over 80 characters
#803: FILE: lib/librte_vhost/vhost.c:1278:
+	if (dev == NULL || avail_wrap_counter == NULL || used_wrap_counter == NULL)

WARNING:LONG_LINE: line over 80 characters
#826: FILE: lib/librte_vhost/vhost.c:1301:
+	*last_avail_idx = dev->virtqueue[queue_id]->inflight_packed->old_used_idx;

WARNING:LONG_LINE: line over 80 characters
#833: FILE: lib/librte_vhost/vhost.c:1308:
+		uint16_t queue_id, bool *avail_wrap_counter, bool *used_wrap_counter)

WARNING:LONG_LINE: line over 80 characters
#837: FILE: lib/librte_vhost/vhost.c:1312:
+	if (dev == NULL || avail_wrap_counter == NULL || used_wrap_counter == NULL)

WARNING:LONG_LINE: line over 80 characters
#1116: FILE: lib/librte_vhost/vhost_user.c:1248:
+		sizeof(uint64_t) * 1 + sizeof(uint16_t) * 4, INFLIGHT_ALIGNMENT);

WARNING:LONG_LINE: line over 80 characters
#1122: FILE: lib/librte_vhost/vhost_user.c:1254:
+		sizeof(uint64_t) * 1 + sizeof(uint16_t) * 6 + sizeof(uint8_t) * 9,

WARNING:LONG_LINE: line over 80 characters
#1258: FILE: lib/librte_vhost/vhost_user.c:1390:
+			vq->inflight_packed = (struct inflight_info_packed *)addr;

WARNING:LONG_LINE: line over 80 characters
#1293: FILE: lib/librte_vhost/vhost_user.c:1454:
+vhost_check_queue_inflights_split(struct virtio_net *dev, struct vhost_virtqueue *vq)

WARNING:LONG_LINE: line over 80 characters
#1318: FILE: lib/librte_vhost/vhost_user.c:1479:
+		inflight_split->desc[inflight_split->last_inflight_io].inflight = 0;

WARNING:LONG_LINE: line over 80 characters
#1349: FILE: lib/librte_vhost/vhost_user.c:1510:
+				resubmit->resubmit_list[resubmit->resubmit_num].index = i;

WARNING:LONG_LINE: line over 80 characters
#1350: FILE: lib/librte_vhost/vhost_user.c:1511:
+				resubmit->resubmit_list[resubmit->resubmit_num].counter =

WARNING:LONG_LINE: line over 80 characters
#1394: FILE: lib/librte_vhost/vhost_user.c:1555:
+		if (inflight_packed->desc[inflight_packed->old_used_idx].inflight == 0) {

WARNING:LONG_LINE: line over 80 characters
#1395: FILE: lib/librte_vhost/vhost_user.c:1556:
+			inflight_packed->old_used_idx = inflight_packed->used_idx;

ERROR:TRAILING_WHITESPACE: trailing whitespace
#1396: FILE: lib/librte_vhost/vhost_user.c:1557:
+^I^I^Iinflight_packed->old_used_wrap_counter = $

WARNING:LONG_LINE: line over 80 characters
#1398: FILE: lib/librte_vhost/vhost_user.c:1559:
+			inflight_packed->old_free_head = inflight_packed->free_head;

WARNING:LONG_LINE: line over 80 characters
#1400: FILE: lib/librte_vhost/vhost_user.c:1561:
+			inflight_packed->used_idx = inflight_packed->old_used_idx;

WARNING:LONG_LINE: line over 80 characters
#1403: FILE: lib/librte_vhost/vhost_user.c:1564:
+			inflight_packed->free_head = inflight_packed->old_free_head;

WARNING:LONG_LINE: line over 80 characters
#1420: FILE: lib/librte_vhost/vhost_user.c:1581:
+						 sizeof(struct rte_vhost_resubmit_desc));

WARNING:LONG_LINE: line over 80 characters
#1428: FILE: lib/librte_vhost/vhost_user.c:1589:
+				resubmit->resubmit_list[resubmit->resubmit_num].index = i;

WARNING:LONG_LINE: line over 80 characters
#1429: FILE: lib/librte_vhost/vhost_user.c:1590:
+				resubmit->resubmit_list[resubmit->resubmit_num].counter =

WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'JinYu <jin.yu@intel.com>'

total: 1 errors, 26 warnings, 1425 lines checked
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.
__rte_experimental must appear alone on the line immediately preceding the return type of a function.


> +
> +struct vring_packed_desc {
> +	uint64_t addr;
> +	uint32_t len;
> +	uint16_t id;
> +	uint16_t flags;
> +};

You should keep the guard, otherwise there will be build issues
when linux/virtio_ring.h defines above type.

> +
> +struct vring_split_desc {
> +	uint64_t addr;
> +	uint32_t len;
> +	uint16_t flags;
> +	uint16_t next;
> +};

You don't need to redefine the descriptor for split ring,
you can use the one provided by linux/virtio_ring.h


> +int
> +rte_vhost_set_inflight_desc_packed(int vid, uint16_t vring_idx,
> +	uint16_t head, uint16_t last, uint16_t *inflight_entry)
> +{
> +	struct virtio_net *dev;
> +	struct vhost_virtqueue *vq;
> +	uint16_t old_free_head, free_head;
> +
> +	dev = get_device(vid);
> +	if (unlikely(!dev))
> +		return -1;
> +
> +	if (unlikely(!(dev->protocol_features &
> +		(1ULL << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD))))
> +		return 0;
> +
> +	if (unlikely(!vq_is_packed(dev)))
> +		return -1;
> +
> +	if (unlikely(vring_idx >= VHOST_MAX_VRING))
> +		return -1;
> +
> +	vq = dev->virtqueue[vring_idx];
> +	if (unlikely(!vq))
> +		return -1;
> +
> +	if (unlikely(!vq->inflight_split))
> +		return -1;
> +
> +	free_head = vq->inflight_packed->free_head;
> +	old_free_head = vq->inflight_packed->old_free_head;
> +
> +	/* init header descriptor */
> +	vq->inflight_packed->desc[old_free_head].num = 0;
> +	vq->inflight_packed->desc[old_free_head].counter = vq->global_counter++;
> +	vq->inflight_packed->desc[old_free_head].inflight = 1;
> +
> +	while (head != ((last + 1) & (vq->size - 1))) {
> +		vq->inflight_packed->desc[old_free_head].num++;
> +		vq->inflight_packed->desc[free_head].addr = vq->desc_packed[head].addr;
> +		vq->inflight_packed->desc[free_head].len = vq->desc_packed[head].len;
> +		vq->inflight_packed->desc[free_head].flags = vq->desc_packed[head].flags;
> +		vq->inflight_packed->desc[free_head].id = vq->desc_packed[head].id;
> +		vq->inflight_packed->desc[old_free_head].last = free_head;
> +		free_head = vq->inflight_packed->desc[free_head].next;
> +		vq->inflight_packed->free_head = free_head;
> +		head = (head + 1) & (vq->size - 1);

You can't assume the ring size will be a power of two in
packed ring.


> +int
> +rte_vhost_clr_inflight_desc_packed(int vid, uint16_t vring_idx,
> +	uint16_t head)
> +{
> +	struct virtio_net *dev;
> +	struct vhost_virtqueue *vq;
> +
> +	dev = get_device(vid);
> +	if (unlikely(!dev))
> +		return -1;
> +
> +	if (unlikely(!(dev->protocol_features &
> +		(1ULL << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD))))
> +		return 0;
> +
> +	if (unlikely(!vq_is_packed(dev)))
> +		return -1;
> +
> +	if (unlikely(vring_idx >= VHOST_MAX_VRING))
> +		return -1;
> +
> +	vq = dev->virtqueue[vring_idx];
> +	if (unlikely(!vq))
> +		return -1;
> +
> +	if (unlikely(!vq->inflight_packed))
> +		return -1;
> +
> +	rte_compiler_barrier();
> +
> +	vq->inflight_packed->desc[head].inflight = 0;
> +
> +	rte_compiler_barrier();
> +
> +	vq->inflight_packed->old_free_head = vq->inflight_packed->free_head;
> +	vq->inflight_packed->old_used_idx = vq->inflight_packed->used_idx;
> +	vq->inflight_packed->old_used_wrap_counter =
> +	vq->inflight_packed->used_wrap_counter;

The indent of the last line needs to be fixed.


> +int
> +rte_vhost_set_last_inflight_io_packed(int vid, uint16_t vring_idx,
> +	uint16_t head)
> +{
> +	struct virtio_net *dev;
> +	struct vhost_virtqueue *vq;
> +
> +	dev = get_device(vid);
> +	if (unlikely(!dev))
> +		return -1;
> +
> +	if (unlikely(!(dev->protocol_features &
> +		(1ULL << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD))))
> +		return 0;
> +
> +	if (unlikely(!vq_is_packed(dev)))
> +		return -1;
> +
> +	if (unlikely(vring_idx >= VHOST_MAX_VRING))
> +		return -1;
> +
> +	vq = dev->virtqueue[vring_idx];
> +	if (!vq)
> +		return -1;
> +
> +	if (unlikely(!vq->inflight_packed))
> +		return -1;
> +
> +	vq->inflight_packed->desc[vq->inflight_packed->desc[head].last].next =
> +	vq->inflight_packed->free_head;

Ditto.

> +	vq->inflight_packed->free_head = head;
> +	vq->inflight_packed->used_idx += vq->inflight_packed->desc[head].num;
> +	if (vq->inflight_packed->used_idx >= vq->inflight_packed->desc_num) {
> +		vq->inflight_packed->used_idx &= vq->inflight_packed->desc_num - 1;

Can't assume the ring size will be a power of two.

> +		vq->inflight_packed->used_wrap_counter =
> +		!vq->inflight_packed->used_wrap_counter;

The indent needs to be fixed.

> +	}
> +
> +	return 0;
> +}
> +
>  uint16_t
>  rte_vhost_avail_entries(int vid, uint16_t queue_id)
>  {
> @@ -950,6 +1270,61 @@ int rte_vhost_get_vring_base(int vid, uint16_t queue_id,
>  	return 0;
>  }
>  
> +int rte_vhost_get_vring_base_counter(int vid, uint16_t queue_id,
> +		bool *avail_wrap_counter, bool *used_wrap_counter)
> +{
> +	struct virtio_net *dev = get_device(vid);
> +
> +	if (dev == NULL || avail_wrap_counter == NULL || used_wrap_counter == NULL)
> +		return -1;
> +
> +	*avail_wrap_counter = dev->virtqueue[queue_id]->avail_wrap_counter;
> +	*used_wrap_counter = dev->virtqueue[queue_id]->used_wrap_counter;

I think we should report wrap counters in the most significant
bits for packed ring in rte_vhost_get_vring_base().


> +int rte_vhost_set_vring_base_counter(int vid, uint16_t queue_id,
> +		bool avail_wrap_counter, bool used_wrap_counter)
> +{
> +	struct virtio_net *dev = get_device(vid);
> +
> +	if (!dev)
> +		return -1;
> +
> +	dev->virtqueue[queue_id]->avail_wrap_counter = avail_wrap_counter;
> +	dev->virtqueue[queue_id]->used_wrap_counter = used_wrap_counter;

Ditto.

> +
> +	return 0;
> +}
> +

Can you provide some steps in your cover letter for us
to give it a try with your example?

Thanks,
Tiwei