From: Kevin Traynor <ktraynor@redhat.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
Jens Freimann <jfreimann@redhat.com>
Cc: Tiwei Bie <tiwei.bie@intel.com>, zhihong.wang@intel.com, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 05/10] net/virtio: refactor virtqueue structure
Date: Tue, 19 Mar 2019 14:59:38 +0000 [thread overview]
Message-ID: <64884489-8cc7-7c3f-5b65-09f46112adb0@redhat.com> (raw)
Message-ID: <20190319145938.Orvj0pDl8ZZoEmODic1E_PO2NCMgiWnNQ7hCXVIGfQk@z> (raw)
In-Reply-To: <1d05e311-c60b-cb1d-ff2b-974e01a0451b@redhat.com>
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 <tiwei.bie@intel.com>
>>>>>> ---
>>>>>> 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
next prev parent reply other threads:[~2019-03-19 15:00 UTC|newest]
Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-19 6:43 [dpdk-dev] [PATCH 00/10] net/virtio: cleanups and fixes for packed/split ring Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 6:43 ` [dpdk-dev] [PATCH 01/10] net/virtio: fix typo in packed ring init Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 8:39 ` Jens Freimann
2019-03-19 8:39 ` Jens Freimann
2019-03-19 12:46 ` Maxime Coquelin
2019-03-19 12:46 ` Maxime Coquelin
2019-03-19 6:43 ` [dpdk-dev] [PATCH 02/10] net/virtio: fix interrupt helper for packed ring Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 12:48 ` Maxime Coquelin
2019-03-19 12:48 ` Maxime Coquelin
2019-03-19 6:43 ` [dpdk-dev] [PATCH 03/10] net/virtio: add missing barrier in interrupt enable Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 12:49 ` Maxime Coquelin
2019-03-19 12:49 ` Maxime Coquelin
2019-03-19 6:43 ` [dpdk-dev] [PATCH 04/10] net/virtio: optimize flags update for packed ring Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 8:54 ` Jens Freimann
2019-03-19 8:54 ` Jens Freimann
2019-03-19 9:37 ` Tiwei Bie
2019-03-19 9:37 ` Tiwei Bie
2019-03-19 10:11 ` Jens Freimann
2019-03-19 10:11 ` Jens Freimann
2019-03-19 12:50 ` Maxime Coquelin
2019-03-19 12:50 ` Maxime Coquelin
2019-03-19 12:58 ` Maxime Coquelin
2019-03-19 12:58 ` Maxime Coquelin
2019-03-19 6:43 ` [dpdk-dev] [PATCH 05/10] net/virtio: refactor virtqueue structure Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 9:44 ` Jens Freimann
2019-03-19 9:44 ` Jens Freimann
2019-03-19 10:09 ` Tiwei Bie
2019-03-19 10:09 ` Tiwei Bie
2019-03-19 13:28 ` Maxime Coquelin
2019-03-19 13:28 ` Maxime Coquelin
2019-03-19 13:47 ` Jens Freimann
2019-03-19 13:47 ` Jens Freimann
2019-03-19 13:50 ` Maxime Coquelin
2019-03-19 13:50 ` Maxime Coquelin
2019-03-19 14:59 ` Kevin Traynor [this message]
2019-03-19 14:59 ` Kevin Traynor
2019-03-20 4:40 ` Tiwei Bie
2019-03-20 4:40 ` Tiwei Bie
2019-03-20 17:50 ` Stephen Hemminger
2019-03-20 17:50 ` Stephen Hemminger
2019-03-21 14:18 ` Maxime Coquelin
2019-03-21 14:18 ` Maxime Coquelin
2019-03-20 4:35 ` Tiwei Bie
2019-03-20 4:35 ` Tiwei Bie
2019-03-19 13:28 ` Maxime Coquelin
2019-03-19 13:28 ` Maxime Coquelin
2019-03-19 6:43 ` [dpdk-dev] [PATCH 06/10] net/virtio: drop redundant suffix in packed ring structure Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 9:47 ` Jens Freimann
2019-03-19 9:47 ` Jens Freimann
2019-03-19 13:29 ` Maxime Coquelin
2019-03-19 13:29 ` Maxime Coquelin
2019-03-19 6:43 ` [dpdk-dev] [PATCH 07/10] net/virtio: drop unused field in Tx region structure Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 9:51 ` Jens Freimann
2019-03-19 9:51 ` Jens Freimann
2019-03-19 13:33 ` Maxime Coquelin
2019-03-19 13:33 ` Maxime Coquelin
2019-03-19 6:43 ` [dpdk-dev] [PATCH 08/10] net/virtio: add interrupt helper for split ring Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 9:53 ` Jens Freimann
2019-03-19 9:53 ` Jens Freimann
2019-03-19 13:34 ` Maxime Coquelin
2019-03-19 13:34 ` Maxime Coquelin
2019-03-19 6:43 ` [dpdk-dev] [PATCH 09/10] net/virtio: add ctrl vq " Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 9:54 ` Jens Freimann
2019-03-19 9:54 ` Jens Freimann
2019-03-19 13:54 ` Maxime Coquelin
2019-03-19 13:54 ` Maxime Coquelin
2019-03-19 6:43 ` [dpdk-dev] [PATCH 10/10] net/virtio: improve batching in standard Rx path Tiwei Bie
2019-03-19 6:43 ` Tiwei Bie
2019-03-19 10:04 ` Jens Freimann
2019-03-19 10:04 ` Jens Freimann
2019-03-19 10:28 ` Tiwei Bie
2019-03-19 10:28 ` Tiwei Bie
2019-03-19 11:08 ` Maxime Coquelin
2019-03-19 11:08 ` Maxime Coquelin
2019-03-19 14:15 ` Maxime Coquelin
2019-03-19 14:15 ` Maxime Coquelin
2019-03-20 7:35 ` [dpdk-dev] [PATCH 00/10] net/virtio: cleanups and fixes for packed/split ring Maxime Coquelin
2019-03-20 7:35 ` Maxime Coquelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=64884489-8cc7-7c3f-5b65-09f46112adb0@redhat.com \
--to=ktraynor@redhat.com \
--cc=dev@dpdk.org \
--cc=jfreimann@redhat.com \
--cc=maxime.coquelin@redhat.com \
--cc=tiwei.bie@intel.com \
--cc=zhihong.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).