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

[Jens Freimann <jfreimann@redhat.com>]

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