Thanks Tiwei.
> -----Original Message-----
> From: Bie, Tiwei
> Sent: Thursday, August 1, 2019 2:38 PM
> To: Yu, Jin <jin...@intel.com>
> Cc: dev@dpdk.org; Liu, Changpeng <changpeng....@intel.com>;
> maxime.coque...@redhat.com; Wang, Zhihong <zhihong.w...@intel.com>;
> Lin Li <lili...@baidu.com>; Xun Ni <ni...@baidu.com>; Yu Zhang
> <zhangy...@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 <lili...@baidu.com>
> > Signed-off-by: Xun Ni <ni...@baidu.com>
> > Signed-off-by: Yu Zhang <zhangy...@baidu.com>
> > Signed-off-by: Jin Yu <jin...@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...@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
