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

["Yu, Jin" <jin.yu@intel.com>]

Thanks Tiwei.

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Thursday, August 1, 2019 2:38 PM
> To: Yu, Jin <jin.yu@intel.com>
> Cc: dev@dpdk.org; Liu, Changpeng <changpeng.liu@intel.com>;
> maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> Lin Li <lilin24@baidu.com>; Xun Ni <nixun@baidu.com>; Yu Zhang
> <zhangyu31@baidu.com>
> Subject: Re: [PATCH v4 1/2] vhost: support inflight share memory protocol
> feature
> 
> 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:

Sorry. Will fix them in the next version.
> 
> 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.

Got it.
> 
> > +
> > +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

Got it.
> 
> 
> > +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.

Yeah, I have checked the spec again, I miss it. Will fix it in next version.
> 
> 
> > +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().

Ok, I just don't want to change the rte_vhost_get_vring_base() function. I think it's widely used.
> 
> 
> > +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?

It's OK , will do this in the next version.
> 
> Thanks,
> Tiwei