[dpdk-dev] [PATCH v2 0/2] vhost: introduce rte_vhost_vring_call()

[dpdk-dev] [PATCH v2 0/2] vhost: introduce rte_vhost_vring_call()

[Stefan Hajnoczi]

v2:
 * Add internal vhost_vring_call() helper function [Maxime]

These patches eliminate code duplication for vhost_virtqueue->callfd users by
introducing rte_vhost_vring_call() (public API) and vhost_vring_call()
(librte_vhost-internal API).

Stefan Hajnoczi (2):
  vhost: add vhost_vring_call() helper
  vhost: introduce rte_vhost_vring_call()

 lib/librte_vhost/rte_vhost.h           | 15 +++++++++++++++
 lib/librte_vhost/vhost.h               | 12 ++++++++++++
 examples/vhost/virtio_net.c            | 11 ++---------
 examples/vhost_scsi/vhost_scsi.c       |  6 +++---
 lib/librte_vhost/vhost.c               | 21 +++++++++++++++++++++
 lib/librte_vhost/virtio_net.c          | 23 +++--------------------
 lib/librte_vhost/rte_vhost_version.map |  7 +++++++
 7 files changed, 63 insertions(+), 32 deletions(-)

-- 
2.14.3

[dpdk-dev] [PATCH v2 1/2] vhost: add vhost_vring_call() helper

[Stefan Hajnoczi]

Extract the callfd eventfd signal operation so virtio_net.c does not
have to repeat it multiple times.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/vhost.h      | 12 ++++++++++++
 lib/librte_vhost/virtio_net.c | 23 +++--------------------
 2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 04f54cb60..ac81d83bb 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -394,4 +394,16 @@ vhost_iova_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	return __vhost_iova_to_vva(dev, vq, iova, size, perm);
 }
 
+static __rte_always_inline void
+vhost_vring_call(struct vhost_virtqueue *vq)
+{
+	/* Flush used->idx update before we read avail->flags. */
+	rte_mb();
+
+	/* Kick the guest if necessary. */
+	if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
+			&& (vq->callfd >= 0))
+		eventfd_write(vq->callfd, (eventfd_t)1);
+}
+
 #endif /* _VHOST_NET_CDEV_H_ */
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 79d80f7fd..a92a5181d 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -408,13 +408,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 		offsetof(struct vring_used, idx),
 		sizeof(vq->used->idx));
 
-	/* flush used->idx update before we read avail->flags. */
-	rte_mb();
-
-	/* Kick the guest if necessary. */
-	if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
-			&& (vq->callfd >= 0))
-		eventfd_write(vq->callfd, (eventfd_t)1);
+	vhost_vring_call(vq);
 out:
 	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
 		vhost_user_iotlb_rd_unlock(vq);
@@ -701,14 +695,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 
 	if (likely(vq->shadow_used_idx)) {
 		flush_shadow_used_ring(dev, vq);
-
-		/* flush used->idx update before we read avail->flags. */
-		rte_mb();
-
-		/* Kick the guest if necessary. */
-		if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
-				&& (vq->callfd >= 0))
-			eventfd_write(vq->callfd, (eventfd_t)1);
+		vhost_vring_call(vq);
 	}
 
 out:
@@ -1107,11 +1094,7 @@ update_used_idx(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	vq->used->idx += count;
 	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
 			sizeof(vq->used->idx));
-
-	/* Kick guest if required. */
-	if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
-			&& (vq->callfd >= 0))
-		eventfd_write(vq->callfd, (eventfd_t)1);
+	vhost_vring_call(vq);
 }
 
 static __rte_always_inline struct zcopy_mbuf *
-- 
2.14.3

[dpdk-dev] [PATCH v2 2/2] vhost: introduce rte_vhost_vring_call()

[Stefan Hajnoczi]

Users of librte_vhost currently implement the vring call operation
themselves.  Each caller performs the operation slightly differently.

This patch introduces a new librte_vhost API called
rte_vhost_vring_call() that performs the operation so that vhost-user
applications don't have to duplicate it.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/rte_vhost.h           | 15 +++++++++++++++
 examples/vhost/virtio_net.c            | 11 ++---------
 examples/vhost_scsi/vhost_scsi.c       |  6 +++---
 lib/librte_vhost/vhost.c               | 21 +++++++++++++++++++++
 lib/librte_vhost/rte_vhost_version.map |  7 +++++++
 5 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index f65364495..890f8a831 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -86,7 +86,9 @@ struct rte_vhost_vring {
 	struct vring_used	*used;
 	uint64_t		log_guest_addr;
 
+	/** Deprecated, use rte_vhost_vring_call() instead. */
 	int			callfd;
+
 	int			kickfd;
 	uint16_t		size;
 };
@@ -436,6 +438,19 @@ int rte_vhost_get_mem_table(int vid, struct rte_vhost_memory **mem);
 int rte_vhost_get_vhost_vring(int vid, uint16_t vring_idx,
 			      struct rte_vhost_vring *vring);
 
+/**
+ * Notify the guest that used descriptors have been added to the vring.  This
+ * function acts as a memory barrier.
+ *
+ * @param vid
+ *  vhost device ID
+ * @param vring_idx
+ *  vring index
+ * @return
+ *  0 on success, -1 on failure
+ */
+int rte_vhost_vring_call(int vid, uint16_t vring_idx);
+
 /**
  * Get vhost RX queue avail count.
  *
diff --git a/examples/vhost/virtio_net.c b/examples/vhost/virtio_net.c
index 1ab57f526..252c5b8ce 100644
--- a/examples/vhost/virtio_net.c
+++ b/examples/vhost/virtio_net.c
@@ -207,13 +207,8 @@ vs_enqueue_pkts(struct vhost_dev *dev, uint16_t queue_id,
 	*(volatile uint16_t *)&vr->used->idx += count;
 	queue->last_used_idx += count;
 
-	/* flush used->idx update before we read avail->flags. */
-	rte_mb();
+	rte_vhost_vring_call(dev->vid, queue_id);
 
-	/* Kick the guest if necessary. */
-	if (!(vr->avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
-			&& (vr->callfd >= 0))
-		eventfd_write(vr->callfd, (eventfd_t)1);
 	return count;
 }
 
@@ -396,9 +391,7 @@ vs_dequeue_pkts(struct vhost_dev *dev, uint16_t queue_id,
 
 	vr->used->idx += i;
 
-	if (!(vr->avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
-			&& (vr->callfd >= 0))
-		eventfd_write(vr->callfd, (eventfd_t)1);
+	rte_vhost_vring_call(dev->vid, queue_id);
 
 	return i;
 }
diff --git a/examples/vhost_scsi/vhost_scsi.c b/examples/vhost_scsi/vhost_scsi.c
index b4f1f8d27..e30e61f6d 100644
--- a/examples/vhost_scsi/vhost_scsi.c
+++ b/examples/vhost_scsi/vhost_scsi.c
@@ -110,7 +110,7 @@ descriptor_is_wr(struct vring_desc *cur_desc)
 }
 
 static void
-submit_completion(struct vhost_scsi_task *task)
+submit_completion(struct vhost_scsi_task *task, uint32_t q_idx)
 {
 	struct rte_vhost_vring *vq;
 	struct vring_used *used;
@@ -131,7 +131,7 @@ submit_completion(struct vhost_scsi_task *task)
 	/* Send an interrupt back to the guest VM so that it knows
 	 * a completion is ready to be processed.
 	 */
-	eventfd_write(vq->callfd, (eventfd_t)1);
+	rte_vhost_vring_call(task->bdev->vid, q_idx);
 }
 
 static void
@@ -263,7 +263,7 @@ process_requestq(struct vhost_scsi_ctrlr *ctrlr, uint32_t q_idx)
 			task->resp->status = 0;
 			task->resp->resid = 0;
 		}
-		submit_completion(task);
+		submit_completion(task, q_idx);
 		rte_free(task);
 	}
 }
diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 4f8b73a09..1244d76d4 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -519,6 +519,27 @@ rte_vhost_get_vhost_vring(int vid, uint16_t vring_idx,
 	return 0;
 }
 
+int
+rte_vhost_vring_call(int vid, uint16_t vring_idx)
+{
+	struct virtio_net *dev;
+	struct vhost_virtqueue *vq;
+
+	dev = get_device(vid);
+	if (!dev)
+		return -1;
+
+	if (vring_idx >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[vring_idx];
+	if (!vq)
+		return -1;
+
+	vhost_vring_call(vq);
+	return 0;
+}
+
 uint16_t
 rte_vhost_avail_entries(int vid, uint16_t queue_id)
 {
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 1e7049535..b30187601 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -52,3 +52,10 @@ DPDK_17.08 {
 	rte_vhost_rx_queue_count;
 
 } DPDK_17.05;
+
+EXPERIMENTAL {
+	global:
+
+	rte_vhost_vring_call;
+
+} DPDK_17.08
-- 
2.14.3

Re: [dpdk-dev] [PATCH v2 0/2] vhost: introduce rte_vhost_vring_call()

[Maxime Coquelin]

Hi Stefan,

On 01/02/2018 10:31 AM, Stefan Hajnoczi wrote:
> v2:
>   * Add internal vhost_vring_call() helper function [Maxime]
> 
> These patches eliminate code duplication for vhost_virtqueue->callfd users by
> introducing rte_vhost_vring_call() (public API) and vhost_vring_call()
> (librte_vhost-internal API).
> 
> Stefan Hajnoczi (2):
>    vhost: add vhost_vring_call() helper
>    vhost: introduce rte_vhost_vring_call()
> 
>   lib/librte_vhost/rte_vhost.h           | 15 +++++++++++++++
>   lib/librte_vhost/vhost.h               | 12 ++++++++++++
>   examples/vhost/virtio_net.c            | 11 ++---------
>   examples/vhost_scsi/vhost_scsi.c       |  6 +++---
>   lib/librte_vhost/vhost.c               | 21 +++++++++++++++++++++
>   lib/librte_vhost/virtio_net.c          | 23 +++--------------------
>   lib/librte_vhost/rte_vhost_version.map |  7 +++++++
>   7 files changed, 63 insertions(+), 32 deletions(-)
> 

I just wonder whether tagging the new API as experimental is needed,
but apart from that it looks good to me:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks!
Maxime

Re: [dpdk-dev] [PATCH v2 0/2] vhost: introduce rte_vhost_vring_call()

[Maxime Coquelin]

On 01/02/2018 07:11 PM, Stefan Hajnoczi wrote:
> On Tue, Jan 02, 2018 at 11:27:02AM +0100, Maxime Coquelin wrote:
>> Hi Stefan,
>>
>> On 01/02/2018 10:31 AM, Stefan Hajnoczi wrote:
>>> v2:
>>>    * Add internal vhost_vring_call() helper function [Maxime]
>>>
>>> These patches eliminate code duplication for vhost_virtqueue->callfd users by
>>> introducing rte_vhost_vring_call() (public API) and vhost_vring_call()
>>> (librte_vhost-internal API).
>>>
>>> Stefan Hajnoczi (2):
>>>     vhost: add vhost_vring_call() helper
>>>     vhost: introduce rte_vhost_vring_call()
>>>
>>>    lib/librte_vhost/rte_vhost.h           | 15 +++++++++++++++
>>>    lib/librte_vhost/vhost.h               | 12 ++++++++++++
>>>    examples/vhost/virtio_net.c            | 11 ++---------
>>>    examples/vhost_scsi/vhost_scsi.c       |  6 +++---
>>>    lib/librte_vhost/vhost.c               | 21 +++++++++++++++++++++
>>>    lib/librte_vhost/virtio_net.c          | 23 +++--------------------
>>>    lib/librte_vhost/rte_vhost_version.map |  7 +++++++
>>>    7 files changed, 63 insertions(+), 32 deletions(-)
>>>
>>
>> I just wonder whether tagging the new API as experimental is needed,
>> but apart from that it looks good to me:
>>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
>  From 3.2. Managing ABI updates, General Guidelines:
> 
>    New APIs will be marked as experimental for at least one release to
>    allow any issues found by users of the new API to be fixed quickly.
> 
> http://dpdk.org/doc/guides/contributing/versioning.html
> 
> I'm new to DPDK so I followed this guideline strictly and expect the
> maintainers to mark the API stable at some point in the future when they
> are happy with it.  Maybe this guideline isn't followed strictly for
> librte_vhost?

Thanks for the pointer, it seems it is not strictly followed for
librte_vhost & other libs. Let's keep it experimental for the coming
release.

Maxime

> Stefan
> 

Re: [dpdk-dev] [PATCH v2 0/2] vhost: introduce rte_vhost_vring_call()

[Yuanhan Liu]

On Wed, Jan 03, 2018 at 09:17:32AM +0100, Maxime Coquelin wrote:
> >>Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> > From 3.2. Managing ABI updates, General Guidelines:
> >
> >   New APIs will be marked as experimental for at least one release to
> >   allow any issues found by users of the new API to be fixed quickly.
> >
> >http://dpdk.org/doc/guides/contributing/versioning.html
> >
> >I'm new to DPDK so I followed this guideline strictly and expect the
> >maintainers to mark the API stable at some point in the future when they
> >are happy with it.  Maybe this guideline isn't followed strictly for
> >librte_vhost?
> 
> Thanks for the pointer, it seems it is not strictly followed for
> librte_vhost & other libs. Let's keep it experimental for the coming
> release.

IIRC, it's a new guideline added recently (people were talking about
that in last year's DPDK user event after all).

And my understanding is, for a new (big set of) APIs, it's good to
make them experimental, as normally, it's not an easy task to make
a big stuff right in one time.

For this one, which is simple and straightforward, I don't see a
strong need to mark it as experimental.

Thus I have removed it (while apply). If anyone has objections,
let me know.

Thanks.

	--yliu