[dpdk-dev] [PATCH v5 7/9] net/virtio: support in-order Rx and Tx

[Ferruh Yigit]

On 7/3/2018 2:36 AM, Liu, Yong wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, July 03, 2018 12:57 AM
>> To: Maxime Coquelin <maxime.coquelin at redhat.com>; Liu, Yong
>> <yong.liu at intel.com>; Bie, Tiwei <tiwei.bie at intel.com>
>> Cc: Wang, Zhihong <zhihong.wang at intel.com>; dev at dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v5 7/9] net/virtio: support in-order Rx and
>> Tx
>>
>> On 7/2/2018 5:53 PM, Maxime Coquelin wrote:
>>>
>>>
>>> On 07/02/2018 06:52 PM, Ferruh Yigit wrote:
>>>> On 7/2/2018 5:41 PM, Ferruh Yigit wrote:
>>>>> On 7/2/2018 2:56 PM, Marvin Liu wrote:
>>>>>> IN_ORDER Rx function depends on merge-able feature. Descriptors
>>>>>> allocation and free will be done in bulk.
>>>>>>
>>>>>> Virtio dequeue logic:
>>>>>>      dequeue_burst_rx(burst mbufs)
>>>>>>      for (each mbuf b) {
>>>>>>              if (b need merge) {
>>>>>>                      merge remained mbufs
>>>>>>                      add merged mbuf to return mbufs list
>>>>>>              } else {
>>>>>>                      add mbuf to return mbufs list
>>>>>>              }
>>>>>>      }
>>>>>>      if (last mbuf c need merge) {
>>>>>>              dequeue_burst_rx(required mbufs)
>>>>>>              merge last mbuf c
>>>>>>      }
>>>>>>      refill_avail_ring_bulk()
>>>>>>      update_avail_ring()
>>>>>>      return mbufs list
>>>>>>
>>>>>> IN_ORDER Tx function can support offloading features. Packets which
>>>>>> matched "can_push" option will be handled by simple xmit function.
>> Those
>>>>>> packets can't match "can_push" will be handled by original xmit
>> function
>>>>>> with in-order flag.
>>>>>>
>>>>>> Virtio enqueue logic:
>>>>>>      xmit_cleanup(used descs)
>>>>>>      for (each xmit mbuf b) {
>>>>>>              if (b can inorder xmit) {
>>>>>>                      add mbuf b to inorder burst list
>>>>>>                      continue
>>>>>>              } else {
>>>>>>                      xmit inorder burst list
>>>>>>                      xmit mbuf b by original function
>>>>>>              }
>>>>>>      }
>>>>>>      if (inorder burst list not empty) {
>>>>>>              xmit inorder burst list
>>>>>>      }
>>>>>>      update_avail_ring()
>>>>>>
>>>>>> Signed-off-by: Marvin Liu <yong.liu at intel.com>
>>>>>> Reviewed-by: Maxime Coquelin <maxime.coquelin at redhat.com>
>>>>>
>>>>> <...>
>>>>>
>>>>>> @@ -150,6 +188,83 @@ virtio_xmit_cleanup(struct virtqueue *vq,
>> uint16_t num)
>>>>>>   	}
>>>>>>   }
>>>>>>
>>>>>> +/* Cleanup from completed inorder transmits. */
>>>>>> +static void
>>>>>> +virtio_xmit_cleanup_inorder(struct virtqueue *vq, uint16_t num)
>>>>>> +{
>>>>>> +	uint16_t i, used_idx, desc_idx, last_idx;
>>>>>
>>>>>
>>>>> Getting following build error [1], from code it looks like false
>> positive, but
>>>>> to get rid of the build error would it be OK to set initial value to
>> "desc_idx"?
>>>>
>>>> I applied this while merging, if this is wrong please let me know, we
>> can fix in
>>>> next-net, Thanks.
>>>
>>> Looks good to me. I didn't catch it with the GCC version I use.
>>
>> I didn't dig more but I also didn't get the error with regular build, the
>> one
>> with all DEBUGs enabled and mach=default combination gave the error, not
>> sure why.
>>
> 
> Ferruh,
> I didn't catch this error either. I tried three gcc versions 7.2.0, 7.3.0 and 4.8.3. 
> May I know your gcc version? 

gcc [1] with debug enabled [2], giving error [3].




[1]
gcc (GCC) 8.1.1 20180502 (Red Hat 8.1.1-1)

make T=x86_64-native-linuxapp-gcc config


[2]
CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=y
CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=y
CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=y

[3]
.../drivers/net/virtio/virtio_rxtx.c: In function ‘virtio_xmit_cleanup_inorder’:
.../drivers/net/virtio/virtio_rxtx.c:195:24: error: ‘desc_idx’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]


  uint16_t i, used_idx, desc_idx, last_idx;
                        ^~~~~~~~
cc1: all warnings being treated as errors


> 
> Thanks,
> Marvin
> 
>>>
>>> Thanks,
>>> Maxime
>>>
>>>>>
>>>>>
>>>>> [1]
>>>>> .../dpdk/drivers/net/virtio/virtio_rxtx.c:195:24: error: ‘desc_idx’
>> may be used
>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>
>>>>>
>>>>>    uint16_t i, used_idx, desc_idx, last_idx;
>>>>>
>>>>>
>>>>>
>>>>>                          ^~~~~~~~
>>>>>
>>>>
>