Re: [dpdk-dev] [PATCH 05/10] net/virtio: refactor virtqueue structure

[Kevin Traynor <ktraynor@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