[PATCH v5 00/11] implement packed virtqueues

[PATCH v5 00/11] 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.


changes from v4->v5:
* fix VIRTQUEUE_DUMP macro
* fix wrap counter logic in transmit and receive functions  

changes from v3->v4:
* added helpers to increment index and set available/used flags
* driver keeps track of number of descriptors used
* change logic in set_rxtx_funcs()
* add patch for ctrl virtqueue with support for packed virtqueues
* rename virtio-1.1.h to virtio-packed.h
* fix wrong sizeof() in "vhost: vring address setup for packed queues"
* fix coding style of function definition in "net/virtio: add packed
  virtqueue helpers"
* fix padding in vring_size()
* move patches to enable packed virtqueues end of series
* v4 has two open problems: I'm sending it out anyway for feedback/help:
 * when VIRTIO_NET_F_MRG_RXBUF enabled only 128 packets are send in
   guest, i.e. when ring is full for the first time. I suspect a bug in
   setting the avail/used flags

changes from v2->v3:
* implement event suppression
* add code do dump packed virtqueues
* don't use assert in vhost code
* rename virtio-user parameter to packed-vq
* support rxvf flush

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


Jens Freimann (10):
  net/virtio: vring init for packed queues
  net/virtio: add virtio 1.1 defines
  net/virtio: add packed virtqueue helpers
  net/virtio: flush packed receive virtqueues
  net/virtio: dump packed virtqueue data
  net/virtio: implement transmit path for packed queues
  net/virtio: implement receive path for packed queues
  net/virtio: disable ctrl virtqueue for packed rings
  net/virtio: add support for mergeable buffers with packed virtqueues
  net/virtio: add support for event suppression

Yuanhan Liu (1):
  net/virtio-user: add option to use packed queues

 drivers/net/virtio/virtio_ethdev.c            |  50 ++-
 drivers/net/virtio/virtio_ethdev.h            |   4 +
 drivers/net/virtio/virtio_pci.h               |   8 +
 drivers/net/virtio/virtio_ring.h              |  85 ++++-
 drivers/net/virtio/virtio_rxtx.c              | 360 +++++++++++++++++-
 .../net/virtio/virtio_user/virtio_user_dev.c  |  10 +-
 .../net/virtio/virtio_user/virtio_user_dev.h  |   2 +-
 drivers/net/virtio/virtio_user_ethdev.c       |  14 +-
 drivers/net/virtio/virtqueue.c                |  17 +
 drivers/net/virtio/virtqueue.h                | 113 +++++-
 10 files changed, 630 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH v5 01/11] 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   | 55 +++++++++++++++++++++++++++---
 drivers/net/virtio/virtqueue.h     | 10 ++++++
 4 files changed, 80 insertions(+), 15 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 614357da7..ad91f7f82 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -299,19 +299,21 @@ virtio_init_vring(struct virtqueue *vq)
 
 	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 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
 	/*
 	 * 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 58fdd3d45..90204d281 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -113,6 +113,8 @@ struct virtnet_ctl;
 
 #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
@@ -314,6 +316,12 @@ vtpci_with_feature(struct virtio_hw *hw, uint64_t bit)
 	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 9e3c2a015..cea4d441e 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -54,11 +54,38 @@ 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;
+};
+
+#define RING_EVENT_FLAGS_ENABLE 0x0
+#define RING_EVENT_FLAGS_DISABLE 0x1
+#define RING_EVENT_FLAGS_DESC 0x2
+struct vring_packed_desc_event {
+	uint16_t desc_event_off_wrap;
+	uint16_t desc_event_flags;
+};
+
 struct vring {
 	unsigned int num;
-	struct vring_desc  *desc;
-	struct vring_avail *avail;
-	struct vring_used  *used;
+	union {
+		struct vring_desc_packed *desc_packed;
+		struct vring_desc *desc;
+	};
+	union {
+		struct vring_avail *avail;
+		struct vring_packed_desc_event *driver_event;
+	};
+	union {
+		struct vring_used  *used;
+		struct vring_packed_desc_event *device_event;
+	};
 };
 
 /* The standard layout for the ring is a continuous chunk of memory which
@@ -95,10 +122,18 @@ 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)) {
+		size = num * sizeof(struct vring_desc_packed);
+		size += sizeof(struct vring_packed_desc_event);
+		size = RTE_ALIGN_CEIL(size, align);
+		size += sizeof(struct vring_packed_desc_event);
+		return size;
+	}
+
 	size = num * sizeof(struct vring_desc);
 	size += sizeof(struct vring_avail) + (num * sizeof(uint16_t));
 	size = RTE_ALIGN_CEIL(size, align);
@@ -108,10 +143,20 @@ vring_size(unsigned int num, unsigned long align)
 }
 
 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;
+		vr->driver_event = (struct vring_packed_desc_event *)(p +
+			num * sizeof(struct vring_desc_packed));
+		vr->device_event = (struct vring_packed_desc_event *)
+				RTE_ALIGN_CEIL((uintptr_t)(vr->driver_event +
+				sizeof(struct vring_packed_desc_event)), align);
+		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 26518ed98..d2a0b651a 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)
-- 
2.17.1

Re: [PATCH v5 01/11] net/virtio: vring init for packed queues

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
> Sent: Friday, September 7, 2018 2:20 AM
> To: dev@dpdk.org
> Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
> Subject: [dpdk-dev] [PATCH v5 01/11] net/virtio: vring init for packed queues
>
> 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   | 55 +++++++++++++++++++++++++++---
>  drivers/net/virtio/virtqueue.h     | 10 ++++++
>  4 files changed, 80 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index 614357da7..ad91f7f82 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -299,19 +299,21 @@ virtio_init_vring(struct virtqueue *vq)
>
>  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 @@
> virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
>  /*
>   * 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 58fdd3d45..90204d281 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -113,6 +113,8 @@ struct virtnet_ctl;
>
>  #define VIRTIO_F_VERSION_132
>  #define VIRTIO_F_IOMMU_PLATFORM33
> +#define VIRTIO_F_RING_PACKED34
> +#define VIRTIO_F_IN_ORDER35
>
>  /*
>   * Some VirtIO feature bits (currently bits 28 through 31) are @@ -314,6
> +316,12 @@ vtpci_with_feature(struct virtio_hw *hw, uint64_t bit)
>  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 9e3c2a015..cea4d441e 100644
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -54,11 +54,38 @@ 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;

Should be __le64 here, according to spec. same as follows.

> +uint32_t len;
> +uint16_t index;
> +uint16_t flags;
> +};

Should have __attribute__ ((aligned(16))) for alignment according to the spec? Not sure if it automatically aligned at 16bytes boundary.

> +#define RING_EVENT_FLAGS_ENABLE 0x0
> +#define RING_EVENT_FLAGS_DISABLE 0x1
> +#define RING_EVENT_FLAGS_DESC 0x2
> +struct vring_packed_desc_event {
> +uint16_t desc_event_off_wrap;
> +uint16_t desc_event_flags;
> +};
> +
>  struct vring {
>  unsigned int num;
> -struct vring_desc  *desc;
> -struct vring_avail *avail;
> -struct vring_used  *used;
> +union {
> +struct vring_desc_packed *desc_packed;
> +struct vring_desc *desc;
> +};
> +union {
> +struct vring_avail *avail;
> +struct vring_packed_desc_event *driver_event;
> +};
> +union {
> +struct vring_used  *used;
> +struct vring_packed_desc_event *device_event;
> +};
>  };
>
>  /* The standard layout for the ring is a continuous chunk of memory which
> @@ -95,10 +122,18 @@ 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)) {
> +size = num * sizeof(struct vring_desc_packed);
> +size += sizeof(struct vring_packed_desc_event);
> +size = RTE_ALIGN_CEIL(size, align);
> +size += sizeof(struct vring_packed_desc_event);
> +return size;
> +}
> +
>  size = num * sizeof(struct vring_desc);
>  size += sizeof(struct vring_avail) + (num * sizeof(uint16_t));
>  size = RTE_ALIGN_CEIL(size, align);
> @@ -108,10 +143,20 @@ vring_size(unsigned int num, unsigned long align)  }
>
>  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;
> +vr->driver_event = (struct vring_packed_desc_event *)(p +
> +num * sizeof(struct vring_desc_packed));
> +vr->device_event = (struct vring_packed_desc_event *)
> +RTE_ALIGN_CEIL((uintptr_t)(vr-
> >driver_event +
> +sizeof(struct vring_packed_desc_event)),
> align);
> +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 26518ed98..d2a0b651a 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)
> --
> 2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v5 01/11] net/virtio: vring init for packed queues

[Maxime Coquelin]

On 09/06/2018 08:19 PM, Jens Freimann wrote:
>   	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);
> +	}

Maybe worth renaming to vring_desc_init_split() for consistency.

Maxime

Re: [PATCH v5 01/11] net/virtio: vring init for packed queues

[Jens Freimann]

On Wed, Sep 12, 2018 at 10:02:30AM +0200, Maxime Coquelin wrote:
>
>
>On 09/06/2018 08:19 PM, Jens Freimann wrote:
>>  	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);
>>+	}
>
>Maybe worth renaming to vring_desc_init_split() for consistency.

yes, I'll rename it.

regards,
Jens 

[PATCH v5 02/11] net/virtio: add virtio 1.1 defines

[Jens Freimann]

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

diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index cea4d441e..e2c597434 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -15,7 +15,11 @@
 #define VRING_DESC_F_WRITE      2
 /* This means the buffer contains a list of buffer descriptors. */
 #define VRING_DESC_F_INDIRECT   4
+/* This flag means the descriptor was made available by the driver */
 
+#define VRING_DESC_F_AVAIL(b)   ((uint16_t)(b) << 7)
+/* This flag means the descriptor was used by the device */
+#define VRING_DESC_F_USED(b)    ((uint16_t)(b) << 15)
 /* 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. */
-- 
2.17.1

Re: [PATCH v5 02/11] net/virtio: add virtio 1.1 defines

[Gavin Hu (Arm Technology China)]

The following 1.1 new defines should be added in this patch or the parent patch.
VIRTIO_F_IO_BARRIER
VIRTIO_F_SR_IOV

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
> Sent: Friday, September 7, 2018 2:20 AM
> To: dev@dpdk.org
> Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
> Subject: [dpdk-dev] [PATCH v5 02/11] net/virtio: add virtio 1.1 defines
>
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtio_ring.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> index cea4d441e..e2c597434 100644
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -15,7 +15,11 @@
>  #define VRING_DESC_F_WRITE      2
>  /* This means the buffer contains a list of buffer descriptors. */
>  #define VRING_DESC_F_INDIRECT   4
> +/* This flag means the descriptor was made available by the driver */
>
> +#define VRING_DESC_F_AVAIL(b)   ((uint16_t)(b) << 7)
> +/* This flag means the descriptor was used by the device */
> +#define VRING_DESC_F_USED(b)    ((uint16_t)(b) << 15)
>  /* 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. */
> --
> 2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v5 02/11] net/virtio: add virtio 1.1 defines

[Tiwei Bie]

On Mon, Sep 10, 2018 at 05:22:43AM +0000, Gavin Hu (Arm Technology China) wrote:
> The following 1.1 new defines should be added in this patch or the parent patch.
> VIRTIO_F_IO_BARRIER
> VIRTIO_F_SR_IOV

I don't think we should introduce these defines in this patch
or this series. This series should focus on packed ring. And
the title of this patch should be something about packed ring
instead of virtio 1.1.

Thanks

> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
> > Sent: Friday, September 7, 2018 2:20 AM
> > To: dev@dpdk.org
> > Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
> > Subject: [dpdk-dev] [PATCH v5 02/11] net/virtio: add virtio 1.1 defines
> >
> > Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> > ---
> >  drivers/net/virtio/virtio_ring.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> > index cea4d441e..e2c597434 100644
> > --- a/drivers/net/virtio/virtio_ring.h
> > +++ b/drivers/net/virtio/virtio_ring.h
> > @@ -15,7 +15,11 @@
> >  #define VRING_DESC_F_WRITE      2
> >  /* This means the buffer contains a list of buffer descriptors. */
> >  #define VRING_DESC_F_INDIRECT   4
> > +/* This flag means the descriptor was made available by the driver */
> >
> > +#define VRING_DESC_F_AVAIL(b)   ((uint16_t)(b) << 7)
> > +/* This flag means the descriptor was used by the device */
> > +#define VRING_DESC_F_USED(b)    ((uint16_t)(b) << 15)
> >  /* 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. */
> > --
> > 2.17.1
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v5 02/11] net/virtio: add virtio 1.1 defines

[Jens Freimann]

On Mon, Sep 10, 2018 at 05:22:43AM +0000, Gavin Hu (Arm Technology China) wrote:
>The following 1.1 new defines should be added in this patch or the parent patch.
>VIRTIO_F_IO_BARRIER
>VIRTIO_F_SR_IOV

I think I should rename this patch to "add packed virtqueue defines"
instead. Above defines should be introduced in the appropriate patch
set. 

Thanks for the review!

regards,
Jens 

[PATCH v5 03/11] 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 | 26 ++++++++++++++++++++++++++
 drivers/net/virtio/virtqueue.h   | 19 +++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index e2c597434..f3b23f419 100644
--- a/drivers/net/virtio/virtio_ring.h
+++ b/drivers/net/virtio/virtio_ring.h
@@ -78,6 +78,8 @@ struct vring_packed_desc_event {
 
 struct vring {
 	unsigned int num;
+	unsigned int avail_wrap_counter;
+	unsigned int used_wrap_counter;
 	union {
 		struct vring_desc_packed *desc_packed;
 		struct vring_desc *desc;
@@ -92,6 +94,30 @@ struct vring {
 	};
 };
 
+static inline void
+_set_desc_avail(struct vring_desc_packed *desc, int wrap_counter)
+{
+	desc->flags |= VRING_DESC_F_AVAIL(wrap_counter) |
+		       VRING_DESC_F_USED(!wrap_counter);
+}
+
+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, struct vring *vr)
+{
+	uint16_t used, avail;
+
+	used = !!(desc->flags & VRING_DESC_F_USED(1));
+	avail = !!(desc->flags & VRING_DESC_F_AVAIL(1));
+
+	return used == avail && used == vr->used_wrap_counter;
+}
+
 /* 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.h b/drivers/net/virtio/virtqueue.h
index d2a0b651a..53fce61b4 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -245,6 +245,25 @@ struct virtio_tx_region {
 			   __attribute__((__aligned__(16)));
 };
 
+static inline uint16_t
+increment_pq_index(uint16_t idx, size_t ring_size)
+{
+	return ++idx >= ring_size ? 0 : idx;
+}
+
+static inline uint16_t
+update_pq_avail_index(struct virtqueue *vq)
+{
+	uint16_t idx;
+
+	idx = increment_pq_index(vq->vq_avail_idx, vq->vq_nentries);
+	if (idx == 0)
+		vq->vq_ring.avail_wrap_counter ^= 1;
+	vq->vq_avail_idx = idx;
+
+	return vq->vq_avail_idx;
+}
+
 static inline void
 vring_desc_init_packed(struct vring *vr, int n)
 {
-- 
2.17.1

Re: [PATCH v5 03/11] net/virtio: add packed virtqueue helpers

[Maxime Coquelin]

On 09/06/2018 08:19 PM, Jens Freimann wrote:
> Add helper functions to set/clear and check descriptor flags.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtio_ring.h | 26 ++++++++++++++++++++++++++
>   drivers/net/virtio/virtqueue.h   | 19 +++++++++++++++++++
>   2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
> index e2c597434..f3b23f419 100644
> --- a/drivers/net/virtio/virtio_ring.h
> +++ b/drivers/net/virtio/virtio_ring.h
> @@ -78,6 +78,8 @@ struct vring_packed_desc_event {
>   
>   struct vring {
>   	unsigned int num;
> +	unsigned int avail_wrap_counter;
> +	unsigned int used_wrap_counter;
>   	union {
>   		struct vring_desc_packed *desc_packed;
>   		struct vring_desc *desc;
> @@ -92,6 +94,30 @@ struct vring {
>   	};
>   };
>   
> +static inline void
> +_set_desc_avail(struct vring_desc_packed *desc, int wrap_counter)
> +{
> +	desc->flags |= VRING_DESC_F_AVAIL(wrap_counter) |
> +		       VRING_DESC_F_USED(!wrap_counter);

It implies the avail and used bits to be cleared beforehand.
Maybe it would be safer to clear them in the helper?

> +}
> +
> +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, struct vring *vr)
> +{
> +	uint16_t used, avail;
> +
> +	used = !!(desc->flags & VRING_DESC_F_USED(1));
> +	avail = !!(desc->flags & VRING_DESC_F_AVAIL(1));
> +
> +	return used == avail && used == vr->used_wrap_counter;
> +}
> +
>   /* 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.h b/drivers/net/virtio/virtqueue.h
> index d2a0b651a..53fce61b4 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -245,6 +245,25 @@ struct virtio_tx_region {
>   			   __attribute__((__aligned__(16)));
>   };
>   
> +static inline uint16_t
> +increment_pq_index(uint16_t idx, size_t ring_size)
> +{
> +	return ++idx >= ring_size ? 0 : idx;
> +}

Not sure this helper is really useful, as it ends up
doing two checks in a row.

> +
> +static inline uint16_t
> +update_pq_avail_index(struct virtqueue *vq)
> +{
> +	uint16_t idx;
> +
> +	idx = increment_pq_index(vq->vq_avail_idx, vq->vq_nentries);
> +	if (idx == 0)
> +		vq->vq_ring.avail_wrap_counter ^= 1;

> +	vq->vq_avail_idx = idx;
> +
> +	return vq->vq_avail_idx;
> +}

So it could be simplified to:

{
	if (++vq->vq_avail_idx >= vq->vq_entries) {
		vq->vq_avail_idx = 0;
		vq->vq_ring.avail_wrap_counter ^= 1;
	}

	return vq->vq_avail_idx;
}

> +
>   static inline void
>   vring_desc_init_packed(struct vring *vr, int n)
>   {
> 

Re: [PATCH v5 03/11] net/virtio: add packed virtqueue helpers

[Jens Freimann]

On Wed, Sep 12, 2018 at 10:25:57AM +0200, Maxime Coquelin wrote:
>
>
>On 09/06/2018 08:19 PM, Jens Freimann wrote:
>>Add helper functions to set/clear and check descriptor flags.
>>
>>Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>>---
>>  drivers/net/virtio/virtio_ring.h | 26 ++++++++++++++++++++++++++
>>  drivers/net/virtio/virtqueue.h   | 19 +++++++++++++++++++
>>  2 files changed, 45 insertions(+)
>>
>>diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
>>index e2c597434..f3b23f419 100644
>>--- a/drivers/net/virtio/virtio_ring.h
>>+++ b/drivers/net/virtio/virtio_ring.h
>>@@ -78,6 +78,8 @@ struct vring_packed_desc_event {
>>  struct vring {
>>  	unsigned int num;
>>+	unsigned int avail_wrap_counter;
>>+	unsigned int used_wrap_counter;
>>  	union {
>>  		struct vring_desc_packed *desc_packed;
>>  		struct vring_desc *desc;
>>@@ -92,6 +94,30 @@ struct vring {
>>  	};
>>  };
>>+static inline void
>>+_set_desc_avail(struct vring_desc_packed *desc, int wrap_counter)
>>+{
>>+	desc->flags |= VRING_DESC_F_AVAIL(wrap_counter) |
>>+		       VRING_DESC_F_USED(!wrap_counter);
>
>It implies the avail and used bits to be cleared beforehand.
>Maybe it would be safer to clear them in the helper?

Safer but also less explicit for someone who just reads the higher
level function. But I think it's better to go for the safer version
here.  

>
>>+}
>>+
>>+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, struct vring *vr)
>>+{
>>+	uint16_t used, avail;
>>+
>>+	used = !!(desc->flags & VRING_DESC_F_USED(1));
>>+	avail = !!(desc->flags & VRING_DESC_F_AVAIL(1));
>>+
>>+	return used == avail && used == vr->used_wrap_counter;
>>+}
>>+
>>  /* 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.h b/drivers/net/virtio/virtqueue.h
>>index d2a0b651a..53fce61b4 100644
>>--- a/drivers/net/virtio/virtqueue.h
>>+++ b/drivers/net/virtio/virtqueue.h
>>@@ -245,6 +245,25 @@ struct virtio_tx_region {
>>  			   __attribute__((__aligned__(16)));
>>  };
>>+static inline uint16_t
>>+increment_pq_index(uint16_t idx, size_t ring_size)
>>+{
>>+	return ++idx >= ring_size ? 0 : idx;
>>+}
>
>Not sure this helper is really useful, as it ends up
>doing two checks in a row.
>
>>+
>>+static inline uint16_t
>>+update_pq_avail_index(struct virtqueue *vq)
>>+{
>>+	uint16_t idx;
>>+
>>+	idx = increment_pq_index(vq->vq_avail_idx, vq->vq_nentries);
>>+	if (idx == 0)
>>+		vq->vq_ring.avail_wrap_counter ^= 1;
>
>>+	vq->vq_avail_idx = idx;
>>+
>>+	return vq->vq_avail_idx;
>>+}
>
>So it could be simplified to:
>
>{
>	if (++vq->vq_avail_idx >= vq->vq_entries) {
>		vq->vq_avail_idx = 0;
>		vq->vq_ring.avail_wrap_counter ^= 1;
>	}
>
>	return vq->vq_avail_idx;

yes, I will either change to what you suggested or
get rid of the helper completely since it is only used in
very few places.

Thanks for the review!

regards,
Jens 

[PATCH v5 04/11] net/virtio: flush packed receive virtqueues

[Jens Freimann]

Flush used descriptors in packed receive virtqueue. As descriptors
can be chained we need to look at the stored number of used descriptors
to find out the length of the chain.

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

diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
index 56a77cc71..d0520dad1 100644
--- a/drivers/net/virtio/virtqueue.c
+++ b/drivers/net/virtio/virtqueue.c
@@ -58,12 +58,29 @@ virtqueue_detach_unused(struct virtqueue *vq)
 void
 virtqueue_rxvq_flush(struct virtqueue *vq)
 {
+	struct vring_desc_packed *descs = vq->vq_ring.desc_packed;
 	struct virtnet_rx *rxq = &vq->rxq;
 	struct virtio_hw *hw = vq->hw;
 	struct vring_used_elem *uep;
 	struct vq_desc_extra *dxp;
 	uint16_t used_idx, desc_idx;
 	uint16_t nb_used, i;
+	uint16_t size = vq->vq_nentries;
+
+	if (vtpci_packed_queue(vq->hw)) {
+		i = vq->vq_used_cons_idx;
+		while (desc_is_used(&descs[i], &vq->vq_ring) &&
+			i < vq->vq_nentries) {
+			dxp = &vq->vq_descx[i];
+			if (dxp->cookie != NULL)
+				rte_pktmbuf_free(dxp->cookie);
+			vq->vq_free_cnt += dxp->ndescs;
+			i = i + dxp->ndescs;
+			i = i >= size ? i - size : i;
+			dxp->ndescs = 0;
+		}
+		return;
+	}
 
 	nb_used = VIRTQUEUE_NUSED(vq);
 
-- 
2.17.1

Re: [PATCH v5 04/11] net/virtio: flush packed receive virtqueues

[Maxime Coquelin]

On 09/06/2018 08:19 PM, Jens Freimann wrote:
> Flush used descriptors in packed receive virtqueue. As descriptors
> can be chained we need to look at the stored number of used descriptors
> to find out the length of the chain.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtqueue.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtqueue.c b/drivers/net/virtio/virtqueue.c
> index 56a77cc71..d0520dad1 100644
> --- a/drivers/net/virtio/virtqueue.c
> +++ b/drivers/net/virtio/virtqueue.c
> @@ -58,12 +58,29 @@ virtqueue_detach_unused(struct virtqueue *vq)
>   void
>   virtqueue_rxvq_flush(struct virtqueue *vq)
>   {
> +	struct vring_desc_packed *descs = vq->vq_ring.desc_packed;
>   	struct virtnet_rx *rxq = &vq->rxq;
>   	struct virtio_hw *hw = vq->hw;
>   	struct vring_used_elem *uep;
>   	struct vq_desc_extra *dxp;
>   	uint16_t used_idx, desc_idx;
>   	uint16_t nb_used, i;
> +	uint16_t size = vq->vq_nentries;
> +
> +	if (vtpci_packed_queue(vq->hw)) {
> +		i = vq->vq_used_cons_idx;
> +		while (desc_is_used(&descs[i], &vq->vq_ring) &&
> +			i < vq->vq_nentries) {
It may be preferable to ensure 'i' is within the ring size before using
it in desc_is_used(). And raise (at least) an error message if it isn't.

> +			dxp = &vq->vq_descx[i];
> +			if (dxp->cookie != NULL)
> +				rte_pktmbuf_free(dxp->cookie);
> +			vq->vq_free_cnt += dxp->ndescs;
> +			i = i + dxp->ndescs;
> +			i = i >= size ? i - size : i;
> +			dxp->ndescs = 0;
> +		}
> +		return;
> +	}
>   
>   	nb_used = VIRTQUEUE_NUSED(vq);
>   
> 

Re: [PATCH v5 04/11] net/virtio: flush packed receive virtqueues

[Jens Freimann]

On Wed, Sep 12, 2018 at 11:12:50AM +0200, Maxime Coquelin wrote:
>On 09/06/2018 08:19 PM, Jens Freimann wrote:
[...]
>>+	if (vtpci_packed_queue(vq->hw)) {
>>+		i = vq->vq_used_cons_idx;
>>+		while (desc_is_used(&descs[i], &vq->vq_ring) &&
>>+			i < vq->vq_nentries) {
>It may be preferable to ensure 'i' is within the ring size before using
>it in desc_is_used(). And raise (at least) an error message if it isn't.

Yes, I'll fix this. Thanks!

regards,
Jens 

[PATCH v5 05/11] net/virtio: dump packed virtqueue data

[Jens Freimann]

Add support to dump packed virtqueue data to the
VIRTQUEUE_DUMP() macro.

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

diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 53fce61b4..531ba8c65 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -384,6 +384,12 @@ virtqueue_notify(struct virtqueue *vq)
 	uint16_t used_idx, nused; \
 	used_idx = (vq)->vq_ring.used->idx; \
 	nused = (uint16_t)(used_idx - (vq)->vq_used_cons_idx); \
+	if (vtpci_packed_queue((vq)->hw)) { \
+	  PMD_INIT_LOG(DEBUG, \
+	  "VQ: - size=%d; free=%d; last_used_idx=%d;", \
+	  (vq)->vq_nentries, (vq)->vq_free_cnt, nused); \
+	  break; \
+	} \
 	PMD_INIT_LOG(DEBUG, \
 	  "VQ: - size=%d; free=%d; used=%d; desc_head_idx=%d;" \
 	  " avail.idx=%d; used_cons_idx=%d; used.idx=%d;" \
-- 
2.17.1

Re: [PATCH v5 05/11] net/virtio: dump packed virtqueue data

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
> Sent: Friday, September 7, 2018 2:20 AM
> To: dev@dpdk.org
> Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
> Subject: [dpdk-dev] [PATCH v5 05/11] net/virtio: dump packed virtqueue
> data
>
> Add support to dump packed virtqueue data to the
> VIRTQUEUE_DUMP() macro.
>
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>

Acked-by: Gavin Hu <gavin.hu@arm.com>

> ---
>  drivers/net/virtio/virtqueue.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index 53fce61b4..531ba8c65 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -384,6 +384,12 @@ virtqueue_notify(struct virtqueue *vq)
>  uint16_t used_idx, nused; \
>  used_idx = (vq)->vq_ring.used->idx; \
>  nused = (uint16_t)(used_idx - (vq)->vq_used_cons_idx); \
> +if (vtpci_packed_queue((vq)->hw)) { \
> +  PMD_INIT_LOG(DEBUG, \
> +  "VQ: - size=%d; free=%d; last_used_idx=%d;", \
> +  (vq)->vq_nentries, (vq)->vq_free_cnt, nused); \
> +  break; \
> +} \
>  PMD_INIT_LOG(DEBUG, \
>    "VQ: - size=%d; free=%d; used=%d; desc_head_idx=%d;" \
>    " avail.idx=%d; used_cons_idx=%d; used.idx=%d;" \
> --
> 2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v5 05/11] net/virtio: dump packed virtqueue data

[Tiwei Bie]

On Mon, Sep 10, 2018 at 06:02:19AM +0000, Gavin Hu (Arm Technology China) wrote:
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
> > Sent: Friday, September 7, 2018 2:20 AM
> > To: dev@dpdk.org
> > Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
> > Subject: [dpdk-dev] [PATCH v5 05/11] net/virtio: dump packed virtqueue
> > data
> >
> > Add support to dump packed virtqueue data to the
> > VIRTQUEUE_DUMP() macro.
> >
> > Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> 
> Acked-by: Gavin Hu <gavin.hu@arm.com>
> 
> > ---
> >  drivers/net/virtio/virtqueue.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> > index 53fce61b4..531ba8c65 100644
> > --- a/drivers/net/virtio/virtqueue.h
> > +++ b/drivers/net/virtio/virtqueue.h
> > @@ -384,6 +384,12 @@ virtqueue_notify(struct virtqueue *vq)
> >  uint16_t used_idx, nused; \
> >  used_idx = (vq)->vq_ring.used->idx; \

The vq_ring.used doesn't exist in packed ring.

> >  nused = (uint16_t)(used_idx - (vq)->vq_used_cons_idx); \

The nused can't be calculated in this way in
packed ring.

> > +if (vtpci_packed_queue((vq)->hw)) { \
> > +  PMD_INIT_LOG(DEBUG, \
> > +  "VQ: - size=%d; free=%d; last_used_idx=%d;", \
> > +  (vq)->vq_nentries, (vq)->vq_free_cnt, nused); \

And nused doesn't mean last_used_idx.

> > +  break; \
> > +} \
> >  PMD_INIT_LOG(DEBUG, \
> >    "VQ: - size=%d; free=%d; used=%d; desc_head_idx=%d;" \
> >    " avail.idx=%d; used_cons_idx=%d; used.idx=%d;" \
> > --
> > 2.17.1
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v5 05/11] net/virtio: dump packed virtqueue data

[Jens Freimann]

On Mon, Sep 10, 2018 at 02:18:17PM +0800, Tiwei Bie wrote:
>On Mon, Sep 10, 2018 at 06:02:19AM +0000, Gavin Hu (Arm Technology China) wrote:
>>
>>
>> > -----Original Message-----
>> > From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
>> > Sent: Friday, September 7, 2018 2:20 AM
>> > To: dev@dpdk.org
>> > Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
>> > Subject: [dpdk-dev] [PATCH v5 05/11] net/virtio: dump packed virtqueue
>> > data
>> >
>> > Add support to dump packed virtqueue data to the
>> > VIRTQUEUE_DUMP() macro.
>> >
>> > Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>>
>> Acked-by: Gavin Hu <gavin.hu@arm.com>
>>
>> > ---
>> >  drivers/net/virtio/virtqueue.h | 6 ++++++
>> >  1 file changed, 6 insertions(+)
>> >
>> > diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>> > index 53fce61b4..531ba8c65 100644
>> > --- a/drivers/net/virtio/virtqueue.h
>> > +++ b/drivers/net/virtio/virtqueue.h
>> > @@ -384,6 +384,12 @@ virtqueue_notify(struct virtqueue *vq)
>> >  uint16_t used_idx, nused; \
>> >  used_idx = (vq)->vq_ring.used->idx; \
>
>The vq_ring.used doesn't exist in packed ring.
>
>> >  nused = (uint16_t)(used_idx - (vq)->vq_used_cons_idx); \
>
>The nused can't be calculated in this way in
>packed ring.

you're right, this doesn't work. I will fix it and test properly.

regards,
Jens 

Re: [PATCH v5 05/11] net/virtio: dump packed virtqueue data

[Maxime Coquelin]

On 09/06/2018 08:19 PM, Jens Freimann wrote:
> Add support to dump packed virtqueue data to the
> VIRTQUEUE_DUMP() macro.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtqueue.h | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index 53fce61b4..531ba8c65 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -384,6 +384,12 @@ virtqueue_notify(struct virtqueue *vq)
>   	uint16_t used_idx, nused; \
>   	used_idx = (vq)->vq_ring.used->idx; \
>   	nused = (uint16_t)(used_idx - (vq)->vq_used_cons_idx); \
> +	if (vtpci_packed_queue((vq)->hw)) { \
> +	  PMD_INIT_LOG(DEBUG, \
> +	  "VQ: - size=%d; free=%d; last_used_idx=%d;", \
> +	  (vq)->vq_nentries, (vq)->vq_free_cnt, nused); \
> +	  break; \
> +	} \
>   	PMD_INIT_LOG(DEBUG, \
>   	  "VQ: - size=%d; free=%d; used=%d; desc_head_idx=%d;" \
>   	  " avail.idx=%d; used_cons_idx=%d; used.idx=%d;" \
> 

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

Thanks,
Maxime

[PATCH v5 06/11] 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 | 10 ++++++++--
 drivers/net/virtio/virtio_user/virtio_user_dev.h |  2 +-
 drivers/net/virtio/virtio_user_ethdev.c          | 14 +++++++++++++-
 3 files changed, 22 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 7df600b02..9979bea0d 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -372,12 +372,13 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
 	 1ULL << VIRTIO_NET_F_GUEST_TSO4	|	\
 	 1ULL << VIRTIO_NET_F_GUEST_TSO6	|	\
 	 1ULL << VIRTIO_F_IN_ORDER		|	\
-	 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 mrg_rxbuf, int in_order)
+		     int mrg_rxbuf, int in_order, int packed_vq)
 {
 	pthread_mutex_init(&dev->mutex, NULL);
 	snprintf(dev->path, PATH_MAX, "%s", path);
@@ -432,6 +433,11 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		dev->unsupported_features |= (1ull << VIRTIO_F_IN_ORDER);
 	}
 
+	if (packed_vq)
+		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);
 	} else {
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index d6e0e137b..7f46ba1d9 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -49,7 +49,7 @@ 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 mrg_rxbuf, int in_order);
+			 int mrg_rxbuf, int in_order, int packed_vq);
 void virtio_user_dev_uninit(struct virtio_user_dev *dev);
 void virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx);
 uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs);
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 525d16cab..72ac86186 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -364,6 +364,8 @@ static const char *valid_args[] = {
 	VIRTIO_USER_ARG_MRG_RXBUF,
 #define VIRTIO_USER_ARG_IN_ORDER       "in_order"
 	VIRTIO_USER_ARG_IN_ORDER,
+#define VIRTIO_USER_ARG_PACKED_VQ "packed_vq"
+	VIRTIO_USER_ARG_PACKED_VQ,
 	NULL
 };
 
@@ -473,6 +475,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
 	char *ifname = NULL;
 	char *mac_addr = NULL;
 	int ret = -1;
+	uint64_t packed_vq = 0;
 
 	kvlist = rte_kvargs_parse(rte_vdev_device_args(dev), valid_args);
 	if (!kvlist) {
@@ -556,6 +559,15 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
 		cq = 1;
 	}
 
+	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_PACKED_VQ) == 1) {
+		if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_PACKED_VQ,
+				       &get_integer_arg, &packed_vq) < 0) {
+			PMD_INIT_LOG(ERR, "error to parse %s",
+				     VIRTIO_USER_ARG_PACKED_VQ);
+			goto end;
+		}
+	}
+
 	if (queues > 1 && cq == 0) {
 		PMD_INIT_LOG(ERR, "multi-q requires ctrl-q");
 		goto end;
@@ -603,7 +615,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
 			vu_dev->is_server = false;
 		if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
 				 queue_size, mac_addr, &ifname, mrg_rxbuf,
-				 in_order) < 0) {
+				 in_order, packed_vq) < 0) {
 			PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
 			virtio_user_eth_dev_free(eth_dev);
 			goto end;
-- 
2.17.1

Re: [PATCH v5 06/11] net/virtio-user: add option to use packed queues

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
> Sent: Friday, September 7, 2018 2:20 AM
> To: dev@dpdk.org
> Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
> Subject: [dpdk-dev] [PATCH v5 06/11] net/virtio-user: add option to use
> packed queues
>
> 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 | 10 ++++++++--
> drivers/net/virtio/virtio_user/virtio_user_dev.h |  2 +-
>  drivers/net/virtio/virtio_user_ethdev.c          | 14 +++++++++++++-
>  3 files changed, 22 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 7df600b02..9979bea0d 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -372,12 +372,13 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>   1ULL << VIRTIO_NET_F_GUEST_TSO4|\
>   1ULL << VIRTIO_NET_F_GUEST_TSO6|\
>   1ULL << VIRTIO_F_IN_ORDER|\
> - 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 mrg_rxbuf, int in_order)
> +     int mrg_rxbuf, int in_order, int packed_vq)
>  {
>  pthread_mutex_init(&dev->mutex, NULL);
>  snprintf(dev->path, PATH_MAX, "%s", path); @@ -432,6 +433,11
> @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int
> queues,
>  dev->unsupported_features |= (1ull <<
> VIRTIO_F_IN_ORDER);
>  }
>
> +if (packed_vq)
> +dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
> +else
> +dev->device_features &= ~(1ull <<
> VIRTIO_F_RING_PACKED);
> +

unsupported_features should be initialized also like following F_MAC.

>  if (dev->mac_specified) {
>  dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
>  } else {
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index d6e0e137b..7f46ba1d9 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -49,7 +49,7 @@ 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 mrg_rxbuf, int in_order);
> + int mrg_rxbuf, int in_order, int packed_vq);
>  void virtio_user_dev_uninit(struct virtio_user_dev *dev);  void
> virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx);
> uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t
> q_pairs); diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index 525d16cab..72ac86186 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -364,6 +364,8 @@ static const char *valid_args[] = {
>  VIRTIO_USER_ARG_MRG_RXBUF,
>  #define VIRTIO_USER_ARG_IN_ORDER       "in_order"
>  VIRTIO_USER_ARG_IN_ORDER,
> +#define VIRTIO_USER_ARG_PACKED_VQ "packed_vq"
> +VIRTIO_USER_ARG_PACKED_VQ,
>  NULL
>  };
>
> @@ -473,6 +475,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>  char *ifname = NULL;
>  char *mac_addr = NULL;
>  int ret = -1;
> +uint64_t packed_vq = 0;
>
>  kvlist = rte_kvargs_parse(rte_vdev_device_args(dev), valid_args);
>  if (!kvlist) {
> @@ -556,6 +559,15 @@ virtio_user_pmd_probe(struct rte_vdev_device
> *dev)
>  cq = 1;
>  }
>
> +if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_PACKED_VQ) == 1) {
> +if (rte_kvargs_process(kvlist,
> VIRTIO_USER_ARG_PACKED_VQ,
> +       &get_integer_arg, &packed_vq) < 0) {
> +PMD_INIT_LOG(ERR, "error to parse %s",
> +     VIRTIO_USER_ARG_PACKED_VQ);
> +goto end;
> +}
> +}
> +
>  if (queues > 1 && cq == 0) {
>  PMD_INIT_LOG(ERR, "multi-q requires ctrl-q");
>  goto end;
> @@ -603,7 +615,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>  vu_dev->is_server = false;
>  if (virtio_user_dev_init(hw->virtio_user_dev, path, queues,
> cq,
>   queue_size, mac_addr, &ifname, mrg_rxbuf,
> - in_order) < 0) {
> + in_order, packed_vq) < 0) {
>  PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
>  virtio_user_eth_dev_free(eth_dev);
>  goto end;
> --
> 2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v5 06/11] net/virtio-user: add option to use packed queues

[Jens Freimann]

On Mon, Sep 10, 2018 at 06:32:09AM +0000, Gavin Hu (Arm Technology China) wrote:
>
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
>> Sent: Friday, September 7, 2018 2:20 AM
>> To: dev@dpdk.org
>> Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
>> Subject: [dpdk-dev] [PATCH v5 06/11] net/virtio-user: add option to use
>> packed queues
>>
>> 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 | 10 ++++++++--
>> drivers/net/virtio/virtio_user/virtio_user_dev.h |  2 +-
>>  drivers/net/virtio/virtio_user_ethdev.c          | 14 +++++++++++++-
>>  3 files changed, 22 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 7df600b02..9979bea0d 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -372,12 +372,13 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>>   1ULL << VIRTIO_NET_F_GUEST_TSO4|\
>>   1ULL << VIRTIO_NET_F_GUEST_TSO6|\
>>   1ULL << VIRTIO_F_IN_ORDER|\
>> - 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 mrg_rxbuf, int in_order)
>> +     int mrg_rxbuf, int in_order, int packed_vq)
>>  {
>>  pthread_mutex_init(&dev->mutex, NULL);
>>  snprintf(dev->path, PATH_MAX, "%s", path); @@ -432,6 +433,11
>> @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int
>> queues,
>>  dev->unsupported_features |= (1ull <<
>> VIRTIO_F_IN_ORDER);
>>  }
>>
>> +if (packed_vq)
>> +dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
>> +else
>> +dev->device_features &= ~(1ull <<
>> VIRTIO_F_RING_PACKED);
>> +
>
>unsupported_features should be initialized also like following F_MAC.

Fixed it. Thanks!

regards,
Jens 

Re: [PATCH v5 06/11] net/virtio-user: add option to use packed queues

[Maxime Coquelin]

On 09/06/2018 08:19 PM, Jens Freimann wrote:
> 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 | 10 ++++++++--
>   drivers/net/virtio/virtio_user/virtio_user_dev.h |  2 +-
>   drivers/net/virtio/virtio_user_ethdev.c          | 14 +++++++++++++-
>   3 files changed, 22 insertions(+), 4 deletions(-)


Shouldn't this patch be better placed at the end of the series to avoid
negotiating packed ring feature while the datapaths aren't implemented?

> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 7df600b02..9979bea0d 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -372,12 +372,13 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>   	 1ULL << VIRTIO_NET_F_GUEST_TSO4	|	\
>   	 1ULL << VIRTIO_NET_F_GUEST_TSO6	|	\
>   	 1ULL << VIRTIO_F_IN_ORDER		|	\
> -	 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 mrg_rxbuf, int in_order)
> +		     int mrg_rxbuf, int in_order, int packed_vq)
>   {
>   	pthread_mutex_init(&dev->mutex, NULL);
>   	snprintf(dev->path, PATH_MAX, "%s", path);
> @@ -432,6 +433,11 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>   		dev->unsupported_features |= (1ull << VIRTIO_F_IN_ORDER);
>   	}
>   
> +	if (packed_vq)
> +		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);
>   	} else {
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index d6e0e137b..7f46ba1d9 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -49,7 +49,7 @@ 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 mrg_rxbuf, int in_order);
> +			 int mrg_rxbuf, int in_order, int packed_vq);
>   void virtio_user_dev_uninit(struct virtio_user_dev *dev);
>   void virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx);
>   uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs);
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
> index 525d16cab..72ac86186 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -364,6 +364,8 @@ static const char *valid_args[] = {
>   	VIRTIO_USER_ARG_MRG_RXBUF,
>   #define VIRTIO_USER_ARG_IN_ORDER       "in_order"
>   	VIRTIO_USER_ARG_IN_ORDER,
> +#define VIRTIO_USER_ARG_PACKED_VQ "packed_vq"
> +	VIRTIO_USER_ARG_PACKED_VQ,
>   	NULL
>   };
>   
> @@ -473,6 +475,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>   	char *ifname = NULL;
>   	char *mac_addr = NULL;
>   	int ret = -1;
> +	uint64_t packed_vq = 0;
>   
>   	kvlist = rte_kvargs_parse(rte_vdev_device_args(dev), valid_args);
>   	if (!kvlist) {
> @@ -556,6 +559,15 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>   		cq = 1;
>   	}
>   
> +	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_PACKED_VQ) == 1) {
> +		if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_PACKED_VQ,
> +				       &get_integer_arg, &packed_vq) < 0) {
> +			PMD_INIT_LOG(ERR, "error to parse %s",
> +				     VIRTIO_USER_ARG_PACKED_VQ);
> +			goto end;
> +		}
> +	}
> +
>   	if (queues > 1 && cq == 0) {
>   		PMD_INIT_LOG(ERR, "multi-q requires ctrl-q");
>   		goto end;
> @@ -603,7 +615,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
>   			vu_dev->is_server = false;
>   		if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
>   				 queue_size, mac_addr, &ifname, mrg_rxbuf,
> -				 in_order) < 0) {
> +				 in_order, packed_vq) < 0) {
>   			PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
>   			virtio_user_eth_dev_free(eth_dev);
>   			goto end;
> 

[PATCH v5 07/11] net/virtio: implement transmit path for packed queues

[Jens Freimann]

This implements the transmit path for devices with
support for packed virtqueues.

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

Signed-off-by: Jens Freiman <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c |   8 +-
 drivers/net/virtio/virtio_ethdev.h |   2 +
 drivers/net/virtio/virtio_rxtx.c   | 113 ++++++++++++++++++++++++++++-
 3 files changed, 121 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index ad91f7f82..d2c5755bb 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -384,6 +384,8 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
 	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
@@ -1338,7 +1340,11 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 		eth_dev->rx_pkt_burst = &virtio_recv_pkts;
 	}
 
-	if (hw->use_inorder_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_inorder_tx) {
 		PMD_INIT_LOG(INFO, "virtio: using inorder Tx path on port %u",
 			eth_dev->data->port_id);
 		eth_dev->tx_pkt_burst = virtio_xmit_pkts_inorder;
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index b726ad108..04161b461 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -79,6 +79,8 @@ uint16_t virtio_recv_mergeable_pkts_inorder(void *rx_queue,
 
 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_xmit_pkts_inorder(void *tx_queue, struct rte_mbuf **tx_pkts,
 		uint16_t nb_pkts);
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index eb891433e..12787070e 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -38,6 +38,112 @@
 #define  VIRTIO_DUMP_PACKET(m, len) do { } while (0)
 #endif
 
+
+/* Cleanup from completed transmits. */
+static void
+virtio_xmit_cleanup_packed(struct virtqueue *vq)
+{
+	uint16_t idx;
+	uint16_t size = vq->vq_nentries;
+	struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
+	struct vq_desc_extra *dxp;
+
+	idx = vq->vq_used_cons_idx;
+	while (desc_is_used(&desc[idx], &vq->vq_ring) &&
+	       vq->vq_free_cnt < size) {
+		dxp = &vq->vq_descx[idx];
+		vq->vq_free_cnt += dxp->ndescs;
+		idx = dxp->ndescs;
+		idx = idx >= size ? idx - size : idx;
+	}
+}
+
+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, prev;
+	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_packed(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;
+		int descs_used;
+
+		if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
+			virtio_xmit_cleanup_packed(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;
+
+		wrap_counter = vq->vq_ring.avail_wrap_counter;
+		idx = vq->vq_avail_idx; 
+		head_idx = idx;
+
+		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 |
+			VRING_DESC_F_AVAIL(vq->vq_ring.avail_wrap_counter) |
+			VRING_DESC_F_USED(!vq->vq_ring.avail_wrap_counter);
+		descs_used = 1;
+
+		do {
+			idx = update_pq_avail_index(vq);
+			desc[idx].addr  = VIRTIO_MBUF_DATA_DMA_ADDR(txm, vq);
+			desc[idx].len   = txm->data_len;
+			desc[idx].flags = VRING_DESC_F_NEXT |
+				VRING_DESC_F_AVAIL(vq->vq_ring.avail_wrap_counter) |
+				VRING_DESC_F_USED(!vq->vq_ring.avail_wrap_counter);
+			descs_used++;
+		} while ((txm = txm->next) != NULL);
+
+		desc[idx].flags &= ~VRING_DESC_F_NEXT;
+
+		rte_smp_wmb();
+		prev = (idx > 0 ? idx : vq->vq_nentries) - 1;
+		desc[prev].index = head_idx; //FIXME
+		desc[head_idx].flags =
+			(VRING_DESC_F_AVAIL(wrap_counter) |
+			 VRING_DESC_F_USED(!wrap_counter));
+
+		vq->vq_descx[head_idx].ndescs = descs_used;
+		idx = update_pq_avail_index(vq);
+	}
+
+	txvq->stats.packets += i;
+	txvq->stats.errors  += nb_pkts - i;
+
+	return i;
+}
+
 int
 virtio_dev_rx_queue_done(void *rxq, uint16_t offset)
 {
@@ -736,7 +842,12 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
 	if (hw->use_inorder_tx)
 		vq->vq_ring.desc[vq->vq_nentries - 1].next = 0;
 
-	VIRTQUEUE_DUMP(vq);
+	if (vtpci_packed_queue(hw)) {
+		vq->vq_ring.avail_wrap_counter = 1;
+	}
+
+	if (!vtpci_packed_queue(hw))
+		VIRTQUEUE_DUMP(vq);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v5 07/11] net/virtio: implement transmit path for packed queues

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
> Sent: Friday, September 7, 2018 2:20 AM
> To: dev@dpdk.org
> Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
> Subject: [dpdk-dev] [PATCH v5 07/11] net/virtio: implement transmit path
> for packed queues
>
> This implements the transmit path for devices with support for packed
> virtqueues.
>
> Add the feature bit and enable code to
> add buffers to vring and mark descriptors as available.
>
> Signed-off-by: Jens Freiman <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c |   8 +-
>  drivers/net/virtio/virtio_ethdev.h |   2 +
>  drivers/net/virtio/virtio_rxtx.c   | 113 ++++++++++++++++++++++++++++-
>  3 files changed, 121 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index ad91f7f82..d2c5755bb 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -384,6 +384,8 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t
> vtpci_queue_idx)
>  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 @@ -1338,7 +1340,11 @@
> set_rxtx_funcs(struct rte_eth_dev *eth_dev)
>  eth_dev->rx_pkt_burst = &virtio_recv_pkts;
>  }
>
> -if (hw->use_inorder_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_inorder_tx) {
>  PMD_INIT_LOG(INFO, "virtio: using inorder Tx path on
> port %u",
>  eth_dev->data->port_id);
>  eth_dev->tx_pkt_burst = virtio_xmit_pkts_inorder; diff --git
> a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index b726ad108..04161b461 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -79,6 +79,8 @@ uint16_t virtio_recv_mergeable_pkts_inorder(void
> *rx_queue,
>
>  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_xmit_pkts_inorder(void *tx_queue, struct rte_mbuf
> **tx_pkts,
>  uint16_t nb_pkts);
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index eb891433e..12787070e 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -38,6 +38,112 @@
>  #define  VIRTIO_DUMP_PACKET(m, len) do { } while (0)  #endif
>
> +
> +/* Cleanup from completed transmits. */ static void
> +virtio_xmit_cleanup_packed(struct virtqueue *vq) {
> +uint16_t idx;
> +uint16_t size = vq->vq_nentries;
> +struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
> +struct vq_desc_extra *dxp;
> +
> +idx = vq->vq_used_cons_idx;
> +while (desc_is_used(&desc[idx], &vq->vq_ring) &&
> +       vq->vq_free_cnt < size) {
> +dxp = &vq->vq_descx[idx];
> +vq->vq_free_cnt += dxp->ndescs;
> +idx = dxp->ndescs;
> +idx = idx >= size ? idx - size : idx;
> +}
> +}
> +
> +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, prev;
> +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_packed(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;
> +int descs_used;
> +
> +if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
> +virtio_xmit_cleanup_packed(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;
> +
> +wrap_counter = vq->vq_ring.avail_wrap_counter;
> +idx = vq->vq_avail_idx;
> +head_idx = idx;
> +
> +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 |
> +VRING_DESC_F_AVAIL(vq-
> >vq_ring.avail_wrap_counter) |
> +VRING_DESC_F_USED(!vq-
> >vq_ring.avail_wrap_counter);
> +descs_used = 1;
> +
> +do {
> +idx = update_pq_avail_index(vq);
> +desc[idx].addr  =
> VIRTIO_MBUF_DATA_DMA_ADDR(txm, vq);
> +desc[idx].len   = txm->data_len;
> +desc[idx].flags = VRING_DESC_F_NEXT |
> +VRING_DESC_F_AVAIL(vq-
> >vq_ring.avail_wrap_counter) |
> +VRING_DESC_F_USED(!vq-
> >vq_ring.avail_wrap_counter);

According to spec, all the flags update should be moved after the memory barriers.

> +descs_used++;
> +} while ((txm = txm->next) != NULL);
> +
> +desc[idx].flags &= ~VRING_DESC_F_NEXT;

Ditto

> +
> +rte_smp_wmb();
> +prev = (idx > 0 ? idx : vq->vq_nentries) - 1;
> +desc[prev].index = head_idx; //FIXME
> +desc[head_idx].flags =
> +(VRING_DESC_F_AVAIL(wrap_counter) |
> + VRING_DESC_F_USED(!wrap_counter));
> +
> +vq->vq_descx[head_idx].ndescs = descs_used;
> +idx = update_pq_avail_index(vq);
> +}
> +
> +txvq->stats.packets += i;
> +txvq->stats.errors  += nb_pkts - i;
> +
> +return i;
> +}
> +
>  int
>  virtio_dev_rx_queue_done(void *rxq, uint16_t offset)  { @@ -736,7 +842,12
> @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>  if (hw->use_inorder_tx)
>  vq->vq_ring.desc[vq->vq_nentries - 1].next = 0;
>
> -VIRTQUEUE_DUMP(vq);
> +if (vtpci_packed_queue(hw)) {
> +vq->vq_ring.avail_wrap_counter = 1;
> +}
> +
> +if (!vtpci_packed_queue(hw))
> +VIRTQUEUE_DUMP(vq);
>
>  return 0;
>  }
> --
> 2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v5 07/11] net/virtio: implement transmit path for packed queues

[Gavin Hu (Arm Technology China)]

One more comment:

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
> Sent: Friday, September 7, 2018 2:20 AM
> To: dev@dpdk.org
> Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
> Subject: [dpdk-dev] [PATCH v5 07/11] net/virtio: implement transmit path
> for packed queues
>
> This implements the transmit path for devices with support for packed
> virtqueues.
>
> Add the feature bit and enable code to
> add buffers to vring and mark descriptors as available.
>
> Signed-off-by: Jens Freiman <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c |   8 +-
>  drivers/net/virtio/virtio_ethdev.h |   2 +
>  drivers/net/virtio/virtio_rxtx.c   | 113 ++++++++++++++++++++++++++++-
>  3 files changed, 121 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index ad91f7f82..d2c5755bb 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -384,6 +384,8 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t
> vtpci_queue_idx)
>  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 @@ -1338,7 +1340,11 @@
> set_rxtx_funcs(struct rte_eth_dev *eth_dev)
>  eth_dev->rx_pkt_burst = &virtio_recv_pkts;
>  }
>
> -if (hw->use_inorder_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_inorder_tx) {
>  PMD_INIT_LOG(INFO, "virtio: using inorder Tx path on
> port %u",
>  eth_dev->data->port_id);
>  eth_dev->tx_pkt_burst = virtio_xmit_pkts_inorder; diff --git
> a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index b726ad108..04161b461 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -79,6 +79,8 @@ uint16_t virtio_recv_mergeable_pkts_inorder(void
> *rx_queue,
>
>  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_xmit_pkts_inorder(void *tx_queue, struct rte_mbuf
> **tx_pkts,
>  uint16_t nb_pkts);
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index eb891433e..12787070e 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -38,6 +38,112 @@
>  #define  VIRTIO_DUMP_PACKET(m, len) do { } while (0)  #endif
>
> +
> +/* Cleanup from completed transmits. */ static void
> +virtio_xmit_cleanup_packed(struct virtqueue *vq) {
> +uint16_t idx;
> +uint16_t size = vq->vq_nentries;
> +struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
> +struct vq_desc_extra *dxp;
> +
> +idx = vq->vq_used_cons_idx;
> +while (desc_is_used(&desc[idx], &vq->vq_ring) &&
> +       vq->vq_free_cnt < size) {
> +dxp = &vq->vq_descx[idx];
> +vq->vq_free_cnt += dxp->ndescs;
> +idx = dxp->ndescs;

Should be "+=" here?

> +idx = idx >= size ? idx - size : idx;
> +}
> +}
> +
> +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, prev;
> +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_packed(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;
> +int descs_used;
> +
> +if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
> +virtio_xmit_cleanup_packed(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;
> +
> +wrap_counter = vq->vq_ring.avail_wrap_counter;
> +idx = vq->vq_avail_idx;
> +head_idx = idx;
> +
> +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 |
> +VRING_DESC_F_AVAIL(vq-
> >vq_ring.avail_wrap_counter) |
> +VRING_DESC_F_USED(!vq-
> >vq_ring.avail_wrap_counter);
> +descs_used = 1;
> +
> +do {
> +idx = update_pq_avail_index(vq);
> +desc[idx].addr  =
> VIRTIO_MBUF_DATA_DMA_ADDR(txm, vq);
> +desc[idx].len   = txm->data_len;
> +desc[idx].flags = VRING_DESC_F_NEXT |
> +VRING_DESC_F_AVAIL(vq-
> >vq_ring.avail_wrap_counter) |
> +VRING_DESC_F_USED(!vq-
> >vq_ring.avail_wrap_counter);
> +descs_used++;
> +} while ((txm = txm->next) != NULL);
> +
> +desc[idx].flags &= ~VRING_DESC_F_NEXT;
> +
> +rte_smp_wmb();
> +prev = (idx > 0 ? idx : vq->vq_nentries) - 1;
> +desc[prev].index = head_idx; //FIXME
> +desc[head_idx].flags =
> +(VRING_DESC_F_AVAIL(wrap_counter) |
> + VRING_DESC_F_USED(!wrap_counter));
> +
> +vq->vq_descx[head_idx].ndescs = descs_used;
> +idx = update_pq_avail_index(vq);
> +}
> +
> +txvq->stats.packets += i;
> +txvq->stats.errors  += nb_pkts - i;
> +
> +return i;
> +}
> +
>  int
>  virtio_dev_rx_queue_done(void *rxq, uint16_t offset)  { @@ -736,7 +842,12
> @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>  if (hw->use_inorder_tx)
>  vq->vq_ring.desc[vq->vq_nentries - 1].next = 0;
>
> -VIRTQUEUE_DUMP(vq);
> +if (vtpci_packed_queue(hw)) {
> +vq->vq_ring.avail_wrap_counter = 1;
> +}
> +
> +if (!vtpci_packed_queue(hw))
> +VIRTQUEUE_DUMP(vq);
>
>  return 0;
>  }
> --
> 2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v5 07/11] net/virtio: implement transmit path for packed queues

[Maxime Coquelin]

On 09/06/2018 08:19 PM, Jens Freimann wrote:
> This implements the transmit path for devices with
> support for packed virtqueues.
> 
> Add the feature bit and enable code to
> add buffers to vring and mark descriptors as available.
> 
> Signed-off-by: Jens Freiman <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c |   8 +-
>   drivers/net/virtio/virtio_ethdev.h |   2 +
>   drivers/net/virtio/virtio_rxtx.c   | 113 ++++++++++++++++++++++++++++-
>   3 files changed, 121 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index ad91f7f82..d2c5755bb 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -384,6 +384,8 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
>   	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
> @@ -1338,7 +1340,11 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
>   		eth_dev->rx_pkt_burst = &virtio_recv_pkts;
>   	}
>   
> -	if (hw->use_inorder_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_inorder_tx) {
>   		PMD_INIT_LOG(INFO, "virtio: using inorder Tx path on port %u",
>   			eth_dev->data->port_id);
>   		eth_dev->tx_pkt_burst = virtio_xmit_pkts_inorder;
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index b726ad108..04161b461 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -79,6 +79,8 @@ uint16_t virtio_recv_mergeable_pkts_inorder(void *rx_queue,
>   
>   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_xmit_pkts_inorder(void *tx_queue, struct rte_mbuf **tx_pkts,
>   		uint16_t nb_pkts);
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index eb891433e..12787070e 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -38,6 +38,112 @@
>   #define  VIRTIO_DUMP_PACKET(m, len) do { } while (0)
>   #endif
>   
> +
> +/* Cleanup from completed transmits. */
> +static void
> +virtio_xmit_cleanup_packed(struct virtqueue *vq)
> +{
> +	uint16_t idx;
> +	uint16_t size = vq->vq_nentries;
> +	struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
> +	struct vq_desc_extra *dxp;
> +
> +	idx = vq->vq_used_cons_idx;
> +	while (desc_is_used(&desc[idx], &vq->vq_ring) &&
> +	       vq->vq_free_cnt < size) {
> +		dxp = &vq->vq_descx[idx];
> +		vq->vq_free_cnt += dxp->ndescs;
> +		idx = dxp->ndescs;
> +		idx = idx >= size ? idx - size : idx;
> +	}
> +}
> +
> +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, prev;
> +	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_packed(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;
> +		int descs_used;
> +
> +		if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
> +			virtio_xmit_cleanup_packed(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;
> +
> +		wrap_counter = vq->vq_ring.avail_wrap_counter;
> +		idx = vq->vq_avail_idx;
> +		head_idx = idx;
> +
> +		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 |
> +			VRING_DESC_F_AVAIL(vq->vq_ring.avail_wrap_counter) |
> +			VRING_DESC_F_USED(!vq->vq_ring.avail_wrap_counter);
> +		descs_used = 1;
> +
> +		do {
> +			idx = update_pq_avail_index(vq);
> +			desc[idx].addr  = VIRTIO_MBUF_DATA_DMA_ADDR(txm, vq);
> +			desc[idx].len   = txm->data_len;
> +			desc[idx].flags = VRING_DESC_F_NEXT |
> +				VRING_DESC_F_AVAIL(vq->vq_ring.avail_wrap_counter) |
> +				VRING_DESC_F_USED(!vq->vq_ring.avail_wrap_counter);
> +			descs_used++;
> +		} while ((txm = txm->next) != NULL);
> +
> +		desc[idx].flags &= ~VRING_DESC_F_NEXT;
> +
> +		rte_smp_wmb();
> +		prev = (idx > 0 ? idx : vq->vq_nentries) - 1;
> +		desc[prev].index = head_idx; //FIXME

//FIXIT! :)

> +		desc[head_idx].flags =
> +			(VRING_DESC_F_AVAIL(wrap_counter) |
> +			 VRING_DESC_F_USED(!wrap_counter));
> +
> +		vq->vq_descx[head_idx].ndescs = descs_used;
> +		idx = update_pq_avail_index(vq);
> +	}
> +
> +	txvq->stats.packets += i;
> +	txvq->stats.errors  += nb_pkts - i;
> +
> +	return i;
> +}
> +
>   int
>   virtio_dev_rx_queue_done(void *rxq, uint16_t offset)
>   {
> @@ -736,7 +842,12 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>   	if (hw->use_inorder_tx)
>   		vq->vq_ring.desc[vq->vq_nentries - 1].next = 0;
>   
> -	VIRTQUEUE_DUMP(vq);
> +	if (vtpci_packed_queue(hw)) {
> +		vq->vq_ring.avail_wrap_counter = 1;
> +	}
> +
> +	if (!vtpci_packed_queue(hw))
> +		VIRTQUEUE_DUMP(vq);

I guess the check isn't necessary anymore since support is added in
patch 5.

>   
>   	return 0;
>   }
> 

Re: [PATCH v5 07/11] net/virtio: implement transmit path for packed queues

[Tiwei Bie]

On Thu, Sep 06, 2018 at 07:19:43PM +0100, Jens Freimann wrote:
> This implements the transmit path for devices with
> support for packed virtqueues.
> 
> Add the feature bit and enable code to
> add buffers to vring and mark descriptors as available.
> 
> Signed-off-by: Jens Freiman <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c |   8 +-
>  drivers/net/virtio/virtio_ethdev.h |   2 +
>  drivers/net/virtio/virtio_rxtx.c   | 113 ++++++++++++++++++++++++++++-
>  3 files changed, 121 insertions(+), 2 deletions(-)
[...]
> +
> +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, prev;
> +	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_packed(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;
> +		int descs_used;
> +
> +		if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
> +			virtio_xmit_cleanup_packed(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;
> +

We also need to update the stats by calling
virtio_update_packet_stats()

We also need to handle the offloads. See
virtqueue_xmit_offload().

> +		vq->vq_free_cnt -= txm->nb_segs + 1;
> +
> +		wrap_counter = vq->vq_ring.avail_wrap_counter;
> +		idx = vq->vq_avail_idx; 
> +		head_idx = idx;
> +
> +		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 |
> +			VRING_DESC_F_AVAIL(vq->vq_ring.avail_wrap_counter) |
> +			VRING_DESC_F_USED(!vq->vq_ring.avail_wrap_counter);
> +		descs_used = 1;
> +
> +		do {
> +			idx = update_pq_avail_index(vq);
> +			desc[idx].addr  = VIRTIO_MBUF_DATA_DMA_ADDR(txm, vq);
> +			desc[idx].len   = txm->data_len;
> +			desc[idx].flags = VRING_DESC_F_NEXT |
> +				VRING_DESC_F_AVAIL(vq->vq_ring.avail_wrap_counter) |
> +				VRING_DESC_F_USED(!vq->vq_ring.avail_wrap_counter);
> +			descs_used++;
> +		} while ((txm = txm->next) != NULL);
> +
> +		desc[idx].flags &= ~VRING_DESC_F_NEXT;
> +
> +		rte_smp_wmb();
> +		prev = (idx > 0 ? idx : vq->vq_nentries) - 1;
> +		desc[prev].index = head_idx; //FIXME
> +		desc[head_idx].flags =
> +			(VRING_DESC_F_AVAIL(wrap_counter) |
> +			 VRING_DESC_F_USED(!wrap_counter));
> +
> +		vq->vq_descx[head_idx].ndescs = descs_used;
> +		idx = update_pq_avail_index(vq);
> +	}
> +
> +	txvq->stats.packets += i;
> +	txvq->stats.errors  += nb_pkts - i;
> +
> +	return i;
> +}
> +
>  int
>  virtio_dev_rx_queue_done(void *rxq, uint16_t offset)
>  {
> @@ -736,7 +842,12 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>  	if (hw->use_inorder_tx)
>  		vq->vq_ring.desc[vq->vq_nentries - 1].next = 0;
>  
> -	VIRTQUEUE_DUMP(vq);
> +	if (vtpci_packed_queue(hw)) {
> +		vq->vq_ring.avail_wrap_counter = 1;
> +	}
> +
> +	if (!vtpci_packed_queue(hw))
> +		VIRTQUEUE_DUMP(vq);
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

[PATCH v5 08/11] net/virtio: implement receive path for packed queues

[Jens Freimann]

Implement the receive part.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c |  15 +++-
 drivers/net/virtio/virtio_ethdev.h |   2 +
 drivers/net/virtio/virtio_rxtx.c   | 131 +++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index d2c5755bb..a2bb726ba 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -384,8 +384,10 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
 	vq->hw = hw;
 	vq->vq_queue_index = vtpci_queue_idx;
 	vq->vq_nentries = vq_size;
-	if (vtpci_packed_queue(hw))
+	if (vtpci_packed_queue(hw)) {
 		vq->vq_ring.avail_wrap_counter = 1;
+		vq->vq_ring.used_wrap_counter = 1;
+	}
 
 	/*
 	 * Reserve a memzone for vring elements
@@ -1320,7 +1322,13 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
 
-	if (hw->use_simple_rx) {
+	/*
+	 * workarount for packed vqs which don't support
+	 * mrg_rxbuf at this point
+	 */
+	if (vtpci_packed_queue(hw)) {
+		eth_dev->rx_pkt_burst = &virtio_recv_pkts_packed;
+	} else if (hw->use_simple_rx) {
 		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;
@@ -1484,7 +1492,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 
 	/* 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 04161b461..25eaff224 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -70,6 +70,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 12787070e..3f5fa7366 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)
@@ -710,6 +711,34 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
 
 	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);
+		}
+		vq->vq_ring.avail_wrap_counter ^= 1;
+		nbufs = desc_idx;
+		goto out;
+	}
+
 	/* Allocate blank mbufs for the each rx descriptor */
 	nbufs = 0;
 
@@ -773,6 +802,7 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev *dev, uint16_t queue_idx)
 		vq_update_avail_idx(vq);
 	}
 
+out:
 	PMD_INIT_LOG(DEBUG, "Allocated %d bufs", nbufs);
 
 	VIRTQUEUE_DUMP(vq);
@@ -993,6 +1023,107 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
 	return 0;
 }
 
+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;
+	struct virtio_net_hdr *hdr;
+	struct vring_desc_packed *descs = vq->vq_ring.desc_packed;
+	struct vring_desc_packed *desc;
+	uint16_t used_idx, id;
+	struct vq_desc_extra *dxp;
+
+	nb_rx = 0;
+	if (unlikely(hw->started == 0))
+		return nb_rx;
+
+	hdr_size = hw->vtnet_hdr_size;
+
+	for (i = 0; i < nb_pkts; i++) {
+		rte_smp_rmb();
+		used_idx = vq->vq_used_cons_idx;
+		desc = &descs[used_idx];
+		id = desc->index;
+		if (!desc_is_used(desc, &vq->vq_ring))
+			break;
+
+		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[id];
+		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 (hw->has_rx_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();
+
+		rx_pkts[nb_rx++] = rxm;
+		vq->vq_used_cons_idx += dxp->ndescs;
+		if (vq->vq_used_cons_idx >= vq->vq_nentries) {
+			vq->vq_used_cons_idx -= vq->vq_nentries;
+			vq->vq_ring.used_wrap_counter ^= 1;
+		}
+	}
+
+	rxvq->stats.packets += nb_rx;
+
+	return nb_rx;
+}
+
 #define VIRTIO_MBUF_BURST_SZ 64
 #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_desc))
 uint16_t
-- 
2.17.1

Re: [PATCH v5 08/11] net/virtio: implement receive path for packed queues

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jens Freimann
> Sent: Friday, September 7, 2018 2:20 AM
> To: dev@dpdk.org
> Cc: tiwei.bie@intel.com; maxime.coquelin@redhat.com
> Subject: [dpdk-dev] [PATCH v5 08/11] net/virtio: implement receive path for
> packed queues
>
> Implement the receive part.
>
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c |  15 +++-
>  drivers/net/virtio/virtio_ethdev.h |   2 +
>  drivers/net/virtio/virtio_rxtx.c   | 131 +++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index d2c5755bb..a2bb726ba 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -384,8 +384,10 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t
> vtpci_queue_idx)
>  vq->hw = hw;
>  vq->vq_queue_index = vtpci_queue_idx;
>  vq->vq_nentries = vq_size;
> -if (vtpci_packed_queue(hw))
> +if (vtpci_packed_queue(hw)) {
>  vq->vq_ring.avail_wrap_counter = 1;
> +vq->vq_ring.used_wrap_counter = 1;
> +}
>
>  /*
>   * Reserve a memzone for vring elements @@ -1320,7 +1322,13 @@
> set_rxtx_funcs(struct rte_eth_dev *eth_dev)  {
>  struct virtio_hw *hw = eth_dev->data->dev_private;
>
> -if (hw->use_simple_rx) {
> +/*
> + * workarount for packed vqs which don't support
> + * mrg_rxbuf at this point
> + */
> +if (vtpci_packed_queue(hw)) {
> +eth_dev->rx_pkt_burst = &virtio_recv_pkts_packed;
> +} else if (hw->use_simple_rx) {
>  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; @@ -1484,7
> +1492,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t
> req_features)
>
>  /* 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 04161b461..25eaff224 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -70,6 +70,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 12787070e..3f5fa7366 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)
> @@ -710,6 +711,34 @@ virtio_dev_rx_queue_setup_finish(struct
> rte_eth_dev *dev, uint16_t queue_idx)
>
>  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);
> +}
> +vq->vq_ring.avail_wrap_counter ^= 1;
> +nbufs = desc_idx;
> +goto out;
> +}
> +
>  /* Allocate blank mbufs for the each rx descriptor */
>  nbufs = 0;
>
> @@ -773,6 +802,7 @@ virtio_dev_rx_queue_setup_finish(struct
> rte_eth_dev *dev, uint16_t queue_idx)
>  vq_update_avail_idx(vq);
>  }
>
> +out:
>  PMD_INIT_LOG(DEBUG, "Allocated %d bufs", nbufs);
>
>  VIRTQUEUE_DUMP(vq);
> @@ -993,6 +1023,107 @@ virtio_rx_offload(struct rte_mbuf *m, struct
> virtio_net_hdr *hdr)
>  return 0;
>  }
>
> +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;
> +struct virtio_net_hdr *hdr;
> +struct vring_desc_packed *descs = vq->vq_ring.desc_packed;
> +struct vring_desc_packed *desc;
> +uint16_t used_idx, id;
> +struct vq_desc_extra *dxp;
> +
> +nb_rx = 0;
> +if (unlikely(hw->started == 0))
> +return nb_rx;
> +
> +hdr_size = hw->vtnet_hdr_size;
> +
> +for (i = 0; i < nb_pkts; i++) {
> +rte_smp_rmb();
> +used_idx = vq->vq_used_cons_idx;
> +desc = &descs[used_idx];
> +id = desc->index;
> +if (!desc_is_used(desc, &vq->vq_ring))
> +break;
> +
> +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[id];
> +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;


There should be a wmb here?

> +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 (hw->has_rx_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();
What's this wmb for?
> +
> +rx_pkts[nb_rx++] = rxm;
> +vq->vq_used_cons_idx += dxp->ndescs;
> +if (vq->vq_used_cons_idx >= vq->vq_nentries) {
> +vq->vq_used_cons_idx -= vq->vq_nentries;
> +vq->vq_ring.used_wrap_counter ^= 1;
> +}
> +}
> +
> +rxvq->stats.packets += nb_rx;
> +
> +return nb_rx;
> +}
> +
>  #define VIRTIO_MBUF_BURST_SZ 64
>  #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct
> vring_desc))  uint16_t
> --
> 2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v5 09/11] net/virtio: disable ctrl virtqueue for packed rings

[Jens Freimann]

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

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index a2bb726ba..b02c65598 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1161,6 +1161,15 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
 			req_features &= ~(1ULL << VIRTIO_NET_F_MTU);
 	}
 
+#ifdef RTE_LIBRTE_VIRTIO_PQ
+	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);
+	}
+#endif
+
 	/*
 	 * Negotiate features: Subset of device feature bits are written back
 	 * guest feature bits.
-- 
2.17.1

Re: [PATCH v5 09/11] net/virtio: disable ctrl virtqueue for packed rings

[Tiwei Bie]

On Thu, Sep 06, 2018 at 07:19:45PM +0100, Jens Freimann wrote:
> Signed-off-by: Jens Freiman <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index a2bb726ba..b02c65598 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1161,6 +1161,15 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
>  			req_features &= ~(1ULL << VIRTIO_NET_F_MTU);
>  	}
>  
> +#ifdef RTE_LIBRTE_VIRTIO_PQ
> +	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);
> +	}
> +#endif

I think we need to support ctrl vq.

And in performance test, we need to use more cores on vhost
side to make sure that we can get the max performance of the
virtio PMD. Otherwise, it's likely that the performance gain
we get is the gain in vhost.

> +
>  	/*
>  	 * Negotiate features: Subset of device feature bits are written back
>  	 * guest feature bits.
> -- 
> 2.17.1
> 

Re: [PATCH v5 09/11] net/virtio: disable ctrl virtqueue for packed rings

[Jens Freimann]

On Fri, Sep 14, 2018 at 01:32:36PM +0800, Tiwei Bie wrote:
>On Thu, Sep 06, 2018 at 07:19:45PM +0100, Jens Freimann wrote:
>> Signed-off-by: Jens Freiman <jfreimann@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_ethdev.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index a2bb726ba..b02c65598 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1161,6 +1161,15 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
>>  			req_features &= ~(1ULL << VIRTIO_NET_F_MTU);
>>  	}
>>
>> +#ifdef RTE_LIBRTE_VIRTIO_PQ
>> +	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);
>> +	}
>> +#endif
>
>I think we need to support ctrl vq.

I will add it to the next version.
>
>And in performance test, we need to use more cores on vhost
>side to make sure that we can get the max performance of the
>virtio PMD. Otherwise, it's likely that the performance gain
>we get is the gain in vhost.

ok.

thanks!

regards,
Jens 

[PATCH v5 10/11] 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 |   4 --
 drivers/net/virtio/virtio_rxtx.c   | 103 +++++++++++++++++++++++++++--
 drivers/net/virtio/virtqueue.h     |   1 +
 3 files changed, 98 insertions(+), 10 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b02c65598..f2e515838 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1331,10 +1331,6 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
 
-	/*
-	 * workarount for packed vqs which don't support
-	 * mrg_rxbuf at this point
-	 */
 	if (vtpci_packed_queue(hw)) {
 		eth_dev->rx_pkt_burst = &virtio_recv_pkts_packed;
 	} else if (hw->use_simple_rx) {
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 3f5fa7366..577786b7e 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -195,6 +195,79 @@ vq_ring_free_chain(struct virtqueue *vq, uint16_t desc_idx)
 	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];
+
+	dxp = &vq->vq_descx[used_idx];
+
+	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 = vq->vq_used_cons_idx;
+		desc = &vq->vq_ring.desc_packed[used_idx];
+		if (!desc_is_used(desc, &vq->vq_ring))
+			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 == 0)
+			vq->vq_ring.used_wrap_counter ^= 1;
+		set_desc_avail(&vq->vq_ring, desc);
+		vq->vq_used_cons_idx = increment_pq_index(vq->vq_used_cons_idx,
+							  vq->vq_nentries);
+	}
+
+	return i;
+}
+
 static uint16_t
 virtqueue_dequeue_burst_rx(struct virtqueue *vq, struct rte_mbuf **rx_pkts,
 			   uint32_t *len, uint16_t num)
@@ -1436,12 +1509,16 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 	uint16_t extra_idx;
 	uint32_t seg_res;
 	uint32_t hdr_size;
+	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;
+	else
+		nb_used = VIRTQUEUE_NUSED(vq);
 
 	virtio_rmb();
 
@@ -1454,13 +1531,21 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 	seg_res = 0;
 	hdr_size = hw->vtnet_hdr_size;
 
+	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 (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;
 
@@ -1513,9 +1598,13 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 			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 {
@@ -1559,6 +1648,9 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 
 	rxvq->stats.packets += nb_rx;
 
+	if (vtpci_packed_queue(vq->hw))
+		return nb_rx;
+
 	/* Allocate new mbuf for the used descriptor */
 	while (likely(!virtqueue_full(vq))) {
 		new_mbuf = rte_mbuf_raw_alloc(rxvq->mpool);
@@ -1578,7 +1670,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 
 	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 531ba8c65..735066486 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 */
-- 
2.17.1

[PATCH v5 11/11] net/virtio: add support for event suppression

[Jens Freimann]

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

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index f2e515838..4249e52c7 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -730,7 +730,7 @@ virtio_dev_rx_queue_intr_enable(struct rte_eth_dev *dev, uint16_t queue_id)
 	struct virtnet_rx *rxvq = dev->data->rx_queues[queue_id];
 	struct virtqueue *vq = rxvq->vq;
 
-	virtqueue_enable_intr(vq);
+	virtqueue_enable_intr(vq, 0, 0);
 	return 0;
 }
 
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 577786b7e..5dee3f12b 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -137,6 +137,10 @@ virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
 
 		vq->vq_descx[head_idx].ndescs = descs_used;
 		idx = update_pq_avail_index(vq);
+		if (unlikely(virtqueue_kick_prepare_packed(vq))) {
+			virtqueue_notify(vq);
+			PMD_RX_LOG(DEBUG, "Notified");
+		}
 	}
 
 	txvq->stats.packets += i;
@@ -1193,6 +1197,10 @@ virtio_recv_pkts_packed(void *rx_queue, struct rte_mbuf **rx_pkts,
 	}
 
 	rxvq->stats.packets += nb_rx;
+	if (nb_rx > 0 && unlikely(virtqueue_kick_prepare_packed(vq))) {
+		virtqueue_notify(vq);
+		PMD_RX_LOG(DEBUG, "Notified");
+	}
 
 	return nb_rx;
 }
@@ -1648,8 +1656,13 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 
 	rxvq->stats.packets += nb_rx;
 
-	if (vtpci_packed_queue(vq->hw))
+	if (vtpci_packed_queue(vq->hw)) {
+		if (unlikely(virtqueue_kick_prepare(vq))) {
+			virtqueue_notify(vq);
+			PMD_RX_LOG(DEBUG, "Notified");
+		}
 		return nb_rx;
+	}
 
 	/* Allocate new mbuf for the used descriptor */
 	while (likely(!virtqueue_full(vq))) {
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 735066486..9d3e322a2 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -176,6 +176,8 @@ struct virtqueue {
 	uint16_t vq_free_cnt;  /**< num of desc available */
 	uint16_t vq_avail_idx; /**< sync until needed */
 	uint16_t vq_free_thresh; /**< free threshold */
+	uint16_t vq_signalled_avail;
+	int vq_signalled_avail_valid;
 
 	void *vq_ring_virt_mem;  /**< linear address of vring*/
 	unsigned int vq_ring_size;
@@ -292,16 +294,37 @@ vring_desc_init(struct vring_desc *dp, uint16_t n)
 static inline void
 virtqueue_disable_intr(struct virtqueue *vq)
 {
-	vq->vq_ring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
+	if (vtpci_packed_queue(vq->hw) && vtpci_with_feature(vq->hw,
+				VIRTIO_RING_F_EVENT_IDX))
+		vq->vq_ring.device_event->desc_event_flags =
+			RING_EVENT_FLAGS_DISABLE;
+	else
+		vq->vq_ring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
 }
 
 /**
  * Tell the backend to interrupt us.
  */
 static inline void
-virtqueue_enable_intr(struct virtqueue *vq)
+virtqueue_enable_intr(struct virtqueue *vq, uint16_t off, uint16_t wrap_counter)
 {
-	vq->vq_ring.avail->flags &= (~VRING_AVAIL_F_NO_INTERRUPT);
+	uint16_t *flags = &vq->vq_ring.device_event->desc_event_flags;
+	uint16_t *event_off_wrap =
+		&vq->vq_ring.device_event->desc_event_off_wrap;
+	if (vtpci_packed_queue(vq->hw)) {
+		*flags = 0;
+		*event_off_wrap = 0;
+		if (*event_off_wrap & RING_EVENT_FLAGS_DESC) {
+			*event_off_wrap = off | 0x7FFF;
+			*event_off_wrap |= wrap_counter << 15;
+			*flags |= RING_EVENT_FLAGS_DESC;
+		} else {
+			*event_off_wrap = 0;
+		}
+		*flags |= RING_EVENT_FLAGS_ENABLE;
+	} else {
+		vq->vq_ring.avail->flags &= (~VRING_AVAIL_F_NO_INTERRUPT);
+	}
 }
 
 /**
@@ -363,12 +386,60 @@ vq_update_avail_ring(struct virtqueue *vq, uint16_t desc_idx)
 	vq->vq_avail_idx++;
 }
 
+static int vhost_idx_diff(struct virtqueue *vq, uint16_t old, uint16_t new)
+{
+	if (new > old)
+		return new - old;
+	return  (new + vq->vq_nentries - old);
+}
+
+static int vring_packed_need_event(struct virtqueue *vq,
+		uint16_t event_off, uint16_t new,
+		uint16_t old)
+{
+	return (uint16_t)(vhost_idx_diff(vq, new, event_off) - 1) <
+		(uint16_t)vhost_idx_diff(vq, new, old);
+}
+
+
 static inline int
 virtqueue_kick_prepare(struct virtqueue *vq)
 {
 	return !(vq->vq_ring.used->flags & VRING_USED_F_NO_NOTIFY);
 }
 
+static inline int
+virtqueue_kick_prepare_packed(struct virtqueue *vq)
+{
+	uint16_t notify_offset, flags, wrap;
+	uint16_t old, new;
+	int v;
+
+	if (vtpci_packed_queue(vq->hw)) {
+		flags = vq->vq_ring.device_event->desc_event_flags;
+		if (!(flags & RING_EVENT_FLAGS_DESC))
+			return flags & RING_EVENT_FLAGS_ENABLE;
+		virtio_rmb();
+		notify_offset = vq->vq_ring.device_event->desc_event_off_wrap;
+		wrap = notify_offset & 0x1;
+		notify_offset >>= 1;
+
+		old = vq->vq_signalled_avail;
+		v = vq->vq_signalled_avail_valid;
+		new = vq->vq_avail_idx;
+		vq->vq_signalled_avail = vq->vq_avail_idx;
+		vq->vq_signalled_avail_valid = 1;
+
+		if (unlikely(!v))
+			return 0;
+
+		return (vring_packed_need_event(vq, new, old, notify_offset) &&
+			wrap == vq->vq_ring.avail_wrap_counter);
+	} else {
+		return !(vq->vq_ring.used->flags & VRING_USED_F_NO_NOTIFY);
+	}
+}
+
 static inline void
 virtqueue_notify(struct virtqueue *vq)
 {
-- 
2.17.1