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 9CC17A3 for ; Tue, 19 Mar 2019 16:00:29 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AD3913087BCA; Tue, 19 Mar 2019 15:00:28 +0000 (UTC) Received: from [10.36.116.76] (ovpn-116-76.ams2.redhat.com [10.36.116.76]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4589B6CE41; Tue, 19 Mar 2019 14:59:39 +0000 (UTC) To: Maxime Coquelin , Jens Freimann Cc: Tiwei Bie , zhihong.wang@intel.com, dev@dpdk.org References: <20190319064312.13743-1-tiwei.bie@intel.com> <20190319064312.13743-6-tiwei.bie@intel.com> <20190319094432.iap4i7ffs6soukr7@jenstp.localdomain> <20190319100939.GA3839@dpdk-tbie.sh.intel.com> <20190319134751.ibgaalfravjm77lh@jenstp.localdomain> <1d05e311-c60b-cb1d-ff2b-974e01a0451b@redhat.com> From: Kevin Traynor Message-ID: <64884489-8cc7-7c3f-5b65-09f46112adb0@redhat.com> Date: Tue, 19 Mar 2019 14:59:38 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: <1d05e311-c60b-cb1d-ff2b-974e01a0451b@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Tue, 19 Mar 2019 15:00:28 +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: , X-List-Received-Date: Tue, 19 Mar 2019 15:00:30 -0000 On 19/03/2019 13:50, Maxime Coquelin wrote: > > > On 3/19/19 2:47 PM, Jens Freimann wrote: >> On Tue, Mar 19, 2019 at 02:28:30PM +0100, Maxime Coquelin wrote: >>> >>> >>> On 3/19/19 11:09 AM, Tiwei Bie wrote: >>>> On Tue, Mar 19, 2019 at 10:44:32AM +0100, Jens Freimann wrote: >>>>> 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. >>>> >>>> I think it will make it clear that fields like used_wrap_counter >>>> are only available in packed ring which will make the code more >>>> readable. >>>> >>>>> >>>>> 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 prefer to have consistent prefix like most fields in this >>>> structure (although some fields don't really follow this). >>> >>> As Jens, I tend to agree that the vq_ prefix is quite redundant. >>> However, I think it is better to keep it in this patch for consistency. >>> >>> Maybe it can be remove in a separate patch later? >> >> I thought it might be convenient to change it now as we are touching >> all related code anyway. But I also don't want to block the patch >> because of >> this cosmetic thing. So let's defer it to a later patch set. > > OK, when I meant later, I meant to remove vq_ prefix for all fields, not > only vq_split & vq_packed. > > But yes, that's just cosmetic so let's keep it as is for now. > I agree the vq_ prefix is not needed and I think the code is more readable in general seeing the packed/split name when using the struct. Please also consider that cosmetic changes in multiple places likely mean backports will not apply cleanly to the stable branches anymore, so it does have a cost. Although in this case, iirc packed rings are not in 18.11, so fixes might need dedicated backports from authors anyway, and there haven't been too many virtio backports to date. >> >> regards, >> Jens 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 DE397A00E6 for ; Tue, 19 Mar 2019 16:00:31 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id DC91A239; Tue, 19 Mar 2019 16:00:30 +0100 (CET) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 9CC17A3 for ; Tue, 19 Mar 2019 16:00:29 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AD3913087BCA; Tue, 19 Mar 2019 15:00:28 +0000 (UTC) Received: from [10.36.116.76] (ovpn-116-76.ams2.redhat.com [10.36.116.76]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4589B6CE41; Tue, 19 Mar 2019 14:59:39 +0000 (UTC) To: Maxime Coquelin , Jens Freimann Cc: Tiwei Bie , zhihong.wang@intel.com, dev@dpdk.org References: <20190319064312.13743-1-tiwei.bie@intel.com> <20190319064312.13743-6-tiwei.bie@intel.com> <20190319094432.iap4i7ffs6soukr7@jenstp.localdomain> <20190319100939.GA3839@dpdk-tbie.sh.intel.com> <20190319134751.ibgaalfravjm77lh@jenstp.localdomain> <1d05e311-c60b-cb1d-ff2b-974e01a0451b@redhat.com> From: Kevin Traynor Message-ID: <64884489-8cc7-7c3f-5b65-09f46112adb0@redhat.com> Date: Tue, 19 Mar 2019 14:59:38 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: <1d05e311-c60b-cb1d-ff2b-974e01a0451b@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Tue, 19 Mar 2019 15:00:28 +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: <20190319145938.Orvj0pDl8ZZoEmODic1E_PO2NCMgiWnNQ7hCXVIGfQk@z> On 19/03/2019 13:50, Maxime Coquelin wrote: > > > On 3/19/19 2:47 PM, Jens Freimann wrote: >> On Tue, Mar 19, 2019 at 02:28:30PM +0100, Maxime Coquelin wrote: >>> >>> >>> On 3/19/19 11:09 AM, Tiwei Bie wrote: >>>> On Tue, Mar 19, 2019 at 10:44:32AM +0100, Jens Freimann wrote: >>>>> 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. >>>> >>>> I think it will make it clear that fields like used_wrap_counter >>>> are only available in packed ring which will make the code more >>>> readable. >>>> >>>>> >>>>> 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 prefer to have consistent prefix like most fields in this >>>> structure (although some fields don't really follow this). >>> >>> As Jens, I tend to agree that the vq_ prefix is quite redundant. >>> However, I think it is better to keep it in this patch for consistency. >>> >>> Maybe it can be remove in a separate patch later? >> >> I thought it might be convenient to change it now as we are touching >> all related code anyway. But I also don't want to block the patch >> because of >> this cosmetic thing. So let's defer it to a later patch set. > > OK, when I meant later, I meant to remove vq_ prefix for all fields, not > only vq_split & vq_packed. > > But yes, that's just cosmetic so let's keep it as is for now. > I agree the vq_ prefix is not needed and I think the code is more readable in general seeing the packed/split name when using the struct. Please also consider that cosmetic changes in multiple places likely mean backports will not apply cleanly to the stable branches anymore, so it does have a cost. Although in this case, iirc packed rings are not in 18.11, so fixes might need dedicated backports from authors anyway, and there haven't been too many virtio backports to date. >> >> regards, >> Jens