From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 08354A00E6 for ; Tue, 19 Mar 2019 10:44:45 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6B0134CC0; Tue, 19 Mar 2019 10:44:38 +0100 (CET) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 3163F4CC7 for ; Tue, 19 Mar 2019 10:44:37 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8EDAD30FE5B5; Tue, 19 Mar 2019 09:44:36 +0000 (UTC) Received: from localhost (dhcp-192-225.str.redhat.com [10.33.192.225]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0F9EC4D70F; Tue, 19 Mar 2019 09:44:33 +0000 (UTC) Date: Tue, 19 Mar 2019 10:44:32 +0100 From: Jens Freimann To: Tiwei Bie Cc: maxime.coquelin@redhat.com, zhihong.wang@intel.com, dev@dpdk.org Message-ID: <20190319094432.iap4i7ffs6soukr7@jenstp.localdomain> References: <20190319064312.13743-1-tiwei.bie@intel.com> <20190319064312.13743-6-tiwei.bie@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8"; format="flowed" Content-Disposition: inline In-Reply-To: <20190319064312.13743-6-tiwei.bie@intel.com> User-Agent: NeoMutt/20180716 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Tue, 19 Mar 2019 09:44:36 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH 05/10] net/virtio: refactor virtqueue structure 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190319094432.QfaEDjeaTbL72Kn-ep2wuCNO4IbcMJRj3QbUyzZGrWY@z> On Tue, Mar 19, 2019 at 02:43:07PM +0800, Tiwei Bie wrote: >Put split ring and packed ring specific fields into separate >sub-structures, and also union them as they won't be available >at the same time. > >Signed-off-by: Tiwei Bie >--- > drivers/net/virtio/virtio_ethdev.c | 71 +++++++++--------- > drivers/net/virtio/virtio_rxtx.c | 66 ++++++++--------- > drivers/net/virtio/virtio_rxtx_simple.h | 2 +- > drivers/net/virtio/virtio_rxtx_simple_neon.c | 2 +- > drivers/net/virtio/virtio_rxtx_simple_sse.c | 2 +- > drivers/net/virtio/virtqueue.c | 6 +- > drivers/net/virtio/virtqueue.h | 77 +++++++++++--------- > 7 files changed, 117 insertions(+), 109 deletions(-) > [snip] ... >diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h >index 80c0c43c3..48b3912e6 100644 >--- a/drivers/net/virtio/virtqueue.h >+++ b/drivers/net/virtio/virtqueue.h >@@ -191,17 +191,22 @@ struct vq_desc_extra { > > struct virtqueue { > struct virtio_hw *hw; /**< virtio_hw structure pointer. */ >- struct vring vq_ring; /**< vring keeping desc, used and avail */ >- struct vring_packed ring_packed; /**< vring keeping descs */ >- bool used_wrap_counter; >- uint16_t cached_flags; /**< cached flags for descs */ >- uint16_t event_flags_shadow; >+ union { >+ struct { >+ /**< vring keeping desc, used and avail */ >+ struct vring ring; >+ } vq_split; > >- /** >- * Last consumed descriptor in the used table, >- * trails vq_ring.used->idx. >- */ >- uint16_t vq_used_cons_idx; >+ struct { >+ /**< vring keeping descs and events */ >+ struct vring_packed ring; >+ bool used_wrap_counter; >+ uint16_t cached_flags; /**< cached flags for descs */ >+ uint16_t event_flags_shadow; >+ } vq_packed; >+ }; >+ >+ uint16_t vq_used_cons_idx; /**< last consumed descriptor */ > uint16_t vq_nentries; /**< vring desc numbers */ > uint16_t vq_free_cnt; /**< num of desc available */ > uint16_t vq_avail_idx; /**< sync until needed */ Honest question: What do we really gain by putting it in a union? We save a little memory. But we also make code less readable IMHO. If we do this, can we at least shorten some variable names, like drop the vq_ prefix? (It's used everywhere like vq->vq_packed*, so with vq->packed* we don't loose any context). I'm not strictly against this change but I'm wondering if it's worth it. regards, Jens