[dpdk-dev] [PATCH 00/17] implement packed virtqueues

[dpdk-dev] [PATCH 00/17] implement packed virtqueues

[Jens Freimann]

This is a basic implementation of packed virtqueues as specified in the
Virtio 1.1 draft. A compiled version of the current draft is available
at https://github.com/oasis-tcs/virtio-docs.git (or as .pdf at
https://github.com/oasis-tcs/virtio-docs/blob/master/virtio-v1.1-packed-wd10.pdf

It does not implement yet indirect descriptors and checksum offloading.

A packed virtqueue is different from a split virtqueue in that it
consists of only a single descriptor ring that replaces available and
used ring, index and descriptor buffer.

Each descriptor is readable and writable and has a flags field. These flags
will mark if a descriptor is available or used.  To detect new available descriptors
even after the ring has wrapped, device and driver each have a
single-bit wrap counter that is flipped from 0 to 1 and vice versa every time
the last descriptor in the ring is used/made available.

The idea behind this is to 1. improve performance by avoiding cache misses
and 2. be easier for devices to implement.

Regarding performance: with these patches I get 21.13 Mpps on my system
as compared to 18.8 Mpps with the virtio 1.0 code. Packet size was 64
bytes, 0.05% acceptable loss.  Test setup is described as in
http://dpdk.org/doc/guides/howto/pvp_reference_benchmark.html

Packet generator:
MoonGen
Intel(R) Xeon(R) CPU E5-2665 0 @ 2.40GHz
Intel X710 NIC
RHEL 7.4

Device under test:
Intel(R) Xeon(R) CPU E5-2667 v4 @ 3.20GHz
Intel X710 NIC
RHEL 7.4

VM on DuT: RHEL7.4

I plan to do more performance test with bigger frame sizes.

This patch series is based on a prototype implemented by Yuanhan Liu and
Tiwei Bie.

changes from v1->v2:
* don't use VIRTQ_DESC_F_NEXT in used descriptors (Jason)
* no rte_panice() in guest triggerable code (Maxime)
* use unlikely when checking for vq (Maxime)
* rename everything from _1_1 to _packed  (Yuanhan)
* add two more patches to implement mergeable receive buffers 

regards,
Jens 

Jens Freimann (13):
  net/virtio: vring init for packed queues
  net/virtio: don't call virtio_disable_intr() for packed queues
  net/virtio: add virtio 1.1 defines
  net/virtio: add packed virtqueue helpers
  net/virtio: don't dump split virtqueue data
  net/virtio: implement transmit path for packed queues
  vhost: add virtio 1.1 defines
  vhost: add helpers for packed virtqueues
  vhost: dequeue for packed queues
  vhost: packed queue enqueue path
  net/virtio: disable ctrl virtqueue for packed rings
  net/virtio: add support for mergeable buffers with packed virtqueues
  vhost: support mergeable rx buffers with packed queues

Yuanhan Liu (4):
  net/virtio-user: add option to use packed queues
  net/virtio: implement receive path for packed queues
  vhost: vring address setup for packed queues
  vhost: enable packed virtqueues

 drivers/net/virtio/Makefile                      |   1 +
 drivers/net/virtio/virtio_ethdev.c               |  60 ++-
 drivers/net/virtio/virtio_ethdev.h               |   5 +
 drivers/net/virtio/virtio_pci.h                  |   8 +
 drivers/net/virtio/virtio_ring.h                 |  64 ++-
 drivers/net/virtio/virtio_rxtx.c                 | 248 ++++++++++-
 drivers/net/virtio/virtio_rxtx_1.1.c             | 161 ++++++++
 drivers/net/virtio/virtio_user/virtio_user_dev.c |  12 +-
 drivers/net/virtio/virtio_user/virtio_user_dev.h |   3 +-
 drivers/net/virtio/virtio_user_ethdev.c          |  15 +-
 drivers/net/virtio/virtqueue.c                   |   3 +
 drivers/net/virtio/virtqueue.h                   |  15 +-
 lib/librte_vhost/vhost.c                         |   4 +
 lib/librte_vhost/vhost.h                         |   7 +-
 lib/librte_vhost/vhost_user.c                    |  17 +-
 lib/librte_vhost/virtio-1.1.h                    |  63 +++
 lib/librte_vhost/virtio_net.c                    | 506 +++++++++++++++++++++--
 17 files changed, 1127 insertions(+), 65 deletions(-)
 create mode 100644 drivers/net/virtio/virtio_rxtx_1.1.c
 create mode 100644 lib/librte_vhost/virtio-1.1.h

-- 
1.8.3.1

[dpdk-dev] [PATCH 01/17] net/virtio: vring init for packed queues

[Jens Freimann]

Add and initialize descriptor data structures.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 22 ++++++++++++----------
 drivers/net/virtio/virtio_pci.h    |  8 ++++++++
 drivers/net/virtio/virtio_ring.h   | 25 +++++++++++++++++++++++--
 drivers/net/virtio/virtqueue.h     | 10 ++++++++++
 4 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 884f74a..ce4f53d 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -299,19 +299,21 @@ struct rte_virtio_xstats_name_off {
 
 	PMD_INIT_FUNC_TRACE();
 
-	/*
-	 * Reinitialise since virtio port might have been stopped and restarted
-	 */
 	memset(ring_mem, 0, vq->vq_ring_size);
-	vring_init(vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
-	vq->vq_used_cons_idx = 0;
-	vq->vq_desc_head_idx = 0;
-	vq->vq_avail_idx = 0;
-	vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
+	vring_init(vq->hw, vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
+
 	vq->vq_free_cnt = vq->vq_nentries;
 	memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
+	vq->vq_used_cons_idx = 0;
+	vq->vq_avail_idx     = 0;
+	if (vtpci_packed_queue(vq->hw)) {
+		vring_desc_init_packed(vr, size);
+	} else {
+		vq->vq_desc_head_idx = 0;
+		vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
 
-	vring_desc_init(vr->desc, size);
+		vring_desc_init(vr->desc, size);
+	}
 
 	/*
 	 * Disable device(host) interrupting guest
@@ -386,7 +388,7 @@ struct rte_virtio_xstats_name_off {
 	/*
 	 * Reserve a memzone for vring elements
 	 */
-	size = vring_size(vq_size, VIRTIO_PCI_VRING_ALIGN);
+	size = vring_size(hw, vq_size, VIRTIO_PCI_VRING_ALIGN);
 	vq->vq_ring_size = RTE_ALIGN_CEIL(size, VIRTIO_PCI_VRING_ALIGN);
 	PMD_INIT_LOG(DEBUG, "vring_size: %d, rounded_vring_size: %d",
 		     size, vq->vq_ring_size);
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index a28ba83..528fb46 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -112,6 +112,8 @@
 
 #define VIRTIO_F_VERSION_1		32
 #define VIRTIO_F_IOMMU_PLATFORM	33
+#define VIRTIO_F_RING_PACKED		34
+#define VIRTIO_F_IN_ORDER		35
 
 /*
  * Some VirtIO feature bits (currently bits 28 through 31) are
@@ -304,6 +306,12 @@ enum virtio_msix_status {
 	return (hw->guest_features & (1ULL << bit)) != 0;
 }
 
+static inline int
+vtpci_packed_queue(struct virtio_hw *hw)
+{
+	return vtpci_with_feature(hw, VIRTIO_F_RING_PACKED);
+}
+
 /*
  * Function declaration from virtio_pci.c
  */
diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index 9e3c2a0..fc45e34 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -9,6 +9,7 @@
 
 #include <rte_common.h>
 
+
 /* This marks a buffer as continuing via the next field. */
 #define VRING_DESC_F_NEXT       1
 /* This marks a buffer as write-only (otherwise read-only). */
@@ -54,11 +55,23 @@ struct vring_used {
 	struct vring_used_elem ring[0];
 };
 
+/* For support of packed virtqueues in Virtio 1.1 the format of descriptors
+ * looks like this.
+ */
+struct vring_desc_packed {
+	uint64_t addr;
+	uint32_t len;
+	uint16_t index;
+	uint16_t flags;
+};
+
+
 struct vring {
 	unsigned int num;
 	struct vring_desc  *desc;
 	struct vring_avail *avail;
 	struct vring_used  *used;
+	struct vring_desc_packed *desc_packed;
 };
 
 /* The standard layout for the ring is a continuous chunk of memory which
@@ -95,10 +108,13 @@ struct vring {
 #define vring_avail_event(vr) (*(uint16_t *)&(vr)->used->ring[(vr)->num])
 
 static inline size_t
-vring_size(unsigned int num, unsigned long align)
+vring_size(struct virtio_hw *hw, unsigned int num, unsigned long align)
 {
 	size_t size;
 
+	if (vtpci_packed_queue(hw))
+		return num * sizeof(struct vring_desc_packed);
+
 	size = num * sizeof(struct vring_desc);
 	size += sizeof(struct vring_avail) + (num * sizeof(uint16_t));
 	size = RTE_ALIGN_CEIL(size, align);
@@ -108,10 +124,15 @@ struct vring {
 }
 
 static inline void
-vring_init(struct vring *vr, unsigned int num, uint8_t *p,
+vring_init(struct virtio_hw *hw, struct vring *vr, unsigned int num, uint8_t *p,
 	unsigned long align)
 {
 	vr->num = num;
+	if (vtpci_packed_queue(hw)) {
+		vr->desc_packed = (struct vring_desc_packed *)p;
+		return;
+	}
+
 	vr->desc = (struct vring_desc *) p;
 	vr->avail = (struct vring_avail *) (p +
 		num * sizeof(struct vring_desc));
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 14364f3..cc2e7c0 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -245,6 +245,16 @@ struct virtio_tx_region {
 			   __attribute__((__aligned__(16)));
 };
 
+static inline void
+vring_desc_init_packed(struct vring *vr, int n)
+{
+	int i;
+	for (i = 0; i < n; i++) {
+		struct vring_desc_packed *desc = &vr->desc_packed[i];
+		desc->index = i;
+	}
+}
+
 /* Chain all the descriptors in the ring with an END */
 static inline void
 vring_desc_init(struct vring_desc *dp, uint16_t n)
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 01/17] net/virtio: vring init for packed queues

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:04PM +0100, Jens Freimann wrote:
> Add and initialize descriptor data structures.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
[...]
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -9,6 +9,7 @@
>  
>  #include <rte_common.h>
>  
> +

There is no need to add this blank line.

>  /* This marks a buffer as continuing via the next field. */
>  #define VRING_DESC_F_NEXT       1
>  /* This marks a buffer as write-only (otherwise read-only). */
> @@ -54,11 +55,23 @@ struct vring_used {
>  	struct vring_used_elem ring[0];
>  };
[...]
>  struct vring {
>  	unsigned int num;
>  	struct vring_desc  *desc;
>  	struct vring_avail *avail;
>  	struct vring_used  *used;
> +	struct vring_desc_packed *desc_packed;

Maybe it's better to use anonymous union here.

>  };
>  
>  /* The standard layout for the ring is a continuous chunk of memory which
> @@ -95,10 +108,13 @@ struct vring {
>  #define vring_avail_event(vr) (*(uint16_t *)&(vr)->used->ring[(vr)->num])
>  
>  static inline size_t
> -vring_size(unsigned int num, unsigned long align)
> +vring_size(struct virtio_hw *hw, unsigned int num, unsigned long align)
>  {
>  	size_t size;
>  
> +	if (vtpci_packed_queue(hw))
> +		return num * sizeof(struct vring_desc_packed);
> +

Besides the descriptors, the ring also contains event
suppression structures.

Thanks

Re: [dpdk-dev] [PATCH 01/17] net/virtio: vring init for packed queues

[Maxime Coquelin]

On 03/16/2018 04:21 PM, Jens Freimann wrote:
> Add and initialize descriptor data structures.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 22 ++++++++++++----------
>   drivers/net/virtio/virtio_pci.h    |  8 ++++++++
>   drivers/net/virtio/virtio_ring.h   | 25 +++++++++++++++++++++++--
>   drivers/net/virtio/virtqueue.h     | 10 ++++++++++
>   4 files changed, 53 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 884f74a..ce4f53d 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -299,19 +299,21 @@ struct rte_virtio_xstats_name_off {
>   
>   	PMD_INIT_FUNC_TRACE();
>   
> -	/*
> -	 * Reinitialise since virtio port might have been stopped and restarted
> -	 */

Are you sure the comment has to be removed?

>   	memset(ring_mem, 0, vq->vq_ring_size);
> -	vring_init(vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
> -	vq->vq_used_cons_idx = 0;
> -	vq->vq_desc_head_idx = 0;
> -	vq->vq_avail_idx = 0;
> -	vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
> +	vring_init(vq->hw, vr, size, ring_mem, VIRTIO_PCI_VRING_ALIGN);
> +
>   	vq->vq_free_cnt = vq->vq_nentries;
>   	memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
> +	vq->vq_used_cons_idx = 0;
> +	vq->vq_avail_idx     = 0;
> +	if (vtpci_packed_queue(vq->hw)) {
> +		vring_desc_init_packed(vr, size);
> +	} else {
> +		vq->vq_desc_head_idx = 0;
> +		vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
>   
> -	vring_desc_init(vr->desc, size);
> +		vring_desc_init(vr->desc, size);
> +	}
>   
>   	/*
>   	 * Disable device(host) interrupting guest
> @@ -386,7 +388,7 @@ struct rte_virtio_xstats_name_off {
>   	/*
>   	 * Reserve a memzone for vring elements
>   	 */
> -	size = vring_size(vq_size, VIRTIO_PCI_VRING_ALIGN);
> +	size = vring_size(hw, vq_size, VIRTIO_PCI_VRING_ALIGN);
>   	vq->vq_ring_size = RTE_ALIGN_CEIL(size, VIRTIO_PCI_VRING_ALIGN);
>   	PMD_INIT_LOG(DEBUG, "vring_size: %d, rounded_vring_size: %d",
>   		     size, vq->vq_ring_size);
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
> index a28ba83..528fb46 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -112,6 +112,8 @@
>   
>   #define VIRTIO_F_VERSION_1		32
>   #define VIRTIO_F_IOMMU_PLATFORM	33
> +#define VIRTIO_F_RING_PACKED		34
> +#define VIRTIO_F_IN_ORDER		35
>   
>   /*
>    * Some VirtIO feature bits (currently bits 28 through 31) are
> @@ -304,6 +306,12 @@ enum virtio_msix_status {
>   	return (hw->guest_features & (1ULL << bit)) != 0;
>   }
>   
> +static inline int
> +vtpci_packed_queue(struct virtio_hw *hw)
> +{
> +	return vtpci_with_feature(hw, VIRTIO_F_RING_PACKED);
> +}
> +
>   /*
>    * Function declaration from virtio_pci.c
>    */
> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> index 9e3c2a0..fc45e34 100644
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -9,6 +9,7 @@
>   
>   #include <rte_common.h>
>   
> +
>   /* This marks a buffer as continuing via the next field. */
>   #define VRING_DESC_F_NEXT       1
>   /* This marks a buffer as write-only (otherwise read-only). */
> @@ -54,11 +55,23 @@ struct vring_used {
>   	struct vring_used_elem ring[0];
>   };
>   
> +/* For support of packed virtqueues in Virtio 1.1 the format of descriptors
> + * looks like this.
> + */
> +struct vring_desc_packed {
> +	uint64_t addr;
> +	uint32_t len;
> +	uint16_t index;
> +	uint16_t flags;
> +};
> +
> +
Trailing new line.
>   struct vring {
>   	unsigned int num;
>   	struct vring_desc  *desc;
>   	struct vring_avail *avail;
>   	struct vring_used  *used;
> +	struct vring_desc_packed *desc_packed;
>   };
...

Re: [dpdk-dev] [PATCH 01/17] net/virtio: vring init for packed queues

[Jens Freimann]

On Wed, Apr 04, 2018 at 09:33:20AM +0200, Maxime Coquelin wrote:
>
>
>On 03/16/2018 04:21 PM, Jens Freimann wrote:
>>Add and initialize descriptor data structures.
>>
>>Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>>---
>>  drivers/net/virtio/virtio_ethdev.c | 22 ++++++++++++----------
>>  drivers/net/virtio/virtio_pci.h    |  8 ++++++++
>>  drivers/net/virtio/virtio_ring.h   | 25 +++++++++++++++++++++++--
>>  drivers/net/virtio/virtqueue.h     | 10 ++++++++++
>>  4 files changed, 53 insertions(+), 12 deletions(-)
>>
>>diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>>index 884f74a..ce4f53d 100644
>>--- a/drivers/net/virtio/virtio_ethdev.c
>>+++ b/drivers/net/virtio/virtio_ethdev.c
>>@@ -299,19 +299,21 @@ struct rte_virtio_xstats_name_off {
>>  	PMD_INIT_FUNC_TRACE();
>>-	/*
>>-	 * Reinitialise since virtio port might have been stopped and restarted
>>-	 */
>
>Are you sure the comment has to be removed?

Fixed in v3. Thanks!

regards,
Jens 

[dpdk-dev] [PATCH 02/17] net/virtio: don't call virtio_disable_intr() for packed queues

[Jens Freimann]

When VIRTIO_F_PACKED_RING is set, don't call virtio_disable_intr().
This function accesses data structures which are not
available when packed virtqueues are enabled.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index ce4f53d..19536eb 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -313,12 +313,11 @@ struct rte_virtio_xstats_name_off {
 		vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
 
 		vring_desc_init(vr->desc, size);
+		/*
+		 * Disable device(host) interrupting guest
+		 */
+		virtqueue_disable_intr(vq);
 	}
-
-	/*
-	 * Disable device(host) interrupting guest
-	 */
-	virtqueue_disable_intr(vq);
 }
 
 static int
@@ -736,7 +735,8 @@ struct rte_virtio_xstats_name_off {
 	struct virtnet_rx *rxvq = dev->data->rx_queues[queue_id];
 	struct virtqueue *vq = rxvq->vq;
 
-	virtqueue_disable_intr(vq);
+	if (!vtpci_packed_queue(vq->hw))
+		virtqueue_disable_intr(vq);
 	return 0;
 }
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 02/17] net/virtio: don't call virtio_disable_intr() for packed queues

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:05PM +0100, Jens Freimann wrote:
> When VIRTIO_F_PACKED_RING is set, don't call virtio_disable_intr().
> This function accesses data structures which are not
> available when packed virtqueues are enabled.

Packed ring has event suppression structures.

Thanks

> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index ce4f53d..19536eb 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -313,12 +313,11 @@ struct rte_virtio_xstats_name_off {
>  		vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
>  
>  		vring_desc_init(vr->desc, size);
> +		/*
> +		 * Disable device(host) interrupting guest
> +		 */
> +		virtqueue_disable_intr(vq);
>  	}
> -
> -	/*
> -	 * Disable device(host) interrupting guest
> -	 */
> -	virtqueue_disable_intr(vq);
>  }
>  
>  static int
> @@ -736,7 +735,8 @@ struct rte_virtio_xstats_name_off {
>  	struct virtnet_rx *rxvq = dev->data->rx_queues[queue_id];
>  	struct virtqueue *vq = rxvq->vq;
>  
> -	virtqueue_disable_intr(vq);
> +	if (!vtpci_packed_queue(vq->hw))
> +		virtqueue_disable_intr(vq);
>  	return 0;
>  }
>  
> -- 
> 1.8.3.1
> 

[dpdk-dev] [PATCH 03/17] net/virtio: add virtio 1.1 defines

[Jens Freimann]

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ring.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index fc45e34..6eb0077 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -17,6 +17,12 @@
 /* This means the buffer contains a list of buffer descriptors. */
 #define VRING_DESC_F_INDIRECT   4
 
+#define VIRTQ_DESC_F_AVAIL     7
+#define VIRTQ_DESC_F_USED      15
+#define DESC_USED (1ULL << VIRTQ_DESC_F_USED)
+#define DESC_AVAIL (1ULL << VIRTQ_DESC_F_AVAIL)
+
+
 /* The Host uses this in used->flags to advise the Guest: don't kick me
  * when you add a buffer.  It's unreliable, so it's simply an
  * optimization.  Guest will still kick if it's out of buffers. */
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 03/17] net/virtio: add virtio 1.1 defines

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:06PM +0100, Jens Freimann wrote:
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtio_ring.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> index fc45e34..6eb0077 100644
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -17,6 +17,12 @@
>  /* This means the buffer contains a list of buffer descriptors. */
>  #define VRING_DESC_F_INDIRECT   4
>  
> +#define VIRTQ_DESC_F_AVAIL     7
> +#define VIRTQ_DESC_F_USED      15

It's better to use consistent prefix (VRING_DESC_F_ instead
of VIRTQ_DESC_F_).

Besides, maybe it's better to define them as (1ULL << 7) and
(1ULL << 15) to make them consistent with the definitions of
other VRING_DESC_F_* flags.

> +#define DESC_USED (1ULL << VIRTQ_DESC_F_USED)
> +#define DESC_AVAIL (1ULL << VIRTQ_DESC_F_AVAIL)
> +
> +

There is no need to add two blank lines here.

Thanks

>  /* The Host uses this in used->flags to advise the Guest: don't kick me
>   * when you add a buffer.  It's unreliable, so it's simply an
>   * optimization.  Guest will still kick if it's out of buffers. */
> -- 
> 1.8.3.1
> 

[dpdk-dev] [PATCH 04/17] net/virtio: add packed virtqueue helpers

[Jens Freimann]

Add helper functions to set/clear and check descriptor flags.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ring.h | 33 +++++++++++++++++++++++++++++++++
 drivers/net/virtio/virtqueue.c   |  3 +++
 2 files changed, 36 insertions(+)

diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index 6eb0077..2de2ede 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -74,12 +74,45 @@ struct vring_desc_packed {
 
 struct vring {
 	unsigned int num;
+	unsigned int avail_wrap_counter;
 	struct vring_desc  *desc;
 	struct vring_avail *avail;
 	struct vring_used  *used;
 	struct vring_desc_packed *desc_packed;
 };
 
+static inline void toggle_wrap_counter(struct vring *vr)
+{
+	vr->avail_wrap_counter ^= 1;
+}
+
+static inline void _set_desc_avail(struct vring_desc_packed *desc,
+				   int wrap_counter)
+{
+	uint16_t flags = desc->flags;
+
+	if (wrap_counter) {
+		flags |= DESC_AVAIL;
+		flags &= ~DESC_USED;
+	} else {
+		flags &= ~DESC_AVAIL;
+		flags |= DESC_USED;
+	}
+
+	desc->flags = flags;
+}
+
+static inline void set_desc_avail(struct vring *vr,
+				  struct vring_desc_packed *desc)
+{
+	_set_desc_avail(desc, vr->avail_wrap_counter);
+}
+
+static inline int desc_is_used(struct vring_desc_packed *desc)
+{
+	return !(desc->flags & DESC_AVAIL) == !(desc->flags & DESC_USED);
+}
+
 /* The standard layout for the ring is a continuous chunk of memory which
  * looks like this.  We assume num is a power of 2.
  *
diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
index a7d0a9c..6806056 100644
--- a/drivers/net/virtio/virtqueue.c
+++ b/drivers/net/virtio/virtqueue.c
@@ -65,6 +65,9 @@ struct rte_mbuf *
 	uint16_t used_idx, desc_idx;
 	uint16_t nb_used, i;
 
+	if (vtpci_packed_queue(vq->hw))
+		return;
+
 	nb_used = VIRTQUEUE_NUSED(vq);
 
 	for (i = 0; i < nb_used; i++) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 04/17] net/virtio: add packed virtqueue helpers

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:07PM +0100, Jens Freimann wrote:
> Add helper functions to set/clear and check descriptor flags.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
[...]
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -74,12 +74,45 @@ struct vring_desc_packed {
>  
>  struct vring {
>  	unsigned int num;
> +	unsigned int avail_wrap_counter;
>  	struct vring_desc  *desc;
>  	struct vring_avail *avail;
>  	struct vring_used  *used;
>  	struct vring_desc_packed *desc_packed;

Maybe it's better to use anonymous union.

>  };
[...]
> diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
> index a7d0a9c..6806056 100644
> --- a/drivers/net/virtio/virtqueue.c
> +++ b/drivers/net/virtio/virtqueue.c
> @@ -65,6 +65,9 @@ struct rte_mbuf *

Please make sure the diff contains function name.

>  	uint16_t used_idx, desc_idx;
>  	uint16_t nb_used, i;
>  
> +	if (vtpci_packed_queue(vq->hw))
> +		return;

I guess packed-ring also needs to support virtqueue_rxvq_flush().

Thanks

> +
>  	nb_used = VIRTQUEUE_NUSED(vq);
>  
>  	for (i = 0; i < nb_used; i++) {
> -- 
> 1.8.3.1
> 

[dpdk-dev] [PATCH 05/17] net/virtio: don't dump split virtqueue data

[Jens Freimann]

VIRTQUEUE_DUMP access split virtqueue data which is not
correct when packed virtqueues are used.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtqueue.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index cc2e7c0..82160ca 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -359,7 +359,9 @@ struct virtio_tx_region {
 }
 
 #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DUMP
-#define VIRTQUEUE_DUMP(vq) do { \
+#define VIRTQUEUE_DUMP(vq) \
+	do { \
+	if (vtpci_packed_queue((vq)->hw)) break; \
 	uint16_t used_idx, nused; \
 	used_idx = (vq)->vq_ring.used->idx; \
 	nused = (uint16_t)(used_idx - (vq)->vq_used_cons_idx); \
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 05/17] net/virtio: don't dump split virtqueue data

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:08PM +0100, Jens Freimann wrote:
> VIRTQUEUE_DUMP access split virtqueue data which is not
> correct when packed virtqueues are used.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtqueue.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index cc2e7c0..82160ca 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -359,7 +359,9 @@ struct virtio_tx_region {
>  }
>  
>  #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DUMP
> -#define VIRTQUEUE_DUMP(vq) do { \
> +#define VIRTQUEUE_DUMP(vq) \
> +	do { \
> +	if (vtpci_packed_queue((vq)->hw)) break; \

Maybe it's better to make VIRTQUEUE_DUMP() support packed ring.

Thanks

>  	uint16_t used_idx, nused; \
>  	used_idx = (vq)->vq_ring.used->idx; \
>  	nused = (uint16_t)(used_idx - (vq)->vq_used_cons_idx); \
> -- 
> 1.8.3.1
> 

[dpdk-dev] [PATCH 06/17] net/virtio-user: add option to use packed queues

[Jens Freimann]

From: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Add option to enable packed queue support for virtio-user
devices.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 12 ++++++++++--
 drivers/net/virtio/virtio_user/virtio_user_dev.h |  3 ++-
 drivers/net/virtio/virtio_user_ethdev.c          | 15 ++++++++++++++-
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index f90fee9..5f2ff09 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -314,11 +314,13 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
 	 1ULL << VIRTIO_NET_F_GUEST_CSUM	|	\
 	 1ULL << VIRTIO_NET_F_GUEST_TSO4	|	\
 	 1ULL << VIRTIO_NET_F_GUEST_TSO6	|	\
-	 1ULL << VIRTIO_F_VERSION_1)
+	 1ULL << VIRTIO_F_VERSION_1		|	\
+	 1ULL << VIRTIO_F_RING_PACKED)
 
 int
 virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
-		     int cq, int queue_size, const char *mac, char **ifname)
+		     int cq, int queue_size, const char *mac, char **ifname,
+		     int version_1_1)
 {
 	snprintf(dev->path, PATH_MAX, "%s", path);
 	dev->max_queue_pairs = queues;
@@ -347,6 +349,12 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
 		PMD_INIT_LOG(ERR, "get_features failed: %s", strerror(errno));
 		return -1;
 	}
+
+	if (version_1_1)
+		dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
+	else
+		dev->device_features &= ~(1ull << VIRTIO_F_RING_PACKED);
+
 	if (dev->mac_specified)
 		dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
 
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 64467b4..ce1e103 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -42,7 +42,8 @@ struct virtio_user_dev {
 int virtio_user_start_device(struct virtio_user_dev *dev);
 int virtio_user_stop_device(struct virtio_user_dev *dev);
 int virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
-			 int cq, int queue_size, const char *mac, char **ifname);
+			 int cq, int queue_size, const char *mac, char **ifname,
+			 int version_1_1);
 void virtio_user_dev_uninit(struct virtio_user_dev *dev);
 void virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx);
 #endif
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 2636490..ee291b3 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -278,6 +278,8 @@
 	VIRTIO_USER_ARG_QUEUE_SIZE,
 #define VIRTIO_USER_ARG_INTERFACE_NAME "iface"
 	VIRTIO_USER_ARG_INTERFACE_NAME,
+#define VIRTIO_USER_ARG_VERSION_1_1     "version_1_1"
+	VIRTIO_USER_ARG_VERSION_1_1,
 	NULL
 };
 
@@ -382,6 +384,7 @@
 	char *ifname = NULL;
 	char *mac_addr = NULL;
 	int ret = -1;
+	uint64_t version_1_1 = 0;
 
 	kvlist = rte_kvargs_parse(rte_vdev_device_args(dev), valid_args);
 	if (!kvlist) {
@@ -456,6 +459,15 @@
 		cq = 1;
 	}
 
+	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_VERSION_1_1) == 1) {
+		if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_VERSION_1_1,
+				       &get_integer_arg, &version_1_1) < 0) {
+			PMD_INIT_LOG(ERR, "error to parse %s",
+				     VIRTIO_USER_ARG_VERSION_1_1);
+			goto end;
+		}
+	}
+
 	if (queues > 1 && cq == 0) {
 		PMD_INIT_LOG(ERR, "multi-q requires ctrl-q");
 		goto end;
@@ -477,7 +489,8 @@
 
 		hw = eth_dev->data->dev_private;
 		if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
-				 queue_size, mac_addr, &ifname) < 0) {
+				 queue_size, mac_addr, &ifname,
+				 version_1_1) < 0) {
 			PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
 			virtio_user_eth_dev_free(eth_dev);
 			goto end;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 06/17] net/virtio-user: add option to use packed queues

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:09PM +0100, Jens Freimann wrote:
[...]
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
> index 2636490..ee291b3 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -278,6 +278,8 @@
>  	VIRTIO_USER_ARG_QUEUE_SIZE,
>  #define VIRTIO_USER_ARG_INTERFACE_NAME "iface"
>  	VIRTIO_USER_ARG_INTERFACE_NAME,
> +#define VIRTIO_USER_ARG_VERSION_1_1     "version_1_1"
> +	VIRTIO_USER_ARG_VERSION_1_1,

Maybe we can enable packed-ring by default for virtio-user.
If we really need a flag to enable it, the devarg name should
be packed_ring instead of version_1_1.

Thanks

Re: [dpdk-dev] [PATCH 06/17] net/virtio-user: add option to use packed queues

[Jens Freimann]

On Mon, Mar 19, 2018 at 04:33:27PM +0800, Tiwei Bie wrote:
>On Fri, Mar 16, 2018 at 04:21:09PM +0100, Jens Freimann wrote:
>[...]
>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
>> index 2636490..ee291b3 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -278,6 +278,8 @@
>>  	VIRTIO_USER_ARG_QUEUE_SIZE,
>>  #define VIRTIO_USER_ARG_INTERFACE_NAME "iface"
>>  	VIRTIO_USER_ARG_INTERFACE_NAME,
>> +#define VIRTIO_USER_ARG_VERSION_1_1     "version_1_1"
>> +	VIRTIO_USER_ARG_VERSION_1_1,
>
>Maybe we can enable packed-ring by default for virtio-user.
>If we really need a flag to enable it, the devarg name should
>be packed_ring instead of version_1_1.

Thinking about it, we should probably just get rid of this patch and
let feature negotiation do its thing. 

Thanks!

regards,
Jens 

[dpdk-dev] [PATCH 07/17] net/virtio: implement transmit path for packed queues

[Jens Freimann]

This implements the transmit path for devices with
support for Virtio 1.1.

Add the feature bit for Virtio 1.1 and enable code to
add buffers to vring and mark descriptors as available.

This is based on a patch by Yuanhan Liu.

Signed-off-by: Jens Freiman <jfreimann@redhat.com>
---
 drivers/net/virtio/Makefile          |   1 +
 drivers/net/virtio/virtio_ethdev.c   |  11 ++-
 drivers/net/virtio/virtio_ethdev.h   |   3 +
 drivers/net/virtio/virtio_rxtx.c     |   7 +-
 drivers/net/virtio/virtio_rxtx_1.1.c | 161 +++++++++++++++++++++++++++++++++++
 5 files changed, 180 insertions(+), 3 deletions(-)
 create mode 100644 drivers/net/virtio/virtio_rxtx_1.1.c

diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
index 6c2c996..aa1e600 100644
--- a/drivers/net/virtio/Makefile
+++ b/drivers/net/virtio/Makefile
@@ -28,6 +28,7 @@ LIBABIVER := 1
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtqueue.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_pci.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx.c
+SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_1.1.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
 
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 19536eb..722a2cd 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -383,6 +383,8 @@ struct rte_virtio_xstats_name_off {
 	vq->hw = hw;
 	vq->vq_queue_index = vtpci_queue_idx;
 	vq->vq_nentries = vq_size;
+	if (vtpci_packed_queue(hw))
+		vq->vq_ring.avail_wrap_counter = 1;
 
 	/*
 	 * Reserve a memzone for vring elements
@@ -603,7 +605,8 @@ struct rte_virtio_xstats_name_off {
 	}
 
 	vtpci_reset(hw);
-	virtio_dev_free_mbufs(dev);
+	if (!vtpci_packed_queue(hw))
+		virtio_dev_free_mbufs(dev);
 	virtio_free_queues(hw);
 }
 
@@ -1360,7 +1363,11 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 		eth_dev->rx_pkt_burst = &virtio_recv_pkts;
 	}
 
-	if (hw->use_simple_tx) {
+	if (vtpci_packed_queue(hw)) {
+		PMD_INIT_LOG(INFO, "virtio: using virtio 1.1 Tx path on port %u",
+			eth_dev->data->port_id);
+		eth_dev->tx_pkt_burst = virtio_xmit_pkts_packed;
+	} else if (hw->use_simple_tx) {
 		PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
 			eth_dev->data->port_id);
 		eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index 4539d2e..cfefe4d 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -36,6 +36,7 @@
 	 1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE |	\
 	 1u << VIRTIO_RING_F_INDIRECT_DESC |    \
 	 1ULL << VIRTIO_F_VERSION_1       |	\
+	 1ULL << VIRTIO_F_RING_PACKED     |	\
 	 1ULL << VIRTIO_F_IOMMU_PLATFORM)
 
 #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES	\
@@ -77,6 +78,8 @@ uint16_t virtio_recv_mergeable_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 
 uint16_t virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
 		uint16_t nb_pkts);
+uint16_t virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
+		uint16_t nb_pkts);
 
 uint16_t virtio_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
 		uint16_t nb_pkts);
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 8dbf2a3..f1df004 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -545,6 +545,10 @@
 
 	PMD_INIT_FUNC_TRACE();
 
+	if (vtpci_packed_queue(hw)) {
+		vq->vq_ring.avail_wrap_counter = 1;
+	}
+
 	if (hw->use_simple_tx) {
 		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
 			vq->vq_ring.avail->ring[desc_idx] =
@@ -565,7 +569,8 @@
 			vq->vq_ring.avail->ring[desc_idx] = desc_idx;
 	}
 
-	VIRTQUEUE_DUMP(vq);
+	if (!vtpci_packed_queue(hw))
+		VIRTQUEUE_DUMP(vq);
 
 	return 0;
 }
diff --git a/drivers/net/virtio/virtio_rxtx_1.1.c b/drivers/net/virtio/virtio_rxtx_1.1.c
new file mode 100644
index 0000000..05ec085
--- /dev/null
+++ b/drivers/net/virtio/virtio_rxtx_1.1.c
@@ -0,0 +1,161 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+
+#include <rte_cycles.h>
+#include <rte_memory.h>
+#include <rte_memzone.h>
+#include <rte_branch_prediction.h>
+#include <rte_mempool.h>
+#include <rte_malloc.h>
+#include <rte_mbuf.h>
+#include <rte_ether.h>
+#include <rte_ethdev.h>
+#include <rte_prefetch.h>
+#include <rte_string_fns.h>
+#include <rte_errno.h>
+#include <rte_byteorder.h>
+#include <rte_cpuflags.h>
+#include <rte_net.h>
+#include <rte_ip.h>
+#include <rte_udp.h>
+#include <rte_tcp.h>
+
+#include "virtio_logs.h"
+#include "virtio_ethdev.h"
+#include "virtio_pci.h"
+#include "virtqueue.h"
+#include "virtio_rxtx.h"
+#include "virtio_ring.h"
+
+/* Cleanup from completed transmits. */
+static void
+virtio_xmit_cleanup(struct virtqueue *vq)
+{
+	uint16_t idx;
+	uint16_t size = vq->vq_nentries;
+	struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
+
+	idx = vq->vq_used_cons_idx & (size - 1);
+	while (desc_is_used(&desc[idx]) &&
+	       vq->vq_free_cnt < size) {
+		while (desc[idx].flags & VRING_DESC_F_NEXT) {
+			vq->vq_free_cnt++;
+			idx = ++vq->vq_used_cons_idx & (size - 1);
+		}
+		vq->vq_free_cnt++;
+		idx = ++vq->vq_used_cons_idx & (size - 1);
+	}
+}
+
+uint16_t
+virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
+		     uint16_t nb_pkts)
+{
+	struct virtnet_tx *txvq = tx_queue;
+	struct virtqueue *vq = txvq->vq;
+	uint16_t i;
+	struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
+	uint16_t idx;
+	struct vq_desc_extra *dxp;
+
+	if (unlikely(nb_pkts < 1))
+		return nb_pkts;
+
+	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
+
+	if (likely(vq->vq_free_cnt < vq->vq_free_thresh))
+		virtio_xmit_cleanup(vq);
+
+	for (i = 0; i < nb_pkts; i++) {
+		struct rte_mbuf *txm = tx_pkts[i];
+		struct virtio_tx_region *txr = txvq->virtio_net_hdr_mz->addr;
+		uint16_t head_idx;
+		int wrap_counter;
+
+		if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
+			virtio_xmit_cleanup(vq);
+
+			if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
+				PMD_TX_LOG(ERR,
+					   "No free tx descriptors to transmit");
+				break;
+			}
+		}
+
+		txvq->stats.bytes += txm->pkt_len;
+
+		vq->vq_free_cnt -= txm->nb_segs + 1;
+
+		idx = (vq->vq_avail_idx++) & (vq->vq_nentries - 1);
+		head_idx = idx;
+		wrap_counter = vq->vq_ring.avail_wrap_counter;
+
+		if ((vq->vq_avail_idx & (vq->vq_nentries - 1)) == 0)
+			toggle_wrap_counter(&vq->vq_ring);
+
+		dxp = &vq->vq_descx[idx];
+		if (dxp->cookie != NULL)
+			rte_pktmbuf_free(dxp->cookie);
+		dxp->cookie = txm;
+
+		desc[idx].addr  = txvq->virtio_net_hdr_mem +
+				  RTE_PTR_DIFF(&txr[idx].tx_hdr, txr);
+		desc[idx].len   = vq->hw->vtnet_hdr_size;
+		desc[idx].flags |= VRING_DESC_F_NEXT;
+
+		do {
+			idx = (vq->vq_avail_idx++) & (vq->vq_nentries - 1);
+			desc[idx].addr  = VIRTIO_MBUF_DATA_DMA_ADDR(txm, vq);
+			desc[idx].len   = txm->data_len;
+			desc[idx].flags |= VRING_DESC_F_NEXT;
+			if (idx == (vq->vq_nentries - 1))
+				toggle_wrap_counter(&vq->vq_ring);
+		} while ((txm = txm->next) != NULL);
+
+		desc[idx].flags &= ~VRING_DESC_F_NEXT;
+
+		rte_smp_wmb();
+		_set_desc_avail(&desc[head_idx], wrap_counter);
+	}
+
+	txvq->stats.packets += i;
+	txvq->stats.errors  += nb_pkts - i;
+
+	return i;
+}
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 07/17] net/virtio: implement transmit path for packed queues

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:10PM +0100, Jens Freimann wrote:
[...]
> diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
> index 6c2c996..aa1e600 100644
> --- a/drivers/net/virtio/Makefile
> +++ b/drivers/net/virtio/Makefile
> @@ -28,6 +28,7 @@ LIBABIVER := 1
>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtqueue.c
>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_pci.c
>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx.c
> +SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_1.1.c

There is no need to introduce this file just for Tx.

>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
[...]
> @@ -603,7 +605,8 @@ struct rte_virtio_xstats_name_off {
>  	}
>  
>  	vtpci_reset(hw);
> -	virtio_dev_free_mbufs(dev);
> +	if (!vtpci_packed_queue(hw))
> +		virtio_dev_free_mbufs(dev);

I think we also need to free mbufs for packed ring.

>  	virtio_free_queues(hw);
>  }
[...]
> +/* Cleanup from completed transmits. */
> +static void
> +virtio_xmit_cleanup(struct virtqueue *vq)
> +{
> +	uint16_t idx;
> +	uint16_t size = vq->vq_nentries;
> +	struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
> +
> +	idx = vq->vq_used_cons_idx & (size - 1);
> +	while (desc_is_used(&desc[idx]) &&
> +	       vq->vq_free_cnt < size) {
> +		while (desc[idx].flags & VRING_DESC_F_NEXT) {

We can't use VRING_DESC_F_NEXT when handling used
descriptors.

Thanks

Re: [dpdk-dev] [PATCH 07/17] net/virtio: implement transmit path for packed queues

[Jens Freimann]

On Mon, Mar 19, 2018 at 05:04:43PM +0800, Tiwei Bie wrote:
>On Fri, Mar 16, 2018 at 04:21:10PM +0100, Jens Freimann wrote:
>[...]
>> diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
>> index 6c2c996..aa1e600 100644
>> --- a/drivers/net/virtio/Makefile
>> +++ b/drivers/net/virtio/Makefile
>> @@ -28,6 +28,7 @@ LIBABIVER := 1
>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtqueue.c
>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_pci.c
>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx.c
>> +SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_1.1.c
>
>There is no need to introduce this file just for Tx.

I agree, this is a leftover from the prototype. I'll merge it into
virtio_rxtx.c
>
>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
>>  SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
>[...]
>> @@ -603,7 +605,8 @@ struct rte_virtio_xstats_name_off {
>>  	}
>>
>>  	vtpci_reset(hw);
>> -	virtio_dev_free_mbufs(dev);
>> +	if (!vtpci_packed_queue(hw))
>> +		virtio_dev_free_mbufs(dev);
>
>I think we also need to free mbufs for packed ring.

yes, will fix

>
>>  	virtio_free_queues(hw);
>>  }
>[...]
>> +/* Cleanup from completed transmits. */
>> +static void
>> +virtio_xmit_cleanup(struct virtqueue *vq)
>> +{
>> +	uint16_t idx;
>> +	uint16_t size = vq->vq_nentries;
>> +	struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
>> +
>> +	idx = vq->vq_used_cons_idx & (size - 1);
>> +	while (desc_is_used(&desc[idx]) &&
>> +	       vq->vq_free_cnt < size) {
>> +		while (desc[idx].flags & VRING_DESC_F_NEXT) {
>
>We can't use VRING_DESC_F_NEXT when handling used
>descriptors.

Already fixes this but must have been lost in a rebase. Will send it
in v3. 

thanks for the review!

regards,
Jens
>
>Thanks

Re: [dpdk-dev] [PATCH 07/17] net/virtio: implement transmit path for packed queues

[Jason Wang]

On 2018年03月16日 23:21, Jens Freimann wrote:
> This implements the transmit path for devices with
> support for Virtio 1.1.
>
> Add the feature bit for Virtio 1.1 and enable code to
> add buffers to vring and mark descriptors as available.
>
> This is based on a patch by Yuanhan Liu.
>
> Signed-off-by: Jens Freiman <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/Makefile          |   1 +
>   drivers/net/virtio/virtio_ethdev.c   |  11 ++-
>   drivers/net/virtio/virtio_ethdev.h   |   3 +
>   drivers/net/virtio/virtio_rxtx.c     |   7 +-
>   drivers/net/virtio/virtio_rxtx_1.1.c | 161 +++++++++++++++++++++++++++++++++++
>   5 files changed, 180 insertions(+), 3 deletions(-)
>   create mode 100644 drivers/net/virtio/virtio_rxtx_1.1.c
>
> diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
> index 6c2c996..aa1e600 100644
> --- a/drivers/net/virtio/Makefile
> +++ b/drivers/net/virtio/Makefile
> @@ -28,6 +28,7 @@ LIBABIVER := 1
>   SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtqueue.c
>   SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_pci.c
>   SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx.c
> +SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_1.1.c
>   SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
>   SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
>   
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 19536eb..722a2cd 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -383,6 +383,8 @@ struct rte_virtio_xstats_name_off {
>   	vq->hw = hw;
>   	vq->vq_queue_index = vtpci_queue_idx;
>   	vq->vq_nentries = vq_size;
> +	if (vtpci_packed_queue(hw))
> +		vq->vq_ring.avail_wrap_counter = 1;
>   
>   	/*
>   	 * Reserve a memzone for vring elements
> @@ -603,7 +605,8 @@ struct rte_virtio_xstats_name_off {
>   	}
>   
>   	vtpci_reset(hw);
> -	virtio_dev_free_mbufs(dev);
> +	if (!vtpci_packed_queue(hw))
> +		virtio_dev_free_mbufs(dev);
>   	virtio_free_queues(hw);
>   }
>   
> @@ -1360,7 +1363,11 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
>   		eth_dev->rx_pkt_burst = &virtio_recv_pkts;
>   	}
>   
> -	if (hw->use_simple_tx) {
> +	if (vtpci_packed_queue(hw)) {
> +		PMD_INIT_LOG(INFO, "virtio: using virtio 1.1 Tx path on port %u",
> +			eth_dev->data->port_id);
> +		eth_dev->tx_pkt_burst = virtio_xmit_pkts_packed;
> +	} else if (hw->use_simple_tx) {
>   		PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
>   			eth_dev->data->port_id);
>   		eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index 4539d2e..cfefe4d 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -36,6 +36,7 @@
>   	 1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE |	\
>   	 1u << VIRTIO_RING_F_INDIRECT_DESC |    \
>   	 1ULL << VIRTIO_F_VERSION_1       |	\
> +	 1ULL << VIRTIO_F_RING_PACKED     |	\
>   	 1ULL << VIRTIO_F_IOMMU_PLATFORM)
>   
>   #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES	\
> @@ -77,6 +78,8 @@ uint16_t virtio_recv_mergeable_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>   
>   uint16_t virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>   		uint16_t nb_pkts);
> +uint16_t virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
> +		uint16_t nb_pkts);
>   
>   uint16_t virtio_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
>   		uint16_t nb_pkts);
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 8dbf2a3..f1df004 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -545,6 +545,10 @@
>   
>   	PMD_INIT_FUNC_TRACE();
>   
> +	if (vtpci_packed_queue(hw)) {
> +		vq->vq_ring.avail_wrap_counter = 1;
> +	}
> +
>   	if (hw->use_simple_tx) {
>   		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
>   			vq->vq_ring.avail->ring[desc_idx] =
> @@ -565,7 +569,8 @@
>   			vq->vq_ring.avail->ring[desc_idx] = desc_idx;
>   	}
>   
> -	VIRTQUEUE_DUMP(vq);
> +	if (!vtpci_packed_queue(hw))
> +		VIRTQUEUE_DUMP(vq);
>   
>   	return 0;
>   }
> diff --git a/drivers/net/virtio/virtio_rxtx_1.1.c b/drivers/net/virtio/virtio_rxtx_1.1.c
> new file mode 100644
> index 0000000..05ec085
> --- /dev/null
> +++ b/drivers/net/virtio/virtio_rxtx_1.1.c
> @@ -0,0 +1,161 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
> + *   All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of Intel Corporation nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <rte_cycles.h>
> +#include <rte_memory.h>
> +#include <rte_memzone.h>
> +#include <rte_branch_prediction.h>
> +#include <rte_mempool.h>
> +#include <rte_malloc.h>
> +#include <rte_mbuf.h>
> +#include <rte_ether.h>
> +#include <rte_ethdev.h>
> +#include <rte_prefetch.h>
> +#include <rte_string_fns.h>
> +#include <rte_errno.h>
> +#include <rte_byteorder.h>
> +#include <rte_cpuflags.h>
> +#include <rte_net.h>
> +#include <rte_ip.h>
> +#include <rte_udp.h>
> +#include <rte_tcp.h>
> +
> +#include "virtio_logs.h"
> +#include "virtio_ethdev.h"
> +#include "virtio_pci.h"
> +#include "virtqueue.h"
> +#include "virtio_rxtx.h"
> +#include "virtio_ring.h"
> +
> +/* Cleanup from completed transmits. */
> +static void
> +virtio_xmit_cleanup(struct virtqueue *vq)
> +{
> +	uint16_t idx;
> +	uint16_t size = vq->vq_nentries;
> +	struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
> +
> +	idx = vq->vq_used_cons_idx & (size - 1);
> +	while (desc_is_used(&desc[idx]) &&
> +	       vq->vq_free_cnt < size) {
> +		while (desc[idx].flags & VRING_DESC_F_NEXT) {
> +			vq->vq_free_cnt++;
> +			idx = ++vq->vq_used_cons_idx & (size - 1);
> +		}
> +		vq->vq_free_cnt++;
> +		idx = ++vq->vq_used_cons_idx & (size - 1);
> +	}
> +}
> +
> +uint16_t
> +virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
> +		     uint16_t nb_pkts)
> +{
> +	struct virtnet_tx *txvq = tx_queue;
> +	struct virtqueue *vq = txvq->vq;
> +	uint16_t i;
> +	struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
> +	uint16_t idx;
> +	struct vq_desc_extra *dxp;
> +
> +	if (unlikely(nb_pkts < 1))
> +		return nb_pkts;
> +
> +	PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
> +
> +	if (likely(vq->vq_free_cnt < vq->vq_free_thresh))
> +		virtio_xmit_cleanup(vq);
> +
> +	for (i = 0; i < nb_pkts; i++) {
> +		struct rte_mbuf *txm = tx_pkts[i];
> +		struct virtio_tx_region *txr = txvq->virtio_net_hdr_mz->addr;
> +		uint16_t head_idx;
> +		int wrap_counter;
> +
> +		if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
> +			virtio_xmit_cleanup(vq);
> +
> +			if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
> +				PMD_TX_LOG(ERR,
> +					   "No free tx descriptors to transmit");
> +				break;
> +			}
> +		}
> +
> +		txvq->stats.bytes += txm->pkt_len;
> +
> +		vq->vq_free_cnt -= txm->nb_segs + 1;
> +
> +		idx = (vq->vq_avail_idx++) & (vq->vq_nentries - 1);
> +		head_idx = idx;
> +		wrap_counter = vq->vq_ring.avail_wrap_counter;
> +
> +		if ((vq->vq_avail_idx & (vq->vq_nentries - 1)) == 0)
> +			toggle_wrap_counter(&vq->vq_ring);
> +
> +		dxp = &vq->vq_descx[idx];
> +		if (dxp->cookie != NULL)
> +			rte_pktmbuf_free(dxp->cookie);
> +		dxp->cookie = txm;
> +
> +		desc[idx].addr  = txvq->virtio_net_hdr_mem +
> +				  RTE_PTR_DIFF(&txr[idx].tx_hdr, txr);
> +		desc[idx].len   = vq->hw->vtnet_hdr_size;
> +		desc[idx].flags |= VRING_DESC_F_NEXT;
> +
> +		do {
> +			idx = (vq->vq_avail_idx++) & (vq->vq_nentries - 1);
> +			desc[idx].addr  = VIRTIO_MBUF_DATA_DMA_ADDR(txm, vq);
> +			desc[idx].len   = txm->data_len;
> +			desc[idx].flags |= VRING_DESC_F_NEXT;
> +			if (idx == (vq->vq_nentries - 1))
> +				toggle_wrap_counter(&vq->vq_ring);
> +		} while ((txm = txm->next) != NULL);
> +
> +		desc[idx].flags &= ~VRING_DESC_F_NEXT;
> +
> +		rte_smp_wmb();
> +		_set_desc_avail(&desc[head_idx], wrap_counter);
> +	}

Need to kick host here.

Thanks

> +
> +	txvq->stats.packets += i;
> +	txvq->stats.errors  += nb_pkts - i;
> +
> +	return i;
> +}

[dpdk-dev] [PATCH 08/17] net/virtio: implement receive path for packed queues

[Jens Freimann]

From: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Implement the receive part here. No support for mergeable buffers yet.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_ethdev.c |   5 +-
 drivers/net/virtio/virtio_ethdev.h |   2 +
 drivers/net/virtio/virtio_rxtx.c   | 134 +++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 722a2cd..888cc49 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1352,6 +1352,8 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 		PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
 			eth_dev->data->port_id);
 		eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
+	} else if (vtpci_packed_queue(hw)) {
+		eth_dev->rx_pkt_burst = &virtio_recv_pkts_packed;
 	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
 		PMD_INIT_LOG(INFO,
 			"virtio: using mergeable buffer Rx path on port %u",
@@ -1507,7 +1509,8 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 
 	/* Setting up rx_header size for the device */
 	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF) ||
-	    vtpci_with_feature(hw, VIRTIO_F_VERSION_1))
+	    vtpci_with_feature(hw, VIRTIO_F_VERSION_1) ||
+	    vtpci_with_feature(hw, VIRTIO_F_RING_PACKED))
 		hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	else
 		hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr);
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index cfefe4d..92c1c4f 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -72,6 +72,8 @@ int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
 
 uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		uint16_t nb_pkts);
+uint16_t virtio_recv_pkts_packed(void *rx_queue, struct rte_mbuf **rx_pkts,
+		uint16_t nb_pkts);
 
 uint16_t virtio_recv_mergeable_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		uint16_t nb_pkts);
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index f1df004..7834747 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -31,6 +31,7 @@
 #include "virtqueue.h"
 #include "virtio_rxtx.h"
 #include "virtio_rxtx_simple.h"
+#include "virtio_ring.h"
 
 #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DUMP
 #define VIRTIO_DUMP_PACKET(m, len) rte_pktmbuf_dump(stdout, m, len)
@@ -428,6 +429,34 @@
 
 	PMD_INIT_FUNC_TRACE();
 
+	if (vtpci_packed_queue(hw)) {
+		struct vring_desc_packed *desc;
+		struct vq_desc_extra *dxp;
+
+		for (desc_idx = 0; desc_idx < vq->vq_nentries;
+				desc_idx++) {
+			m = rte_mbuf_raw_alloc(rxvq->mpool);
+			if (unlikely(m == NULL))
+				return -ENOMEM;
+
+			dxp = &vq->vq_descx[desc_idx];
+			dxp->cookie = m;
+			dxp->ndescs = 1;
+
+			desc = &vq->vq_ring.desc_packed[desc_idx];
+			desc->addr = VIRTIO_MBUF_ADDR(m, vq) +
+				RTE_PKTMBUF_HEADROOM - hw->vtnet_hdr_size;
+			desc->len = m->buf_len - RTE_PKTMBUF_HEADROOM +
+				hw->vtnet_hdr_size;
+			desc->flags |= VRING_DESC_F_WRITE;
+			rte_smp_wmb();
+			set_desc_avail(&vq->vq_ring, desc);
+		}
+		toggle_wrap_counter(&vq->vq_ring);
+
+		return 0;
+	}
+
 	/* Allocate blank mbufs for the each rx descriptor */
 	nbufs = 0;
 
@@ -702,6 +731,111 @@
 		vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6);
 }
 
+uint16_t
+virtio_recv_pkts_packed(void *rx_queue, struct rte_mbuf **rx_pkts,
+		     uint16_t nb_pkts)
+{
+	struct virtnet_rx *rxvq = rx_queue;
+	struct virtqueue *vq = rxvq->vq;
+	struct virtio_hw *hw = vq->hw;
+	struct rte_mbuf *rxm, *nmb;
+	uint16_t nb_rx;
+	uint32_t len;
+	uint32_t i;
+	uint32_t hdr_size;
+	int offload;
+	struct virtio_net_hdr *hdr;
+	struct vring_desc_packed *descs = vq->vq_ring.desc_packed;
+	struct vring_desc_packed *desc;
+	uint16_t used_idx = vq->vq_used_cons_idx;
+	struct vq_desc_extra *dxp;
+
+	nb_rx = 0;
+	if (unlikely(hw->started == 0))
+		return nb_rx;
+
+	hdr_size = hw->vtnet_hdr_size;
+	offload = rx_offload_enabled(hw);
+
+	for (i = 0; i < nb_pkts; i++) {
+		desc = &descs[used_idx & (vq->vq_nentries - 1)];
+		if (!desc_is_used(desc))
+			break;
+
+		rte_smp_rmb();
+
+		nmb = rte_mbuf_raw_alloc(rxvq->mpool);
+		if (unlikely(nmb == NULL)) {
+			struct rte_eth_dev *dev
+				= &rte_eth_devices[rxvq->port_id];
+			dev->data->rx_mbuf_alloc_failed++;
+			break;
+		}
+
+		dxp = &vq->vq_descx[used_idx & (vq->vq_nentries - 1)];
+
+		len = desc->len;
+		rxm = dxp->cookie;
+		dxp->cookie = nmb;
+		dxp->ndescs = 1;
+
+		desc->addr = VIRTIO_MBUF_ADDR(nmb, vq) +
+			RTE_PKTMBUF_HEADROOM - hw->vtnet_hdr_size;
+		desc->len = nmb->buf_len - RTE_PKTMBUF_HEADROOM +
+			hw->vtnet_hdr_size;
+		desc->flags |= VRING_DESC_F_WRITE;
+
+		PMD_RX_LOG(DEBUG, "packet len:%d", len);
+
+		if (unlikely(len < hdr_size + ETHER_HDR_LEN)) {
+			PMD_RX_LOG(ERR, "Packet drop");
+			rte_pktmbuf_free(rxm);
+			rxvq->stats.errors++;
+			continue;
+		}
+
+		rxm->port = rxvq->port_id;
+		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->ol_flags = 0;
+		rxm->vlan_tci = 0;
+
+		rxm->pkt_len = (uint32_t)(len - hdr_size);
+		rxm->data_len = (uint16_t)(len - hdr_size);
+
+		hdr = (struct virtio_net_hdr *)((char *)rxm->buf_addr +
+			RTE_PKTMBUF_HEADROOM - hdr_size);
+
+		if (hw->vlan_strip)
+			rte_vlan_strip(rxm);
+
+		if (offload && virtio_rx_offload(rxm, hdr) < 0) {
+			rte_pktmbuf_free(rxm);
+			rxvq->stats.errors++;
+			continue;
+		}
+
+		VIRTIO_DUMP_PACKET(rxm, rxm->data_len);
+
+		rxvq->stats.bytes += rxm->pkt_len;
+		virtio_update_packet_stats(&rxvq->stats, rxm);
+
+		rte_smp_wmb();
+		set_desc_avail(&vq->vq_ring, desc);
+
+		rx_pkts[nb_rx++] = rxm;
+
+		used_idx++;
+		if ((used_idx & (vq->vq_nentries - 1)) == 0)
+			toggle_wrap_counter(&vq->vq_ring);
+	}
+
+	rxvq->stats.packets += nb_rx;
+
+	vq->vq_used_cons_idx = used_idx;
+
+	return nb_rx;
+}
+
 #define VIRTIO_MBUF_BURST_SZ 64
 #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
 uint16_t
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 08/17] net/virtio: implement receive path for packed queues

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:11PM +0100, Jens Freimann wrote:
[...]
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 722a2cd..888cc49 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1352,6 +1352,8 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
>  		PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
>  			eth_dev->data->port_id);
>  		eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> +	} else if (vtpci_packed_queue(hw)) {
> +		eth_dev->rx_pkt_burst = &virtio_recv_pkts_packed;

If MRG_RXBUF isn't supported at this point, then we will
need to make sure that RING_PACKED and MRG_RXBUF won't be
negotiated at the same time. Otherwise this commit will
break the driver.

>  	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
>  		PMD_INIT_LOG(INFO,
>  			"virtio: using mergeable buffer Rx path on port %u",
> @@ -1507,7 +1509,8 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
>  
>  	/* Setting up rx_header size for the device */
>  	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF) ||
> -	    vtpci_with_feature(hw, VIRTIO_F_VERSION_1))
> +	    vtpci_with_feature(hw, VIRTIO_F_VERSION_1) ||
> +	    vtpci_with_feature(hw, VIRTIO_F_RING_PACKED))
>  		hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>  	else
>  		hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr);
[...]
>  #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DUMP
>  #define VIRTIO_DUMP_PACKET(m, len) rte_pktmbuf_dump(stdout, m, len)
> @@ -428,6 +429,34 @@
>  
>  	PMD_INIT_FUNC_TRACE();
>  
> +	if (vtpci_packed_queue(hw)) {
> +		struct vring_desc_packed *desc;
> +		struct vq_desc_extra *dxp;
> +
> +		for (desc_idx = 0; desc_idx < vq->vq_nentries;
> +				desc_idx++) {
> +			m = rte_mbuf_raw_alloc(rxvq->mpool);
> +			if (unlikely(m == NULL))
> +				return -ENOMEM;
> +
> +			dxp = &vq->vq_descx[desc_idx];
> +			dxp->cookie = m;
> +			dxp->ndescs = 1;
> +
> +			desc = &vq->vq_ring.desc_packed[desc_idx];
> +			desc->addr = VIRTIO_MBUF_ADDR(m, vq) +
> +				RTE_PKTMBUF_HEADROOM - hw->vtnet_hdr_size;
> +			desc->len = m->buf_len - RTE_PKTMBUF_HEADROOM +
> +				hw->vtnet_hdr_size;
> +			desc->flags |= VRING_DESC_F_WRITE;
> +			rte_smp_wmb();
> +			set_desc_avail(&vq->vq_ring, desc);
> +		}
> +		toggle_wrap_counter(&vq->vq_ring);
> +
> +		return 0;

Maybe it's better to give the debug code (which is at the
end of this function) a chance to run.

Thanks

Re: [dpdk-dev] [PATCH 08/17] net/virtio: implement receive path for packed queues

[Jason Wang]

On 2018年03月16日 23:21, Jens Freimann wrote:
> From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>
> Implement the receive part here. No support for mergeable buffers yet.
>
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c |   5 +-
>   drivers/net/virtio/virtio_ethdev.h |   2 +
>   drivers/net/virtio/virtio_rxtx.c   | 134 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 140 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 722a2cd..888cc49 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1352,6 +1352,8 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
>   		PMD_INIT_LOG(INFO, "virtio: using simple Rx path on port %u",
>   			eth_dev->data->port_id);
>   		eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
> +	} else if (vtpci_packed_queue(hw)) {
> +		eth_dev->rx_pkt_burst = &virtio_recv_pkts_packed;
>   	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
>   		PMD_INIT_LOG(INFO,
>   			"virtio: using mergeable buffer Rx path on port %u",
> @@ -1507,7 +1509,8 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
>   
>   	/* Setting up rx_header size for the device */
>   	if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF) ||
> -	    vtpci_with_feature(hw, VIRTIO_F_VERSION_1))
> +	    vtpci_with_feature(hw, VIRTIO_F_VERSION_1) ||
> +	    vtpci_with_feature(hw, VIRTIO_F_RING_PACKED))
>   		hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>   	else
>   		hw->vtnet_hdr_size = sizeof(struct virtio_net_hdr);
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index cfefe4d..92c1c4f 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -72,6 +72,8 @@ int virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>   
>   uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>   		uint16_t nb_pkts);
> +uint16_t virtio_recv_pkts_packed(void *rx_queue, struct rte_mbuf **rx_pkts,
> +		uint16_t nb_pkts);
>   
>   uint16_t virtio_recv_mergeable_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>   		uint16_t nb_pkts);
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index f1df004..7834747 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -31,6 +31,7 @@
>   #include "virtqueue.h"
>   #include "virtio_rxtx.h"
>   #include "virtio_rxtx_simple.h"
> +#include "virtio_ring.h"
>   
>   #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DUMP
>   #define VIRTIO_DUMP_PACKET(m, len) rte_pktmbuf_dump(stdout, m, len)
> @@ -428,6 +429,34 @@
>   
>   	PMD_INIT_FUNC_TRACE();
>   
> +	if (vtpci_packed_queue(hw)) {
> +		struct vring_desc_packed *desc;
> +		struct vq_desc_extra *dxp;
> +
> +		for (desc_idx = 0; desc_idx < vq->vq_nentries;
> +				desc_idx++) {
> +			m = rte_mbuf_raw_alloc(rxvq->mpool);
> +			if (unlikely(m == NULL))
> +				return -ENOMEM;
> +
> +			dxp = &vq->vq_descx[desc_idx];
> +			dxp->cookie = m;
> +			dxp->ndescs = 1;
> +
> +			desc = &vq->vq_ring.desc_packed[desc_idx];
> +			desc->addr = VIRTIO_MBUF_ADDR(m, vq) +
> +				RTE_PKTMBUF_HEADROOM - hw->vtnet_hdr_size;
> +			desc->len = m->buf_len - RTE_PKTMBUF_HEADROOM +
> +				hw->vtnet_hdr_size;
> +			desc->flags |= VRING_DESC_F_WRITE;
> +			rte_smp_wmb();
> +			set_desc_avail(&vq->vq_ring, desc);
> +		}
> +		toggle_wrap_counter(&vq->vq_ring);
> +
> +		return 0;
> +	}
> +
>   	/* Allocate blank mbufs for the each rx descriptor */
>   	nbufs = 0;
>   
> @@ -702,6 +731,111 @@
>   		vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6);
>   }
>   
> +uint16_t
> +virtio_recv_pkts_packed(void *rx_queue, struct rte_mbuf **rx_pkts,
> +		     uint16_t nb_pkts)
> +{
> +	struct virtnet_rx *rxvq = rx_queue;
> +	struct virtqueue *vq = rxvq->vq;
> +	struct virtio_hw *hw = vq->hw;
> +	struct rte_mbuf *rxm, *nmb;
> +	uint16_t nb_rx;
> +	uint32_t len;
> +	uint32_t i;
> +	uint32_t hdr_size;
> +	int offload;
> +	struct virtio_net_hdr *hdr;
> +	struct vring_desc_packed *descs = vq->vq_ring.desc_packed;
> +	struct vring_desc_packed *desc;
> +	uint16_t used_idx = vq->vq_used_cons_idx;
> +	struct vq_desc_extra *dxp;
> +
> +	nb_rx = 0;
> +	if (unlikely(hw->started == 0))
> +		return nb_rx;
> +
> +	hdr_size = hw->vtnet_hdr_size;
> +	offload = rx_offload_enabled(hw);
> +
> +	for (i = 0; i < nb_pkts; i++) {
> +		desc = &descs[used_idx & (vq->vq_nentries - 1)];
> +		if (!desc_is_used(desc))
> +			break;
> +
> +		rte_smp_rmb();
> +
> +		nmb = rte_mbuf_raw_alloc(rxvq->mpool);
> +		if (unlikely(nmb == NULL)) {
> +			struct rte_eth_dev *dev
> +				= &rte_eth_devices[rxvq->port_id];
> +			dev->data->rx_mbuf_alloc_failed++;
> +			break;
> +		}
> +
> +		dxp = &vq->vq_descx[used_idx & (vq->vq_nentries - 1)];
> +
> +		len = desc->len;
> +		rxm = dxp->cookie;
> +		dxp->cookie = nmb;
> +		dxp->ndescs = 1;
> +
> +		desc->addr = VIRTIO_MBUF_ADDR(nmb, vq) +
> +			RTE_PKTMBUF_HEADROOM - hw->vtnet_hdr_size;
> +		desc->len = nmb->buf_len - RTE_PKTMBUF_HEADROOM +
> +			hw->vtnet_hdr_size;
> +		desc->flags |= VRING_DESC_F_WRITE;
> +
> +		PMD_RX_LOG(DEBUG, "packet len:%d", len);
> +
> +		if (unlikely(len < hdr_size + ETHER_HDR_LEN)) {
> +			PMD_RX_LOG(ERR, "Packet drop");
> +			rte_pktmbuf_free(rxm);
> +			rxvq->stats.errors++;
> +			continue;
> +		}
> +
> +		rxm->port = rxvq->port_id;
> +		rxm->data_off = RTE_PKTMBUF_HEADROOM;
> +		rxm->ol_flags = 0;
> +		rxm->vlan_tci = 0;
> +
> +		rxm->pkt_len = (uint32_t)(len - hdr_size);
> +		rxm->data_len = (uint16_t)(len - hdr_size);
> +
> +		hdr = (struct virtio_net_hdr *)((char *)rxm->buf_addr +
> +			RTE_PKTMBUF_HEADROOM - hdr_size);
> +
> +		if (hw->vlan_strip)
> +			rte_vlan_strip(rxm);
> +
> +		if (offload && virtio_rx_offload(rxm, hdr) < 0) {
> +			rte_pktmbuf_free(rxm);
> +			rxvq->stats.errors++;
> +			continue;
> +		}
> +
> +		VIRTIO_DUMP_PACKET(rxm, rxm->data_len);
> +
> +		rxvq->stats.bytes += rxm->pkt_len;
> +		virtio_update_packet_stats(&rxvq->stats, rxm);
> +
> +		rte_smp_wmb();
> +		set_desc_avail(&vq->vq_ring, desc);
> +
> +		rx_pkts[nb_rx++] = rxm;
> +
> +		used_idx++;
> +		if ((used_idx & (vq->vq_nentries - 1)) == 0)
> +			toggle_wrap_counter(&vq->vq_ring);
> +	}
> +
> +	rxvq->stats.packets += nb_rx;

I believe we need to kick virtqueue here since we could not assume the 
backend (vhost-net) is always polling for the virtqueue.

Thanks

> +
> +	vq->vq_used_cons_idx = used_idx;
> +
> +	return nb_rx;
> +}
> +
>   #define VIRTIO_MBUF_BURST_SZ 64
>   #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
>   uint16_t

[dpdk-dev] [PATCH 09/17] vhost: add virtio 1.1 defines

[Jens Freimann]

This should actually be in the kernel header file, but it isn't
yet. For now let's use our own headers.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 lib/librte_vhost/vhost.h      |  4 +++-
 lib/librte_vhost/virtio-1.1.h | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_vhost/virtio-1.1.h

diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index d947bc9..06e973e 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -173,7 +173,9 @@ struct vhost_msg {
 #ifndef VIRTIO_F_VERSION_1
  #define VIRTIO_F_VERSION_1 32
 #endif
-
+#ifndef VIRTIO_F_RING_PACKED
+ #define VIRTIO_F_RING_PACKED 34
+#endif
 #define VHOST_USER_F_PROTOCOL_FEATURES	30
 
 /* Features supported by this builtin vhost-user net driver. */
diff --git a/lib/librte_vhost/virtio-1.1.h b/lib/librte_vhost/virtio-1.1.h
new file mode 100644
index 0000000..b37d621
--- /dev/null
+++ b/lib/librte_vhost/virtio-1.1.h
@@ -0,0 +1,20 @@
+#ifndef __VIRTIO_PACKED_H
+#define __VIRTIO_PACKED_H
+
+#define VRING_DESC_F_NEXT       1
+#define VRING_DESC_F_WRITE      2
+#define VRING_DESC_F_INDIRECT   4
+
+#define VIRTQ_DESC_F_AVAIL      7
+#define VIRTQ_DESC_F_USED      15
+#define DESC_USED               (1ULL << VIRTQ_DESC_F_USED)
+#define DESC_AVAIL              (1ULL << VIRTQ_DESC_F_AVAIL)
+
+struct vring_desc_packed {
+	uint64_t addr;
+	uint32_t len;
+	uint16_t index;
+	uint16_t flags;
+};
+
+#endif /* __VIRTIO_PACKED_H */
-- 
1.8.3.1

[dpdk-dev] [PATCH 10/17] vhost: vring address setup for packed queues

[Jens Freimann]

From: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Add code to set up packed queues when enabled.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Signed-off-by: Jens Freimann <jfreiman@redhat.com>
---
 lib/librte_vhost/vhost.c      |  1 +
 lib/librte_vhost/vhost.h      |  1 +
 lib/librte_vhost/vhost_user.c | 17 ++++++++++++++++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index a407067..a300812 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -567,6 +567,7 @@ struct virtio_net *
 		return -1;
 	}
 
+
 	dev->virtqueue[queue_id]->used->flags = VRING_USED_F_NO_NOTIFY;
 	return 0;
 }
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 06e973e..d35c4b1 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -70,6 +70,7 @@ struct batch_copy_elem {
  */
 struct vhost_virtqueue {
 	struct vring_desc	*desc;
+	struct vring_desc_packed   *desc_packed;
 	struct vring_avail	*avail;
 	struct vring_used	*used;
 	uint32_t		size;
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 90ed211..bd1e393 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -415,6 +415,19 @@
 	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
 	struct vhost_vring_addr *addr = &vq->ring_addrs;
 
+	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED)) {
+		vq->desc_packed = (struct vring_desc_packed *) ring_addr_to_vva
+			(dev, vq, addr->desc_user_addr, sizeof(vq->desc_packed));
+		vq->desc = NULL;
+		vq->avail = NULL;
+		vq->used = NULL;
+		vq->log_guest_addr = 0;
+
+		assert(vq->last_used_idx == 0);
+
+		return dev;
+	}
+
 	/* The addresses are converted from QEMU virtual to Vhost virtual. */
 	if (vq->desc && vq->avail && vq->used)
 		return dev;
@@ -427,6 +440,7 @@
 			dev->vid);
 		return dev;
 	}
+	vq->desc_packed = NULL;
 
 	dev = numa_realloc(dev, vq_index);
 	vq = dev->virtqueue[vq_index];
@@ -764,7 +778,8 @@
 static int
 vq_is_ready(struct vhost_virtqueue *vq)
 {
-	return vq && vq->desc && vq->avail && vq->used &&
+	return vq &&
+	       (vq->desc_packed || (vq->desc && vq->avail && vq->used)) &&
 	       vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
 	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
 }
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 10/17] vhost: vring address setup for packed queues

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:13PM +0100, Jens Freimann wrote:
> From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> 
> Add code to set up packed queues when enabled.
> 
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Signed-off-by: Jens Freimann <jfreiman@redhat.com>
> ---
>  lib/librte_vhost/vhost.c      |  1 +
>  lib/librte_vhost/vhost.h      |  1 +
>  lib/librte_vhost/vhost_user.c | 17 ++++++++++++++++-
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index a407067..a300812 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -567,6 +567,7 @@ struct virtio_net *
>  		return -1;
>  	}
>  
> +

There is no need to add this blank line.

>  	dev->virtqueue[queue_id]->used->flags = VRING_USED_F_NO_NOTIFY;
>  	return 0;
>  }
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index 06e973e..d35c4b1 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -70,6 +70,7 @@ struct batch_copy_elem {
>   */
>  struct vhost_virtqueue {
>  	struct vring_desc	*desc;
> +	struct vring_desc_packed   *desc_packed;
>  	struct vring_avail	*avail;
>  	struct vring_used	*used;

It's better to use union here.

> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 90ed211..bd1e393 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -415,6 +415,19 @@
>  	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
>  	struct vhost_vring_addr *addr = &vq->ring_addrs;
>  
> +	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED)) {
> +		vq->desc_packed = (struct vring_desc_packed *) ring_addr_to_vva
> +			(dev, vq, addr->desc_user_addr, sizeof(vq->desc_packed));
> +		vq->desc = NULL;
> +		vq->avail = NULL;
> +		vq->used = NULL;
> +		vq->log_guest_addr = 0;
> +
> +		assert(vq->last_used_idx == 0);

As a library, we shouldn't crash the process.

Thanks

[dpdk-dev] [PATCH 11/17] vhost: add helpers for packed virtqueues

[Jens Freimann]

Add some helper functions to set/check descriptor flags
and toggle the used wrap counter.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 lib/librte_vhost/virtio-1.1.h | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/librte_vhost/virtio-1.1.h b/lib/librte_vhost/virtio-1.1.h
index b37d621..7b37d26 100644
--- a/lib/librte_vhost/virtio-1.1.h
+++ b/lib/librte_vhost/virtio-1.1.h
@@ -17,4 +17,47 @@ struct vring_desc_packed {
 	uint16_t flags;
 };
 
+static inline void
+toggle_wrap_counter(struct vhost_virtqueue *vq)
+{
+	vq->used_wrap_counter ^= 1;
+}
+
+static inline int
+desc_is_avail(struct vhost_virtqueue *vq, struct vring_desc_packed *desc)
+{
+	if (unlikely(!vq))
+		return -1;
+
+	if (vq->used_wrap_counter == 1)
+		if ((desc->flags & DESC_AVAIL) && !(desc->flags & DESC_USED))
+			return 1;
+	if (vq->used_wrap_counter == 0)
+		if (!(desc->flags & DESC_AVAIL) && (desc->flags & DESC_USED))
+			return 1;
+	return 0;
+}
+
+static inline void
+_set_desc_used(struct vring_desc_packed *desc, int wrap_counter)
+{
+	uint16_t flags = desc->flags;
+
+	if (wrap_counter == 1) {
+		flags |= DESC_USED;
+		flags |= DESC_AVAIL;
+	} else {
+		flags &= ~DESC_USED;
+		flags &= ~DESC_AVAIL;
+	}
+
+	desc->flags = flags;
+}
+
+static inline void
+set_desc_used(struct vhost_virtqueue *vq, struct vring_desc_packed *desc)
+{
+	_set_desc_used(desc, vq->used_wrap_counter);
+}
+
 #endif /* __VIRTIO_PACKED_H */
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 11/17] vhost: add helpers for packed virtqueues

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:14PM +0100, Jens Freimann wrote:
> Add some helper functions to set/check descriptor flags
> and toggle the used wrap counter.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
[...]
>  
> +static inline void
> +toggle_wrap_counter(struct vhost_virtqueue *vq)
> +{
> +	vq->used_wrap_counter ^= 1;
> +}
> +
> +static inline int
> +desc_is_avail(struct vhost_virtqueue *vq, struct vring_desc_packed *desc)
> +{
> +	if (unlikely(!vq))
> +		return -1;

Maybe it's better to let the caller make sure the vq
won't be NULL.

> +
> +	if (vq->used_wrap_counter == 1)
> +		if ((desc->flags & DESC_AVAIL) && !(desc->flags & DESC_USED))
> +			return 1;
> +	if (vq->used_wrap_counter == 0)

Maybe it's better to use '} else {' here.

Thanks

> +		if (!(desc->flags & DESC_AVAIL) && (desc->flags & DESC_USED))
> +			return 1;
> +	return 0;
> +}

Re: [dpdk-dev] [PATCH 11/17] vhost: add helpers for packed virtqueues

[Jens Freimann]

On Mon, Mar 19, 2018 at 06:39:56PM +0800, Tiwei Bie wrote:
>On Fri, Mar 16, 2018 at 04:21:14PM +0100, Jens Freimann wrote:
>> Add some helper functions to set/check descriptor flags
>> and toggle the used wrap counter.
>>
>> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>[...]
>>
>> +static inline void
>> +toggle_wrap_counter(struct vhost_virtqueue *vq)
>> +{
>> +	vq->used_wrap_counter ^= 1;
>> +}
>> +
>> +static inline int
>> +desc_is_avail(struct vhost_virtqueue *vq, struct vring_desc_packed *desc)
>> +{
>> +	if (unlikely(!vq))
>> +		return -1;
>
>Maybe it's better to let the caller make sure the vq
>won't be NULL.
>
>> +
>> +	if (vq->used_wrap_counter == 1)
>> +		if ((desc->flags & DESC_AVAIL) && !(desc->flags & DESC_USED))
>> +			return 1;
>> +	if (vq->used_wrap_counter == 0)
>
>Maybe it's better to use '} else {' here.

Agree with both, thanks for the review!

regards,
Jens 

[dpdk-dev] [PATCH 12/17] vhost: dequeue for packed queues

[Jens Freimann]

Implement code to dequeue and process descriptors from
the vring if VIRTIO_F_PACKED is enabled.

Check if descriptor was made available by driver by looking at
VIRTIO_F_DESC_AVAIL flag in descriptor. If so dequeue and set
the used flag VIRTIO_F_DESC_USED to the current value of the
used wrap counter.

Used ring wrap counter needs to be toggled when last descriptor is
written out. This allows the host/guest to detect new descriptors even
after the ring has wrapped.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 lib/librte_vhost/vhost.c      |   1 +
 lib/librte_vhost/vhost.h      |   1 +
 lib/librte_vhost/virtio_net.c | 228 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 230 insertions(+)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index a300812..8cba10d 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -198,6 +198,7 @@ struct virtio_net *
 
 	vq->kickfd = VIRTIO_UNINITIALIZED_EVENTFD;
 	vq->callfd = VIRTIO_UNINITIALIZED_EVENTFD;
+	vq->used_wrap_counter = 1;
 
 	vhost_user_iotlb_init(dev, vring_idx);
 	/* Backends are set to -1 indicating an inactive device. */
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index d35c4b1..f77fefe 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -108,6 +108,7 @@ struct vhost_virtqueue {
 
 	struct batch_copy_elem	*batch_copy_elems;
 	uint16_t		batch_copy_nb_elems;
+	uint32_t		used_wrap_counter;
 
 	rte_rwlock_t	iotlb_lock;
 	rte_rwlock_t	iotlb_pending_lock;
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 700aca7..8f59e4f 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -19,6 +19,7 @@
 
 #include "iotlb.h"
 #include "vhost.h"
+#include "virtio-1.1.h"
 
 #define MAX_PKT_BURST 32
 
@@ -1118,6 +1119,233 @@
 	}
 }
 
+static inline uint16_t
+dequeue_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
+	     struct rte_mempool *mbuf_pool, struct rte_mbuf *m,
+	     struct vring_desc_packed *descs)
+{
+	struct vring_desc_packed *desc;
+	uint64_t desc_addr;
+	uint32_t desc_avail, desc_offset;
+	uint32_t mbuf_avail, mbuf_offset;
+	uint32_t cpy_len;
+	struct rte_mbuf *cur = m, *prev = m;
+	struct virtio_net_hdr *hdr = NULL;
+	uint16_t head_idx = vq->last_used_idx & (vq->size - 1);
+	int wrap_counter = vq->used_wrap_counter;
+	int rc = 0;
+
+	rte_spinlock_lock(&vq->access_lock);
+
+	if (unlikely(vq->enabled == 0))
+		goto out;
+
+	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
+		vhost_user_iotlb_rd_lock(vq);
+
+	desc = &descs[vq->last_used_idx & (vq->size - 1)];
+	if (unlikely((desc->len < dev->vhost_hlen)) ||
+			(desc->flags & VRING_DESC_F_INDIRECT)) {
+			RTE_LOG(ERR, VHOST_DATA,
+				"INDIRECT not supported yet\n");
+			rc = -1;
+			goto out;
+	}
+
+	desc_addr = vhost_iova_to_vva(dev, vq, desc->addr,
+				      sizeof(*desc), VHOST_ACCESS_RO);
+
+	if (unlikely(!desc_addr)) {
+		rc = -1;
+		goto out;
+	}
+
+	if (virtio_net_with_host_offload(dev)) {
+		hdr = (struct virtio_net_hdr *)((uintptr_t)desc_addr);
+		rte_prefetch0(hdr);
+	}
+
+	/*
+	 * A virtio driver normally uses at least 2 desc buffers
+	 * for Tx: the first for storing the header, and others
+	 * for storing the data.
+	 */
+	if (likely((desc->len == dev->vhost_hlen) &&
+		   (desc->flags & VRING_DESC_F_NEXT) != 0)) {
+		if ((++vq->last_used_idx & (vq->size - 1)) == 0)
+			toggle_wrap_counter(vq);
+
+		desc = &descs[vq->last_used_idx & (vq->size - 1)];
+
+		if (unlikely(desc->flags & VRING_DESC_F_INDIRECT)) {
+			RTE_LOG(ERR, VHOST_DATA,
+				"INDIRECT not supported yet\n");
+			rc = -1;
+			goto out;
+		}
+
+		desc_addr = vhost_iova_to_vva(dev, vq, desc->addr,
+					      sizeof(*desc), VHOST_ACCESS_RO);
+		if (unlikely(!desc_addr)) {
+			rc = -1;
+			goto out;
+		}
+
+		desc_offset = 0;
+		desc_avail  = desc->len;
+	} else {
+		desc_avail  = desc->len - dev->vhost_hlen;
+		desc_offset = dev->vhost_hlen;
+	}
+
+	rte_prefetch0((void *)(uintptr_t)(desc_addr + desc_offset));
+
+	PRINT_PACKET(dev, (uintptr_t)(desc_addr + desc_offset), desc_avail, 0);
+
+	mbuf_offset = 0;
+	mbuf_avail  = m->buf_len - RTE_PKTMBUF_HEADROOM;
+	while (1) {
+		uint64_t hpa;
+
+		cpy_len = RTE_MIN(desc_avail, mbuf_avail);
+
+		/*
+		 * A desc buf might across two host physical pages that are
+		 * not continuous. In such case (gpa_to_hpa returns 0), data
+		 * will be copied even though zero copy is enabled.
+		 */
+		if (unlikely(dev->dequeue_zero_copy && (hpa = gpa_to_hpa(dev,
+					desc->addr + desc_offset, cpy_len)))) {
+			cur->data_len = cpy_len;
+			cur->data_off = 0;
+			cur->buf_addr = (void *)(uintptr_t)desc_addr;
+			cur->buf_physaddr = hpa;
+
+			/*
+			 * In zero copy mode, one mbuf can only reference data
+			 * for one or partial of one desc buff.
+			 */
+			mbuf_avail = cpy_len;
+		} else {
+			rte_memcpy(rte_pktmbuf_mtod_offset(cur, void *,
+							   mbuf_offset),
+				(void *)((uintptr_t)(desc_addr + desc_offset)),
+				cpy_len);
+		}
+
+		mbuf_avail  -= cpy_len;
+		mbuf_offset += cpy_len;
+		desc_avail  -= cpy_len;
+		desc_offset += cpy_len;
+
+		/* This desc reaches to its end, get the next one */
+		if (desc_avail == 0) {
+			if ((desc->flags & VRING_DESC_F_NEXT) == 0)
+				break;
+
+			if ((++vq->last_used_idx & (vq->size - 1)) == 0)
+				toggle_wrap_counter(vq);
+
+			desc = &descs[vq->last_used_idx & (vq->size - 1)];
+			if (unlikely(desc->flags & VRING_DESC_F_INDIRECT)) {
+				RTE_LOG(ERR, VHOST_DATA, "INDIRECT not supported yet");
+			}
+
+			desc_addr = vhost_iova_to_vva(dev, vq, desc->addr,
+						      sizeof(*desc), VHOST_ACCESS_RO);
+			if (unlikely(!desc_addr)) {
+				rc = -1;
+				goto out;
+			}
+
+			rte_prefetch0((void *)(uintptr_t)desc_addr);
+
+			desc_offset = 0;
+			desc_avail  = desc->len;
+
+			PRINT_PACKET(dev, (uintptr_t)desc_addr, desc->len, 0);
+		}
+
+		/*
+		 * This mbuf reaches to its end, get a new one
+		 * to hold more data.
+		 */
+		if (mbuf_avail == 0) {
+			cur = rte_pktmbuf_alloc(mbuf_pool);
+			if (unlikely(cur == NULL)) {
+				RTE_LOG(ERR, VHOST_DATA, "Failed to "
+					"allocate memory for mbuf.\n");
+				rc = -1;
+				goto out;
+			}
+
+			prev->next = cur;
+			prev->data_len = mbuf_offset;
+			m->nb_segs += 1;
+			m->pkt_len += mbuf_offset;
+			prev = cur;
+
+			mbuf_offset = 0;
+			mbuf_avail  = cur->buf_len - RTE_PKTMBUF_HEADROOM;
+		}
+	}
+
+	if (hdr)
+		vhost_dequeue_offload(hdr, m);
+
+	if ((++vq->last_used_idx & (vq->size - 1)) == 0)
+		toggle_wrap_counter(vq);
+
+	rte_smp_wmb();
+	_set_desc_used(&descs[head_idx], wrap_counter);
+
+	prev->data_len = mbuf_offset;
+	m->pkt_len    += mbuf_offset;
+
+out:
+	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
+		vhost_user_iotlb_rd_unlock(vq);
+	rte_spinlock_unlock(&vq->access_lock);
+
+	return rc;
+}
+
+static inline uint16_t
+vhost_dequeue_burst_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
+			struct rte_mempool *mbuf_pool, struct rte_mbuf **pkts,
+			uint16_t count)
+{
+	uint16_t i;
+	uint16_t idx;
+	struct vring_desc_packed *desc = vq->desc_packed;
+	int err;
+
+	count = RTE_MIN(MAX_PKT_BURST, count);
+	for (i = 0; i < count; i++) {
+		idx = vq->last_used_idx & (vq->size - 1);
+		if (!desc_is_avail(vq, &desc[idx]))
+			break;
+		rte_smp_rmb();
+
+		pkts[i] = rte_pktmbuf_alloc(mbuf_pool);
+		if (unlikely(pkts[i] == NULL)) {
+			RTE_LOG(ERR, VHOST_DATA,
+				"Failed to allocate memory for mbuf.\n");
+			break;
+		}
+
+		err = dequeue_desc(dev, vq, mbuf_pool, pkts[i], desc);
+		if (unlikely(err)) {
+			rte_pktmbuf_free(pkts[i]);
+			break;
+		}
+	}
+
+	rte_spinlock_unlock(&vq->access_lock);
+
+	return i;
+}
+
 uint16_t
 rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
 	struct rte_mempool *mbuf_pool, struct rte_mbuf **pkts, uint16_t count)
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 12/17] vhost: dequeue for packed queues

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:15PM +0100, Jens Freimann wrote:
> Implement code to dequeue and process descriptors from
> the vring if VIRTIO_F_PACKED is enabled.

VIRTIO_F_RING_PACKED.

> 
> Check if descriptor was made available by driver by looking at
> VIRTIO_F_DESC_AVAIL flag in descriptor. If so dequeue and set
> the used flag VIRTIO_F_DESC_USED to the current value of the
> used wrap counter.
> 
> Used ring wrap counter needs to be toggled when last descriptor is
> written out. This allows the host/guest to detect new descriptors even
> after the ring has wrapped.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  lib/librte_vhost/vhost.c      |   1 +
>  lib/librte_vhost/vhost.h      |   1 +
>  lib/librte_vhost/virtio_net.c | 228 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 230 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index a300812..8cba10d 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -198,6 +198,7 @@ struct virtio_net *
>  
>  	vq->kickfd = VIRTIO_UNINITIALIZED_EVENTFD;
>  	vq->callfd = VIRTIO_UNINITIALIZED_EVENTFD;
> +	vq->used_wrap_counter = 1;
>  
>  	vhost_user_iotlb_init(dev, vring_idx);
>  	/* Backends are set to -1 indicating an inactive device. */
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index d35c4b1..f77fefe 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -108,6 +108,7 @@ struct vhost_virtqueue {
>  
>  	struct batch_copy_elem	*batch_copy_elems;
>  	uint16_t		batch_copy_nb_elems;
> +	uint32_t		used_wrap_counter;

I didn't look into this, is uint32_t the best choice for
defining used_wrap_counter compared with uint16_t or uint8_t?

>  
>  	rte_rwlock_t	iotlb_lock;
>  	rte_rwlock_t	iotlb_pending_lock;
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index 700aca7..8f59e4f 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -19,6 +19,7 @@
>  
>  #include "iotlb.h"
>  #include "vhost.h"
> +#include "virtio-1.1.h"
>  
>  #define MAX_PKT_BURST 32
>  
> @@ -1118,6 +1119,233 @@
>  	}
>  }
>  
> +static inline uint16_t
> +dequeue_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,

It's better to have the word "packed" in the function name.

> +	     struct rte_mempool *mbuf_pool, struct rte_mbuf *m,
> +	     struct vring_desc_packed *descs)
> +{
> +	struct vring_desc_packed *desc;
> +	uint64_t desc_addr;
> +	uint32_t desc_avail, desc_offset;
> +	uint32_t mbuf_avail, mbuf_offset;
> +	uint32_t cpy_len;
> +	struct rte_mbuf *cur = m, *prev = m;
> +	struct virtio_net_hdr *hdr = NULL;
> +	uint16_t head_idx = vq->last_used_idx & (vq->size - 1);

The ring size may not be a power of 2.

> +	int wrap_counter = vq->used_wrap_counter;
> +	int rc = 0;
> +
> +	rte_spinlock_lock(&vq->access_lock);
> +
> +	if (unlikely(vq->enabled == 0))
> +		goto out;
> +
> +	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
> +		vhost_user_iotlb_rd_lock(vq);
> +
> +	desc = &descs[vq->last_used_idx & (vq->size - 1)];
> +	if (unlikely((desc->len < dev->vhost_hlen)) ||
> +			(desc->flags & VRING_DESC_F_INDIRECT)) {
> +			RTE_LOG(ERR, VHOST_DATA,
> +				"INDIRECT not supported yet\n");
> +			rc = -1;
> +			goto out;

If INDIRECT isn't supported, we will need to make sure
that INDIRECT and RING_PACKED won't be negotiated at
the same time.

Thanks

[dpdk-dev] [PATCH 13/17] vhost: packed queue enqueue path

[Jens Freimann]

Implement enqueue of packets to the receive virtqueue.

Set descriptor flag VIRTQ_DESC_F_USED and toggle used wrap counter if
last descriptor in ring is used. Perform a write memory barrier before
flags are written to descriptor.

Chained descriptors are not supported with this patch.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 lib/librte_vhost/virtio_net.c | 129 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 8f59e4f..ec4908a 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -695,6 +695,135 @@
 	return pkt_idx;
 }
 
+static inline uint32_t __attribute__((always_inline))
+vhost_enqueue_burst_packed(struct virtio_net *dev, uint16_t queue_id,
+	      struct rte_mbuf **pkts, uint32_t count)
+{
+	struct vhost_virtqueue *vq;
+	struct vring_desc_packed *descs;
+	uint16_t idx;
+	uint16_t mask;
+	uint16_t i;
+
+	vq = dev->virtqueue[queue_id];
+
+	rte_spinlock_lock(&vq->access_lock);
+
+	if (unlikely(vq->enabled == 0)) {
+		i = 0;
+		goto out_access_unlock;
+	}
+
+	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
+		vhost_user_iotlb_rd_lock(vq);
+
+	descs = vq->desc_packed;
+	mask = vq->size - 1;
+
+	for (i = 0; i < count; i++) {
+		uint32_t desc_avail, desc_offset;
+		uint32_t mbuf_avail, mbuf_offset;
+		uint32_t cpy_len;
+		struct vring_desc_packed *desc;
+		uint64_t desc_addr;
+		struct virtio_net_hdr_mrg_rxbuf *hdr;
+		struct rte_mbuf *m = pkts[i];
+
+		/* XXX: there is an assumption that no desc will be chained */
+		idx = vq->last_used_idx & mask;
+		desc = &descs[idx];
+
+		if (!desc_is_avail(vq, desc))
+			break;
+		rte_smp_rmb();
+
+		desc_addr = vhost_iova_to_vva(dev, vq, desc->addr,
+					      sizeof(*desc), VHOST_ACCESS_RW);
+		/*
+		 * Checking of 'desc_addr' placed outside of 'unlikely' macro
+		 * to avoid performance issue with some versions of gcc (4.8.4
+		 * and 5.3.0) which otherwise stores offset on the stack instead
+		 * of in a register.
+		 */
+		if (unlikely(desc->len < dev->vhost_hlen) || !desc_addr)
+			break;
+
+		hdr = (struct virtio_net_hdr_mrg_rxbuf *)(uintptr_t)desc_addr;
+		virtio_enqueue_offload(m, &hdr->hdr);
+		vhost_log_write(dev, desc->addr, dev->vhost_hlen);
+		PRINT_PACKET(dev, (uintptr_t)desc_addr, dev->vhost_hlen, 0);
+
+		desc_offset = dev->vhost_hlen;
+		desc_avail  = desc->len - dev->vhost_hlen;
+
+		mbuf_avail  = rte_pktmbuf_data_len(m);
+		mbuf_offset = 0;
+		while (mbuf_avail != 0 || m->next != NULL) {
+			/* done with current mbuf, fetch next */
+			if (mbuf_avail == 0) {
+				m = m->next;
+
+				mbuf_offset = 0;
+				mbuf_avail  = rte_pktmbuf_data_len(m);
+			}
+
+			/* done with current desc buf, fetch next */
+			if (desc_avail == 0) {
+				if ((desc->flags & VRING_DESC_F_NEXT) == 0) {
+					/* Room in vring buffer is not enough */
+					goto out;
+				}
+
+				idx = (idx + 1);
+				desc = &descs[idx];
+				if (unlikely(!desc_is_avail(vq, desc)))
+					goto out ;
+
+				desc_addr = vhost_iova_to_vva(dev, vq, desc->addr,
+							      sizeof(*desc),
+							      VHOST_ACCESS_RW);
+				if (unlikely(!desc_addr))
+					goto out;
+
+				desc_offset = 0;
+				desc_avail  = desc->len;
+			}
+
+			cpy_len = RTE_MIN(desc_avail, mbuf_avail);
+			rte_memcpy((void *)((uintptr_t)(desc_addr + desc_offset)),
+				rte_pktmbuf_mtod_offset(m, void *, mbuf_offset),
+				cpy_len);
+			vhost_log_write(dev, desc->addr + desc_offset, cpy_len);
+			PRINT_PACKET(dev, (uintptr_t)(desc_addr + desc_offset),
+				     cpy_len, 0);
+
+			mbuf_avail  -= cpy_len;
+			mbuf_offset += cpy_len;
+			desc_avail  -= cpy_len;
+			desc_offset += cpy_len;
+		}
+
+		descs[idx].len = pkts[i]->pkt_len + dev->vhost_hlen;
+		rte_smp_wmb();
+		set_desc_used(vq, desc);
+
+		vq->last_used_idx++;
+		if ((vq->last_used_idx & (vq->size - 1)) == 0)
+			toggle_wrap_counter(vq);
+	}
+
+out:
+	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
+		vhost_user_iotlb_rd_unlock(vq);
+
+out_access_unlock:
+	rte_spinlock_unlock(&vq->access_lock);
+
+	count = i;
+
+	return count;
+}
+
 uint16_t
 rte_vhost_enqueue_burst(int vid, uint16_t queue_id,
 	struct rte_mbuf **pkts, uint16_t count)
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 13/17] vhost: packed queue enqueue path

[Tiwei Bie]

On Fri, Mar 16, 2018 at 04:21:16PM +0100, Jens Freimann wrote:
[...]
> +static inline uint32_t __attribute__((always_inline))
> +vhost_enqueue_burst_packed(struct virtio_net *dev, uint16_t queue_id,
> +	      struct rte_mbuf **pkts, uint32_t count)
> +{
> +	struct vhost_virtqueue *vq;
> +	struct vring_desc_packed *descs;
> +	uint16_t idx;
> +	uint16_t mask;
> +	uint16_t i;
> +
> +	vq = dev->virtqueue[queue_id];
> +
> +	rte_spinlock_lock(&vq->access_lock);
> +
> +	if (unlikely(vq->enabled == 0)) {
> +		i = 0;
> +		goto out_access_unlock;
> +	}
> +
> +	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
> +		vhost_user_iotlb_rd_lock(vq);
> +
> +	descs = vq->desc_packed;
> +	mask = vq->size - 1;

Queue size may not be a power of 2 in packed ring.

> +
> +	for (i = 0; i < count; i++) {
[...]
> +		mbuf_avail  = rte_pktmbuf_data_len(m);
> +		mbuf_offset = 0;
> +		while (mbuf_avail != 0 || m->next != NULL) {
> +			/* done with current mbuf, fetch next */
> +			if (mbuf_avail == 0) {
> +				m = m->next;
> +
> +				mbuf_offset = 0;
> +				mbuf_avail  = rte_pktmbuf_data_len(m);
> +			}
> +
> +			/* done with current desc buf, fetch next */
> +			if (desc_avail == 0) {
> +				if ((desc->flags & VRING_DESC_F_NEXT) == 0) {
> +					/* Room in vring buffer is not enough */
> +					goto out;
> +				}
> +
> +				idx = (idx + 1);
> +				desc = &descs[idx];

Need to check whether idx >= queue size.

Thanks

Re: [dpdk-dev] [PATCH 13/17] vhost: packed queue enqueue path

[Jens Freimann]

On Mon, Mar 19, 2018 at 07:02:56PM +0800, Tiwei Bie wrote:
>On Fri, Mar 16, 2018 at 04:21:16PM +0100, Jens Freimann wrote:
>[...]
>> +static inline uint32_t __attribute__((always_inline))
>> +vhost_enqueue_burst_packed(struct virtio_net *dev, uint16_t queue_id,
>> +	      struct rte_mbuf **pkts, uint32_t count)
>> +{
>> +	struct vhost_virtqueue *vq;
>> +	struct vring_desc_packed *descs;
>> +	uint16_t idx;
>> +	uint16_t mask;
>> +	uint16_t i;
>> +
>> +	vq = dev->virtqueue[queue_id];
>> +
>> +	rte_spinlock_lock(&vq->access_lock);
>> +
>> +	if (unlikely(vq->enabled == 0)) {
>> +		i = 0;
>> +		goto out_access_unlock;
>> +	}
>> +
>> +	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
>> +		vhost_user_iotlb_rd_lock(vq);
>> +
>> +	descs = vq->desc_packed;
>> +	mask = vq->size - 1;
>
>Queue size may not be a power of 2 in packed ring.

yes, you are right. my patches don't support this yet, but I plan to
add it in the next version.

Thanks for the review!

regards,
Jens 

Re: [dpdk-dev] [PATCH 13/17] vhost: packed queue enqueue path

[Tiwei Bie]

On Wed, Mar 21, 2018 at 09:45:59AM +0100, Jens Freimann wrote:
> On Mon, Mar 19, 2018 at 07:02:56PM +0800, Tiwei Bie wrote:
> > On Fri, Mar 16, 2018 at 04:21:16PM +0100, Jens Freimann wrote:
> > [...]
> > > +static inline uint32_t __attribute__((always_inline))
> > > +vhost_enqueue_burst_packed(struct virtio_net *dev, uint16_t queue_id,
> > > +	      struct rte_mbuf **pkts, uint32_t count)
> > > +{
> > > +	struct vhost_virtqueue *vq;
> > > +	struct vring_desc_packed *descs;
> > > +	uint16_t idx;
> > > +	uint16_t mask;
> > > +	uint16_t i;
> > > +
> > > +	vq = dev->virtqueue[queue_id];
> > > +
> > > +	rte_spinlock_lock(&vq->access_lock);
> > > +
> > > +	if (unlikely(vq->enabled == 0)) {
> > > +		i = 0;
> > > +		goto out_access_unlock;
> > > +	}
> > > +
> > > +	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
> > > +		vhost_user_iotlb_rd_lock(vq);
> > > +
> > > +	descs = vq->desc_packed;
> > > +	mask = vq->size - 1;
> > 
> > Queue size may not be a power of 2 in packed ring.
> 
> yes, you are right. my patches don't support this yet, but I plan to
> add it in the next version.

Yeah, thanks. Otherwise, because this is vhost code, and
the vq->size is set by virtio driver. So when above code
works with a virtio driver which uses a ring size that's
not a power of 2, such assumption will cause problems.

Thanks

> 
> Thanks for the review!
> 
> regards,
> Jens

[dpdk-dev] [PATCH 14/17] vhost: enable packed virtqueues

[Jens Freimann]

From: Yuanhan Liu <yuanhan.liu@linux.intel.com>

This patch enables the code do enqueue and dequeue packed to/from a
packed virtqueue.  Add feature bit for packed virtqueues as defined in
Virtio 1.1 draft.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/vhost.h      | 1 +
 lib/librte_vhost/virtio_net.c | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index f77fefe..561d640 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -188,6 +188,7 @@ struct vhost_msg {
 				(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE) | \
 				(1ULL << VIRTIO_NET_F_MQ)      | \
 				(1ULL << VIRTIO_F_VERSION_1)   | \
+				(1ULL << VIRTIO_F_RING_PACKED) | \
 				(1ULL << VHOST_F_LOG_ALL)      | \
 				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) | \
 				(1ULL << VIRTIO_NET_F_GSO) | \
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index ec4908a..d96d717 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -840,7 +840,9 @@ static inline uint32_t __attribute__((always_inline))
 		return 0;
 	}
 
-	if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF))
+	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED))
+		return vhost_enqueue_burst_packed(dev, queue_id, pkts, count);
+	else if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF))
 		return virtio_dev_merge_rx(dev, queue_id, pkts, count);
 	else
 		return virtio_dev_rx(dev, queue_id, pkts, count);
@@ -1513,6 +1515,9 @@ static inline uint32_t __attribute__((always_inline))
 	if (unlikely(vq->enabled == 0))
 		goto out_access_unlock;
 
+	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED))
+		return vhost_dequeue_burst_packed(dev, vq, mbuf_pool, pkts, count);
+
 	vq->batch_copy_nb_elems = 0;
 
 	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
-- 
1.8.3.1

[dpdk-dev] [PATCH 15/17] net/virtio: disable ctrl virtqueue for packed rings

[Jens Freimann]

Signed-off-by: Jens Freiman <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 888cc49..9f372a4 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1195,6 +1195,13 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 			req_features &= ~(1ULL << VIRTIO_NET_F_MTU);
 	}
 
+	if (req_features & (1ULL << VIRTIO_F_RING_PACKED)) {
+		req_features &= ~(1ull << VIRTIO_NET_F_CTRL_MAC_ADDR);
+		req_features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
+		req_features &= ~(1ull << VIRTIO_NET_F_CTRL_RX);
+		req_features &= ~(1ull << VIRTIO_NET_F_CTRL_VLAN);
+	}
+
 	/*
 	 * Negotiate features: Subset of device feature bits are written back
 	 * guest feature bits.
-- 
1.8.3.1

[dpdk-dev] [PATCH 16/17] net/virtio: add support for mergeable buffers with packed virtqueues

[Jens Freimann]

Implement support for receiving merged buffers in virtio when packed virtqueues
are enabled. 

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c |   5 +-
 drivers/net/virtio/virtio_rxtx.c   | 107 +++++++++++++++++++++++++++++++++----
 drivers/net/virtio/virtqueue.h     |   1 +
 3 files changed, 103 insertions(+), 10 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 9f372a4..d9e7399 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1360,7 +1360,10 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 			eth_dev->data->port_id);
 		eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
 	} else if (vtpci_packed_queue(hw)) {
-		eth_dev->rx_pkt_burst = &virtio_recv_pkts_packed;
+		if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF))
+			eth_dev->rx_pkt_burst = &virtio_recv_mergeable_pkts;
+		else
+			eth_dev->rx_pkt_burst = &virtio_recv_pkts_packed;
 	} else if (vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
 		PMD_INIT_LOG(INFO,
 			"virtio: using mergeable buffer Rx path on port %u",
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 7834747..5545aa4 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -64,8 +64,8 @@
 	vq->vq_free_cnt = (uint16_t)(vq->vq_free_cnt + dxp->ndescs);
 	if ((dp->flags & VRING_DESC_F_INDIRECT) == 0) {
 		while (dp->flags & VRING_DESC_F_NEXT) {
-			desc_idx_last = dp->next;
-			dp = &vq->vq_ring.desc[dp->next];
+			desc_idx_last = desc_idx++;
+			dp = &vq->vq_ring.desc[desc_idx];
 		}
 	}
 	dxp->ndescs = 0;
@@ -86,6 +86,76 @@
 	dp->next = VQ_RING_DESC_CHAIN_END;
 }
 
+static void
+virtio_refill_packed(struct virtqueue *vq, uint16_t used_idx, struct virtnet_rx *rxvq)
+{
+	struct vq_desc_extra *dxp;
+	struct vring_desc_packed *descs = vq->vq_ring.desc_packed;
+	struct vring_desc_packed *desc;
+	struct rte_mbuf *nmb;
+
+	nmb = rte_mbuf_raw_alloc(rxvq->mpool);
+	if (unlikely(nmb == NULL)) {
+		struct rte_eth_dev *dev
+			= &rte_eth_devices[rxvq->port_id];
+		dev->data->rx_mbuf_alloc_failed++;
+		return;
+	}
+
+	desc = &descs[used_idx & (vq->vq_nentries - 1)];
+
+	dxp = &vq->vq_descx[used_idx & (vq->vq_nentries - 1)];
+
+	dxp->cookie = nmb;
+	dxp->ndescs = 1;
+
+	desc->addr = VIRTIO_MBUF_ADDR(nmb, vq) +
+		RTE_PKTMBUF_HEADROOM - vq->hw->vtnet_hdr_size;
+	desc->len = nmb->buf_len - RTE_PKTMBUF_HEADROOM +
+		vq->hw->vtnet_hdr_size;
+	desc->flags |= VRING_DESC_F_WRITE;
+
+
+}
+
+static uint16_t
+virtqueue_dequeue_burst_rx_packed(struct virtqueue *vq, struct rte_mbuf **rx_pkts,
+			   uint32_t *len, uint16_t num, struct virtnet_rx *rx_queue)
+{
+	struct rte_mbuf *cookie;
+	uint16_t used_idx;
+	struct vring_desc_packed *desc;
+	uint16_t i;
+
+	for (i = 0; i < num; i++) {
+		used_idx = (uint16_t)(vq->vq_used_cons_idx & (vq->vq_nentries - 1));
+		desc = &vq->vq_ring.desc_packed[used_idx];
+		if (!desc_is_used(desc))
+			return i;
+		len[i] = desc->len;
+		cookie = (struct rte_mbuf *)vq->vq_descx[used_idx].cookie;
+
+		if (unlikely(cookie == NULL)) {
+			PMD_DRV_LOG(ERR, "vring descriptor with no mbuf cookie at %u",
+				vq->vq_used_cons_idx);
+			break;
+		}
+		rte_prefetch0(cookie);
+		rte_packet_prefetch(rte_pktmbuf_mtod(cookie, void *));
+		rx_pkts[i] = cookie;
+
+		virtio_refill_packed(vq, used_idx, rx_queue);
+
+		rte_smp_wmb();
+		if ((vq->vq_used_cons_idx & (vq->vq_nentries - 1)) == 0)
+			toggle_wrap_counter(&vq->vq_ring);
+		set_desc_avail(&vq->vq_ring, desc);
+		vq->vq_used_cons_idx++;
+	}
+
+	return i;
+}
+
 static uint16_t
 virtqueue_dequeue_burst_rx(struct virtqueue *vq, struct rte_mbuf **rx_pkts,
 			   uint32_t *len, uint16_t num)
@@ -968,12 +1038,16 @@
 	uint32_t seg_res;
 	uint32_t hdr_size;
 	int offload;
+	uint32_t rx_num = 0;
 
 	nb_rx = 0;
 	if (unlikely(hw->started == 0))
 		return nb_rx;
 
-	nb_used = VIRTQUEUE_NUSED(vq);
+	if (vtpci_packed_queue(vq->hw))
+		nb_used = VIRTIO_MBUF_BURST_SZ; //FIXME
+	else
+		nb_used = VIRTQUEUE_NUSED(vq);
 
 	virtio_rmb();
 
@@ -987,14 +1061,23 @@
 	hdr_size = hw->vtnet_hdr_size;
 	offload = rx_offload_enabled(hw);
 
+	vq->vq_used_idx = vq->vq_used_cons_idx;
+
 	while (i < nb_used) {
 		struct virtio_net_hdr_mrg_rxbuf *header;
 
 		if (nb_rx == nb_pkts)
 			break;
 
-		num = virtqueue_dequeue_burst_rx(vq, rcv_pkts, len, 1);
-		if (num != 1)
+		if (vtpci_packed_queue(vq->hw))
+			num = virtqueue_dequeue_burst_rx_packed(vq, rcv_pkts, len, 1, 
+				(struct virtnet_rx *) rx_queue);
+		else
+			num = virtqueue_dequeue_burst_rx(vq, rcv_pkts, len, 1);
+		if (num == 0) {
+			return nb_rx;
+		}
+		if (num != 1 )
 			continue;
 
 		i++;
@@ -1045,9 +1128,13 @@
 			uint16_t  rcv_cnt =
 				RTE_MIN(seg_res, RTE_DIM(rcv_pkts));
 			if (likely(VIRTQUEUE_NUSED(vq) >= rcv_cnt)) {
-				uint32_t rx_num =
-					virtqueue_dequeue_burst_rx(vq,
-					rcv_pkts, len, rcv_cnt);
+				if (vtpci_packed_queue(vq->hw))
+					rx_num = virtqueue_dequeue_burst_rx_packed(vq,
+							     rcv_pkts, len, rcv_cnt,
+							     (struct virtnet_rx *) rx_queue);
+				else
+					rx_num = virtqueue_dequeue_burst_rx(vq,
+							      rcv_pkts, len, rcv_cnt);
 				i += rx_num;
 				rcv_cnt = rx_num;
 			} else {
@@ -1091,6 +1178,9 @@
 
 	rxvq->stats.packets += nb_rx;
 
+	if (vtpci_packed_queue(vq->hw))
+		return nb_rx;
+
 	/* Allocate new mbuf for the used descriptor */
 	error = ENOSPC;
 	while (likely(!virtqueue_full(vq))) {
@@ -1111,7 +1201,6 @@
 
 	if (likely(nb_enqueued)) {
 		vq_update_avail_idx(vq);
-
 		if (unlikely(virtqueue_kick_prepare(vq))) {
 			virtqueue_notify(vq);
 			PMD_RX_LOG(DEBUG, "Notified");
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 82160ca..13cadf8 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -171,6 +171,7 @@ struct virtqueue {
 	 * trails vq_ring.used->idx.
 	 */
 	uint16_t vq_used_cons_idx;
+	uint16_t vq_used_idx;
 	uint16_t vq_nentries;  /**< vring desc numbers */
 	uint16_t vq_free_cnt;  /**< num of desc available */
 	uint16_t vq_avail_idx; /**< sync until needed */
-- 
1.8.3.1

[dpdk-dev] [PATCH 17/17] vhost: support mergeable rx buffers with packed queues

[Jens Freimann]

This implements support for mergeable receive buffers in vhost when using
packed virtqueues. The difference to split virtqueues is not big, it differs
mostly where descriptor flags are touched and virtio features are checked.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 lib/librte_vhost/vhost.c      |   2 +
 lib/librte_vhost/virtio_net.c | 160 +++++++++++++++++++++++++++++++++---------
 2 files changed, 127 insertions(+), 35 deletions(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 8cba10d..18ff6c6 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -561,6 +561,8 @@ struct virtio_net *
 
 	if (dev == NULL)
 		return -1;
+	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED))
+		return -1;
 
 	if (enable) {
 		RTE_LOG(ERR, VHOST_CONFIG,
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index d96d717..f247911 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -401,17 +401,53 @@
 }
 
 static __rte_always_inline int
-fill_vec_buf(struct virtio_net *dev, struct vhost_virtqueue *vq,
-			 uint32_t avail_idx, uint32_t *vec_idx,
-			 struct buf_vector *buf_vec, uint16_t *desc_chain_head,
-			 uint16_t *desc_chain_len)
+__fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
+			 struct buf_vector *buf_vec,
+			 uint32_t *len, uint32_t *vec_id)
+{
+	uint16_t idx = vq->last_avail_idx & (vq->size - 1);
+	struct vring_desc_packed *descs= vq->desc_packed;
+	uint32_t _vec_id = *vec_id;
+
+	if (vq->desc_packed[idx].flags & VRING_DESC_F_INDIRECT) {
+		descs = (struct vring_desc_packed *)(uintptr_t)
+			vhost_iova_to_vva(dev, vq, vq->desc_packed[idx].addr,
+						vq->desc_packed[idx].len,
+						VHOST_ACCESS_RO);
+		if (unlikely(!descs))
+			return -1;
+
+		idx = 0;
+	}
+
+	while (1) {
+		if (unlikely(_vec_id >= BUF_VECTOR_MAX || idx >= vq->size))
+			return -1;
+
+		*len += descs[idx & (vq->size - 1)].len;
+		buf_vec[_vec_id].buf_addr = descs[idx].addr;
+		buf_vec[_vec_id].buf_len  = descs[idx].len;
+		buf_vec[_vec_id].desc_idx = idx;
+		_vec_id++;
+
+		if ((descs[idx & (vq->size - 1)].flags & VRING_DESC_F_NEXT) == 0)
+			break;
+
+		idx++;
+	}
+	*vec_id = _vec_id;
+
+	return 0;
+}
+
+static __rte_always_inline int
+__fill_vec_buf_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
+			 struct buf_vector *buf_vec,
+			 uint32_t *len, uint32_t *vec_id, uint32_t avail_idx)
 {
 	uint16_t idx = vq->avail->ring[avail_idx & (vq->size - 1)];
-	uint32_t vec_id = *vec_idx;
-	uint32_t len    = 0;
 	struct vring_desc *descs = vq->desc;
-
-	*desc_chain_head = idx;
+	uint32_t _vec_id = *vec_id;
 
 	if (vq->desc[idx].flags & VRING_DESC_F_INDIRECT) {
 		descs = (struct vring_desc *)(uintptr_t)
@@ -425,20 +461,53 @@
 	}
 
 	while (1) {
-		if (unlikely(vec_id >= BUF_VECTOR_MAX || idx >= vq->size))
+		if (unlikely(_vec_id >= BUF_VECTOR_MAX || idx >= vq->size))
 			return -1;
 
-		len += descs[idx].len;
-		buf_vec[vec_id].buf_addr = descs[idx].addr;
-		buf_vec[vec_id].buf_len  = descs[idx].len;
-		buf_vec[vec_id].desc_idx = idx;
-		vec_id++;
+		*len += descs[idx].len;
+		buf_vec[_vec_id].buf_addr = descs[idx].addr;
+		buf_vec[_vec_id].buf_len  = descs[idx].len;
+		buf_vec[_vec_id].desc_idx = idx;
+		_vec_id++;
 
 		if ((descs[idx].flags & VRING_DESC_F_NEXT) == 0)
 			break;
 
 		idx = descs[idx].next;
 	}
+	*vec_id = _vec_id;
+
+	return 0;
+}
+
+static __rte_always_inline int
+fill_vec_buf(struct virtio_net *dev, struct vhost_virtqueue *vq,
+			 uint32_t avail_idx, uint32_t *vec_idx,
+			 struct buf_vector *buf_vec, uint16_t *desc_chain_head,
+			 uint16_t *desc_chain_len)
+{
+	uint16_t idx;
+	uint32_t vec_id = *vec_idx;
+	uint32_t len    = 0;
+
+	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED)) {
+		idx = vq->last_avail_idx & (vq->size -1);
+	} else {
+		idx = vq->avail->ring[avail_idx & (vq->size - 1)];
+	}
+
+
+	*desc_chain_head = idx;
+
+	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED)) {
+		if (__fill_vec_buf_packed(dev, vq,
+				buf_vec, &len, &vec_id))
+			return -1;
+	} else {
+		if (__fill_vec_buf_split(dev, vq,
+				buf_vec, &len, &vec_id, avail_idx))
+			return -1;
+	}
 
 	*desc_chain_len = len;
 	*vec_idx = vec_id;
@@ -465,14 +534,16 @@
 	cur_idx  = vq->last_avail_idx;
 
 	while (size > 0) {
-		if (unlikely(cur_idx == avail_head))
+		if (unlikely(cur_idx == avail_head) &&
+			!(dev->features & (1ull < VIRTIO_F_RING_PACKED)))
 			return -1;
 
 		if (unlikely(fill_vec_buf(dev, vq, cur_idx, &vec_idx, buf_vec,
 						&head_idx, &len) < 0))
 			return -1;
 		len = RTE_MIN(len, size);
-		update_shadow_used_ring(vq, head_idx, len);
+		if (!(dev->features & (1ULL << VIRTIO_F_RING_PACKED)))
+			update_shadow_used_ring(vq, head_idx, len);
 		size -= len;
 
 		cur_idx++;
@@ -620,6 +691,8 @@
 	uint16_t num_buffers;
 	struct buf_vector buf_vec[BUF_VECTOR_MAX];
 	uint16_t avail_head;
+	uint16_t i;
+	struct vring_desc_packed *descs = NULL;
 
 	LOG_DEBUG(VHOST_DATA, "(%d) %s\n", dev->vid, __func__);
 	if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->nr_vring))) {
@@ -634,7 +707,6 @@
 
 	if (unlikely(vq->enabled == 0))
 		goto out_access_unlock;
-
 	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
 		vhost_user_iotlb_rd_lock(vq);
 
@@ -648,10 +720,14 @@
 
 	vq->batch_copy_nb_elems = 0;
 
-	rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
-
-	vq->shadow_used_idx = 0;
-	avail_head = *((volatile uint16_t *)&vq->avail->idx);
+	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED)) {
+		avail_head = vq->last_avail_idx;
+		descs = vq->desc_packed;
+	} else {
+		rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
+		avail_head = *((volatile uint16_t *)&vq->avail->idx);
+		vq->shadow_used_idx = 0;
+	}
 	for (pkt_idx = 0; pkt_idx < count; pkt_idx++) {
 		uint32_t pkt_len = pkts[pkt_idx]->pkt_len + dev->vhost_hlen;
 
@@ -661,7 +737,9 @@
 			LOG_DEBUG(VHOST_DATA,
 				"(%d) failed to get enough desc from vring\n",
 				dev->vid);
-			vq->shadow_used_idx -= num_buffers;
+
+			if (!dev->features & (1ULL & VIRTIO_F_RING_PACKED))
+				vq->shadow_used_idx -= num_buffers;
 			break;
 		}
 
@@ -671,7 +749,8 @@
 
 		if (copy_mbuf_to_desc_mergeable(dev, vq, pkts[pkt_idx],
 						buf_vec, num_buffers) < 0) {
-			vq->shadow_used_idx -= num_buffers;
+			if (!dev->features & (1ULL & VIRTIO_F_RING_PACKED))
+				vq->shadow_used_idx -= num_buffers;
 			break;
 		}
 
@@ -680,9 +759,18 @@
 
 	do_data_copy_enqueue(dev, vq);
 
-	if (likely(vq->shadow_used_idx)) {
-		flush_shadow_used_ring(dev, vq);
-		vhost_vring_call(dev, vq);
+	if (!(dev->features & (1ULL << VIRTIO_F_RING_PACKED))) {
+		if (likely(vq->shadow_used_idx)) {
+			flush_shadow_used_ring(dev, vq);
+			vhost_vring_call(dev, vq);
+		}
+	} else {
+		rte_smp_wmb();
+		for (i = avail_head; i < vq->last_avail_idx; i++) {
+			if ((i & (vq->size - 1)) == 0)
+				toggle_wrap_counter(vq);
+			set_desc_used(vq, &descs[i & (vq->size - 1)]);
+		}
 	}
 
 out:
@@ -774,7 +862,7 @@ static inline uint32_t __attribute__((always_inline))
 					goto out;
 				}
 
-				idx = (idx + 1);
+				idx = (idx + 1) & mask;
 				desc = &descs[idx];
 				if (unlikely(!desc_is_avail(vq, desc)))
 					goto out ;
@@ -840,10 +928,11 @@ static inline uint32_t __attribute__((always_inline))
 		return 0;
 	}
 
-	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED))
-		return vhost_enqueue_burst_packed(dev, queue_id, pkts, count);
-	else if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF))
+	if ((dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) &&
+	    (dev->features & (1ULL << VIRTIO_F_RING_PACKED)))
 		return virtio_dev_merge_rx(dev, queue_id, pkts, count);
+	else if (dev->features & (1ULL << VIRTIO_F_RING_PACKED))
+		return vhost_enqueue_burst_packed(dev, queue_id, pkts, count);
 	else
 		return virtio_dev_rx(dev, queue_id, pkts, count);
 }
@@ -1266,8 +1355,6 @@ static inline uint32_t __attribute__((always_inline))
 	int wrap_counter = vq->used_wrap_counter;
 	int rc = 0;
 
-	rte_spinlock_lock(&vq->access_lock);
-
 	if (unlikely(vq->enabled == 0))
 		goto out;
 
@@ -1451,6 +1538,9 @@ static inline uint32_t __attribute__((always_inline))
 	struct vring_desc_packed *desc = vq->desc_packed;
 	int err;
 
+	if (unlikely(rte_spinlock_trylock(&vq->access_lock) == 0))
+		return 0;
+
 	count = RTE_MIN(MAX_PKT_BURST, count);
 	for (i = 0; i < count; i++) {
 		idx = vq->last_used_idx & (vq->size - 1);
@@ -1509,15 +1599,15 @@ static inline uint32_t __attribute__((always_inline))
 
 	vq = dev->virtqueue[queue_id];
 
+	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED))
+		return vhost_dequeue_burst_packed(dev, vq, mbuf_pool, pkts, count);
+
 	if (unlikely(rte_spinlock_trylock(&vq->access_lock) == 0))
 		return 0;
 
 	if (unlikely(vq->enabled == 0))
 		goto out_access_unlock;
 
-	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED))
-		return vhost_dequeue_burst_packed(dev, vq, mbuf_pool, pkts, count);
-
 	vq->batch_copy_nb_elems = 0;
 
 	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
-- 
1.8.3.1