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

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

Any review, please?

2015-11-13 09:30, Tom Kiely:
> 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>
  

On Tue, Nov 24, 2015 at 10:20:22PM +0100, Thomas Monjalon wrote:
> Any review, please?

Huawei, would you review it? Sorry that I've not read too much
code about virtio PMD driver yet.

	--yliu


> 2015-11-13 09:30, Tom Kiely:
> > 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>
  

On 11/25/2015 9:47 AM, Yuanhan Liu wrote:
> On Tue, Nov 24, 2015 at 10:20:22PM +0100, Thomas Monjalon wrote:
>> Any review, please?
> Huawei, would you review it? Sorry that I've not read too much
> code about virtio PMD driver yet.
Np. will do it by end of this week.
>
> 	--yliu
>
>
>> 2015-11-13 09:30, Tom Kiely:
>>> 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>
  

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

On 11/26/2015 1:33 AM, 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
> ?
Tom:
Any further progress?
>>  		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);
>
  

Hi,
    Sorry for the delay. I have been occupied on another critical issue. 
I'll look at this today.
    Tom

On 12/17/2015 04:47 AM, Xie, Huawei wrote:
> On 11/26/2015 1:33 AM, 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
>> ?
> Tom:
> Any further progress?
>>>   		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);
  

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

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.

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

On Tue, Jan 05, 2016 at 07:13:04AM +0000, Xie, Huawei 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.
> 
> I find virtqueue_full is confusing, maybe we could change it to some
> other meaningful name.
> 
> >
> >    Tom
> >
Ping.

Tom and Huawei, what is the status of this patch? Will there be a V2?

/Bruce
  

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.

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

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

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

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

Could you please send the patch?
  

Sure.
    Tom

On 03/04/2016 06:16 AM, Xie, Huawei wrote:
> 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
> Could you please send the patch?
>
  

On Fri, Mar 4, 2016 at 3:11 AM, Tom Kiely <tkiely@brocade.com> wrote:
> Sure.
>    Tom
>
>
> On 03/04/2016 06:16 AM, Xie, Huawei wrote:
>>
>> 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
>>
>> Could you please send the patch?
>>
>

I should have replied to this earlier. I submitted a patch last week:
http://dpdk.org/dev/patchwork/patch/10904/
  

On Fri, Mar 04, 2016 at 08:25:07AM -0500, Kyle Larose wrote:
> On Fri, Mar 4, 2016 at 3:11 AM, Tom Kiely <tkiely@brocade.com> wrote:
> > Sure.
> >    Tom
> >
> >
> > On 03/04/2016 06:16 AM, Xie, Huawei wrote:
> >>
> >> 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
> >>
> >> Could you please send the patch?
> >>
> >
> 
> I should have replied to this earlier. I submitted a patch last week:
> http://dpdk.org/dev/patchwork/patch/10904/

Thanks, Kyle. Unfortunately the patch you submitted is missing your signoff.
Can you perhaps resubmit it as a V2 with the necessary sign-off as described
in the contributors guide:
http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-body

Huawei or Tom, could one of you guys perhaps review and ack the patch once it's
submitted with a signoff?

Thanks,
/Bruce
  

On Wed, Mar 9, 2016 at 4:37 PM, Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Fri, Mar 04, 2016 at 08:25:07AM -0500, Kyle Larose wrote:
>> On Fri, Mar 4, 2016 at 3:11 AM, Tom Kiely <tkiely@brocade.com> wrote:
>> > Sure.
>> >    Tom
>> >
>> >
>> > On 03/04/2016 06:16 AM, Xie, Huawei wrote:
>> >>
>> >> 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
>> >>
>> >> Could you please send the patch?
>> >>
>> >
>>
>> I should have replied to this earlier. I submitted a patch last week:
>> http://dpdk.org/dev/patchwork/patch/10904/
>
> Thanks, Kyle. Unfortunately the patch you submitted is missing your signoff.
> Can you perhaps resubmit it as a V2 with the necessary sign-off as described
> in the contributors guide:
> http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-body
>

Hey Bruce,

Thanks. I signed off, and resubmitted the patch. Hopefully I didn't
make any other amateur mistakes this time!

> Huawei or Tom, could one of you guys perhaps review and ack the patch once it's
> submitted with a signoff?
>
> Thanks,
> /Bruce