[dpdk-dev] [PATCH v7 0/8] implement packed virtqueues

[dpdk-dev] [PATCH v7 0/8] implement packed virtqueues

[Jens Freimann]

I'm sending this out to get some comments especially on the TX path code. 

I added support for mergeable rx buffers, out of order processing and
indirect. The receive path works well but the TX path sometimes locks up
after a random number of packets transmitted, so please review this
patch extra careful. This is also why I didn't add any new performance
numbers to the cover letter yet.

To support out-of-order processing I used the vq_desc_extra struct to
add a .next field and use it as a list for managing descriptors. This
seemed to add less complexity to the code than adding a new data
structure to use as a list for packed queue descriptors. 

I also took out the patch for supporting virtio-user as it turned out
more complex than expected. I will try to get it working for the next
version, but if I don't can we add it in a later pach set (and then
probably not in 18.11?]

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

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 pointers.

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.


Jens Freimann (8):
  net/virtio: vring init for packed queues
  net/virtio: add packed virtqueue defines
  net/virtio: add packed virtqueue helpers
  net/virtio: dump packed virtqueue data
  net/virtio: implement transmit path for packed queues
  net/virtio: implement receive path for packed queues
  net/virtio: add virtio send command packed queue support
  net/virtio: enable packed virtqueues by default

 drivers/net/virtio/virtio_ethdev.c | 161 +++++++--
 drivers/net/virtio/virtio_ethdev.h |   5 +
 drivers/net/virtio/virtio_pci.h    |   8 +
 drivers/net/virtio/virtio_ring.h   |  96 ++++-
 drivers/net/virtio/virtio_rxtx.c   | 544 ++++++++++++++++++++++++++++-
 drivers/net/virtio/virtqueue.c     |  23 ++
 drivers/net/virtio/virtqueue.h     |  52 ++-
 7 files changed, 846 insertions(+), 43 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v7 1/8] net/virtio: vring init for packed queues

[Jens Freimann]

Add and initialize descriptor data structures.

To allow out of order processing a .next field was added to
struct vq_desc_extra because there is none in the packed virtqueue
descriptor itself. This is used to chain descriptors and process them
similiar to how it is handled for split virtqueues.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 28 +++++++++------
 drivers/net/virtio/virtio_pci.h    |  8 +++++
 drivers/net/virtio/virtio_ring.h   | 55 +++++++++++++++++++++++++++---
 drivers/net/virtio/virtqueue.h     | 13 ++++++-
 4 files changed, 88 insertions(+), 16 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b81df0a99..d6a1613dd 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -299,19 +299,27 @@ 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);
+	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;
 	vq->vq_desc_head_idx = 0;
-	vq->vq_avail_idx = 0;
 	vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
-	vq->vq_free_cnt = vq->vq_nentries;
-	memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
+	if (vtpci_packed_queue(vq->hw)) {
+		uint16_t i;
+		for(i = 0; i < size - 1; i++) {
+			vq->vq_descx[i].next = i + 1;
+			vq->vq_ring.desc_packed[i].index = i;
+		} 
+		vq->vq_ring.desc_packed[i].index = i;
+		vq->vq_descx[i].next = VQ_RING_DESC_CHAIN_END;
+	} else {
 
-	vring_desc_init(vr->desc, size);
+		vring_desc_init_split(vr->desc, size);
+	}
 
 	/*
 	 * Disable device(host) interrupting guest
@@ -386,7 +394,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);
@@ -489,7 +497,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
 		for (i = 0; i < vq_size; i++) {
 			struct vring_desc *start_dp = txr[i].tx_indir;
 
-			vring_desc_init(start_dp, RTE_DIM(txr[i].tx_indir));
+			vring_desc_init_split(start_dp, RTE_DIM(txr[i].tx_indir));
 
 			/* first indirect descriptor is always the tx header */
 			start_dp->addr = txvq->virtio_net_hdr_mem
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..309069fdb 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;
+} __attribute__ ((aligned(16)));
+
+#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..6a4f92b79 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -161,6 +161,7 @@ struct virtio_pmd_ctrl {
 struct vq_desc_extra {
 	void *cookie;
 	uint16_t ndescs;
+	uint16_t next;
 };
 
 struct virtqueue {
@@ -245,9 +246,19 @@ 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)
+vring_desc_init_split(struct vring_desc *dp, uint16_t n)
 {
 	uint16_t i;
 
-- 
2.17.1

[dpdk-dev] [PATCH v7 2/8] net/virtio: add packed virtqueue 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 309069fdb..36a65f9b3 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

[dpdk-dev] [PATCH v7 3/8] 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   | 11 +++++++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h
index 36a65f9b3..b9e63d4d4 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 6a4f92b79..b55ace958 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -246,6 +246,17 @@ struct virtio_tx_region {
 			   __attribute__((__aligned__(16)));
 };
 
+static inline uint16_t
+update_pq_avail_index(struct virtqueue *vq)
+{
+	if (++vq->vq_avail_idx >= vq->vq_nentries) {
+		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)
 {
-- 
2.17.1

Re: [dpdk-dev] [PATCH v7 1/8] net/virtio: vring init for packed queues

[Jens Freimann]

On Thu, Oct 04, 2018 at 01:54:00PM +0200, Maxime Coquelin wrote:
>On 10/03/2018 03:11 PM, Jens Freimann wrote:
>>Add and initialize descriptor data structures.
>>
>>To allow out of order processing a .next field was added to
>>struct vq_desc_extra because there is none in the packed virtqueue
>>descriptor itself. This is used to chain descriptors and process them
>>similiar to how it is handled for split virtqueues.
>>
>>Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>>---
>>  drivers/net/virtio/virtio_ethdev.c | 28 +++++++++------
>>  drivers/net/virtio/virtio_pci.h    |  8 +++++
>>  drivers/net/virtio/virtio_ring.h   | 55 +++++++++++++++++++++++++++---
>>  drivers/net/virtio/virtqueue.h     | 13 ++++++-
>>  4 files changed, 88 insertions(+), 16 deletions(-)
>>
>>diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>>index b81df0a99..d6a1613dd 100644
>>--- a/drivers/net/virtio/virtio_ethdev.c
>>+++ b/drivers/net/virtio/virtio_ethdev.c
>>@@ -299,19 +299,27 @@ 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);
>>+	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;
>>  	vq->vq_desc_head_idx = 0;
>>-	vq->vq_avail_idx = 0;
>>  	vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
>>-	vq->vq_free_cnt = vq->vq_nentries;
>>-	memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
>>+	if (vtpci_packed_queue(vq->hw)) {
>>+		uint16_t i;
>>+		for(i = 0; i < size - 1; i++) {
>>+			vq->vq_descx[i].next = i + 1;
>
>I would move it in a dedicated loop, and do it only if IN_ORDER hasn't
>been negotiated. Not for performance reason of course, but just to
>highlight that this extra stuff isn't needed with in-order.

makes sense, I'll change it. 
>
>>+			vq->vq_ring.desc_packed[i].index = i;
>
>I would use the vring_desc_init_packed function declared below instead.

ok
>
>>+		}
>
>Trailing space.
[...]
>>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
>Isn't that feature already declared?

Yes, it is. I'll remove it here.

[...].

>>  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;
>>+	}
>>+
>
>As a general comment, I would find it cleaner to have dedicated
>functions for split and packed variants, like vring_init_split,
>vring_init_packed, etc...

ok, no problem.

>>  	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..6a4f92b79 100644
>>--- a/drivers/net/virtio/virtqueue.h
>>+++ b/drivers/net/virtio/virtqueue.h
>>@@ -161,6 +161,7 @@ struct virtio_pmd_ctrl {
>>  struct vq_desc_extra {
>>  	void *cookie;
>>  	uint16_t ndescs;
>>+	uint16_t next;
>>  };
>>  struct virtqueue {
>>@@ -245,9 +246,19 @@ 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;
>>+	}
>>+}
>
>I see the split variant is also called to init the indirect tables.
>Do you confirm this isn't needed in the case of packed ring?

Yes, the split variant just chains descriptors in the indirect table. For
packed virtqueues we don't need to do this according to the spec.  

Thanks for the review!

regards,
Jens 

Re: [dpdk-dev] [PATCH v7 1/8] net/virtio: vring init for packed queues

[Maxime Coquelin]

On 10/03/2018 03:11 PM, Jens Freimann wrote:
> Add and initialize descriptor data structures.
> 
> To allow out of order processing a .next field was added to
> struct vq_desc_extra because there is none in the packed virtqueue
> descriptor itself. This is used to chain descriptors and process them
> similiar to how it is handled for split virtqueues.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 28 +++++++++------
>   drivers/net/virtio/virtio_pci.h    |  8 +++++
>   drivers/net/virtio/virtio_ring.h   | 55 +++++++++++++++++++++++++++---
>   drivers/net/virtio/virtqueue.h     | 13 ++++++-
>   4 files changed, 88 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index b81df0a99..d6a1613dd 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -299,19 +299,27 @@ 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);
> +	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;
>   	vq->vq_desc_head_idx = 0;
> -	vq->vq_avail_idx = 0;
>   	vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
> -	vq->vq_free_cnt = vq->vq_nentries;
> -	memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
> +	if (vtpci_packed_queue(vq->hw)) {
> +		uint16_t i;
> +		for(i = 0; i < size - 1; i++) {
> +			vq->vq_descx[i].next = i + 1;

I would move it in a dedicated loop, and do it only if IN_ORDER hasn't
been negotiated. Not for performance reason of course, but just to
highlight that this extra stuff isn't needed with in-order.

> +			vq->vq_ring.desc_packed[i].index = i;

I would use the vring_desc_init_packed function declared below instead.

> +		}

Trailing space.

> +		vq->vq_ring.desc_packed[i].index = i;
> +		vq->vq_descx[i].next = VQ_RING_DESC_CHAIN_END;
> +	} else {
>   
> -	vring_desc_init(vr->desc, size);
> +		vring_desc_init_split(vr->desc, size);
> +	}
>   
>   	/*
>   	 * Disable device(host) interrupting guest
> @@ -386,7 +394,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);
> @@ -489,7 +497,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
>   		for (i = 0; i < vq_size; i++) {
>   			struct vring_desc *start_dp = txr[i].tx_indir;
>   
> -			vring_desc_init(start_dp, RTE_DIM(txr[i].tx_indir));
> +			vring_desc_init_split(start_dp, RTE_DIM(txr[i].tx_indir));
>   
>   			/* first indirect descriptor is always the tx header */
>   			start_dp->addr = txvq->virtio_net_hdr_mem
> 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
Isn't that feature already declared?

>   
>   /*
>    * 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..309069fdb 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;
> +} __attribute__ ((aligned(16)));
> +
> +#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;
> +	}
> +

As a general comment, I would find it cleaner to have dedicated
functions for split and packed variants, like vring_init_split,
vring_init_packed, etc...


>   	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..6a4f92b79 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -161,6 +161,7 @@ struct virtio_pmd_ctrl {
>   struct vq_desc_extra {
>   	void *cookie;
>   	uint16_t ndescs;
> +	uint16_t next;
>   };
>   
>   struct virtqueue {
> @@ -245,9 +246,19 @@ 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;
> +	}
> +}

I see the split variant is also called to init the indirect tables.
Do you confirm this isn't needed in the case of packed ring?

> +
>   /* Chain all the descriptors in the ring with an END */
>   static inline void
> -vring_desc_init(struct vring_desc *dp, uint16_t n)
> +vring_desc_init_split(struct vring_desc *dp, uint16_t n)
>   {
>   	uint16_t i;
>   
> 

[dpdk-dev] [PATCH v7 1/8] net/virtio: vring init for packed queues

[Jens Freimann]

Add and initialize descriptor data structures.

To allow out of order processing a .next field was added to
struct vq_desc_extra because there is none in the packed virtqueue
descriptor itself. This is used to chain descriptors and process them
similiar to how it is handled for split virtqueues.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 28 +++++++++------
 drivers/net/virtio/virtio_pci.h    |  8 +++++
 drivers/net/virtio/virtio_ring.h   | 55 +++++++++++++++++++++++++++---
 drivers/net/virtio/virtqueue.h     | 13 ++++++-
 4 files changed, 88 insertions(+), 16 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b81df0a99..d6a1613dd 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -299,19 +299,27 @@ 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);
+	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;
 	vq->vq_desc_head_idx = 0;
-	vq->vq_avail_idx = 0;
 	vq->vq_desc_tail_idx = (uint16_t)(vq->vq_nentries - 1);
-	vq->vq_free_cnt = vq->vq_nentries;
-	memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
+	if (vtpci_packed_queue(vq->hw)) {
+		uint16_t i;
+		for(i = 0; i < size - 1; i++) {
+			vq->vq_descx[i].next = i + 1;
+			vq->vq_ring.desc_packed[i].index = i;
+		} 
+		vq->vq_ring.desc_packed[i].index = i;
+		vq->vq_descx[i].next = VQ_RING_DESC_CHAIN_END;
+	} else {
 
-	vring_desc_init(vr->desc, size);
+		vring_desc_init_split(vr->desc, size);
+	}
 
 	/*
 	 * Disable device(host) interrupting guest
@@ -386,7 +394,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);
@@ -489,7 +497,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
 		for (i = 0; i < vq_size; i++) {
 			struct vring_desc *start_dp = txr[i].tx_indir;
 
-			vring_desc_init(start_dp, RTE_DIM(txr[i].tx_indir));
+			vring_desc_init_split(start_dp, RTE_DIM(txr[i].tx_indir));
 
 			/* first indirect descriptor is always the tx header */
 			start_dp->addr = txvq->virtio_net_hdr_mem
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..309069fdb 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;
+} __attribute__ ((aligned(16)));
+
+#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..6a4f92b79 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -161,6 +161,7 @@ struct virtio_pmd_ctrl {
 struct vq_desc_extra {
 	void *cookie;
 	uint16_t ndescs;
+	uint16_t next;
 };
 
 struct virtqueue {
@@ -245,9 +246,19 @@ 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)
+vring_desc_init_split(struct vring_desc *dp, uint16_t n)
 {
 	uint16_t i;
 
-- 
2.17.1