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

[Tom Kiely <tkiely@brocade.com>]

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.

    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);