Re: [dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation

["Xie, Huawei" <huawei.xie@intel.com>]

On 2/23/2016 12:23 AM, Tom Kiely wrote:
> Hi,
>     Sorry I missed the last few messages until now. I'm happy with
> just removing the "if". Kyle, when you say you fixed it, do you mean
> that you will push the patch or have already done so ?
>    Thanks,
>        Tom
>
> On 02/18/2016 02:03 PM, Kyle Larose wrote:
>> On Tue, Jan 5, 2016 at 2:13 AM, Xie, Huawei <huawei.xie@intel.com>
>> wrote:
>>> On 12/17/2015 7:18 PM, Tom Kiely wrote:
>>>>
>>>> On 11/25/2015 05:32 PM, Xie, Huawei wrote:
>>>>> On 11/13/2015 5:33 PM, Tom Kiely wrote:
>>>>>> If all rx descriptors are processed while transient
>>>>>> mbuf exhaustion is present, the rx ring ends up with
>>>>>> no available descriptors. Thus no packets are received
>>>>>> on that ring. Since descriptor refill is performed post
>>>>>> rx descriptor processing, in this case no refill is
>>>>>> ever subsequently performed resulting in permanent rx
>>>>>> traffic drop.
>>>>>>
>>>>>> Signed-off-by: Tom Kiely <tkiely@brocade.com>
>>>>>> ---
>>>>>>    drivers/net/virtio/virtio_rxtx.c |    6 ++++--
>>>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/virtio/virtio_rxtx.c
>>>>>> b/drivers/net/virtio/virtio_rxtx.c
>>>>>> index 5770fa2..a95e234 100644
>>>>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>>>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>>>>> @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
>>>>>> **rx_pkts, uint16_t nb_pkts)
>>>>>>        if (likely(num > DESC_PER_CACHELINE))
>>>>>>            num = num - ((rxvq->vq_used_cons_idx + num) %
>>>>>> DESC_PER_CACHELINE);
>>>>>>    -    if (num == 0)
>>>>>> +    /* Refill free descriptors even if no pkts recvd */
>>>>>> +    if (num == 0 && virtqueue_full(rxvq))
>>>>> Should the return condition be that no used buffers and we have avail
>>>>> descs in avail ring, i.e,
>>>>>       num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries
>>>>>
>>>>> rather than
>>>>>       num == 0 && rxvq->vq_free_cnt == 0
>>>> Yes we could do that but I don't see a good reason to wait until the
>>>> vq_free_cnt == vq_nentries
>>>> before attempting the refill. The existing code will attempt refill
>>>> even if only 1 packet was received
>>>> and the free count is small. To me it seems safer to extend that to
>>>> try refill even if no packet was received
>>>> but the free count is non-zero.
>>> The existing code attempt to refill only if 1 packet was received.
>>>
>>> If we want to refill even no packet was received, then the strict
>>> condition should be
>>>      num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries
>>>
>>> The safer condition, what you want to use,  should be
>>>      num == 0 && !virtqueue_full(...)
>>> rather than
>>>      num == 0 && virtqueue_full(...)
>>>
>>> We could simplify things a bit, just remove this check, if the
>>> following
>>> receiving code already takes care of the "num == 0" condition.
>>>
>> FWIW, I fixed this issue myself by just removing the if(num == 0)
>> checks entirely. I didn't see any benefit in short-circuiting a loop
>> which pretty much does nothing anyway when num == 0. Further, we only
>> hit this case when there's no packets to receive, which means there's
>> probably a few cycles to spare. This is even simpler.

Yes, as i said, that is the simplest fix.

>>
>>> I find virtqueue_full is confusing, maybe we could change it to some
>>> other meaningful name.
>>>
>>>>     Tom
>>>>
>>>>>>            return 0;
>>>>>>          num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num);
>>>>>> @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue,
>>>>>>          virtio_rmb();
>>>>>>    -    if (nb_used == 0)
>>>>>> +    /* Refill free descriptors even if no pkts recvd */
>>>>>> +    if (nb_used == 0 && virtqueue_full(rxvq))
>>>>>>            return 0;
>>>>>>          PMD_RX_LOG(DEBUG, "used:%d\n", nb_used);
>>>>
>
>