From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 1081A2BBD for ; Fri, 5 Oct 2018 10:10:33 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0BF28307D868; Fri, 5 Oct 2018 08:10:33 +0000 (UTC) Received: from localhost (dhcp-192-241.str.redhat.com [10.33.192.241]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8888816D31; Fri, 5 Oct 2018 08:10:29 +0000 (UTC) Date: Fri, 5 Oct 2018 10:10:28 +0200 From: Jens Freimann To: Maxime Coquelin Cc: dev@dpdk.org, tiwei.bie@intel.com, Gavin.Hu@arm.com Message-ID: <20181005081028.ee2jkmaqsa7kagaa@jenstp.localdomain> References: <20181003131118.21491-1-jfreimann@redhat.com> <20181003131118.21491-2-jfreimann@redhat.com> <458553bf-260a-d63d-46e3-69b8c676d73f@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <458553bf-260a-d63d-46e3-69b8c676d73f@redhat.com> User-Agent: NeoMutt/20180716 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Fri, 05 Oct 2018 08:10:33 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v7 1/8] net/virtio: vring init for packed queues X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Oct 2018 08:10:34 -0000 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 >>--- >> 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