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

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

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

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

[Thomas Monjalon]

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>

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

[Yuanhan Liu]

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>

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

[Xie, Huawei]

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>

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

[Xie, Huawei]

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

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

[Tom Kiely]

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

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

[Tom Kiely]

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

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

[Xie, Huawei]

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

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

[Bruce Richardson]

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

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

[Kyle Larose]

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

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

[Tom Kiely]

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

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

[Xie, Huawei]

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

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

[Xie, Huawei]

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?

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

[Tom Kiely]

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

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

[Kyle Larose]

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/

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

[Bruce Richardson]

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

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

[Kyle Larose]

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

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

[Kyle Larose]

Virtio has an mbuf descriptor ring containing mbufs to be used for receiving
traffic. When the host queues traffic to be sent to the guest, it consumes
these descriptors. If none exist, it discards the packet.

The virtio pmd allocates mbufs to the descriptor ring every time it
succesfully receives a packet. However, it never does it if it does not
receive a valid packet. If the descriptor ring is exhausted, and the mbuf
mempool does not have any mbufs free (which can happen for various reasons,
such as queueing along the processing pipeline), then the receive call will
not allocate any mbufs to the descriptor ring, and when it finishes, the
descriptor ring will be empty. The ring being empty means that we will never
receive a packet again, which means we will never allocate mbufs to the ring:
we are stuck.

Ultimately, the problem arises because there is a dependency between receiving
packets and making the descriptor ring not be empty, and a dependency between
the descriptor ring not being empty, and receicing packets.

To fix the problem, this pakes makes virtio always try to allocate mbufs to the descriptor
ring, if necessary, when polling for packets. Do this by removing the early
exit if no packets were received. Since the packet loop later will do
nothing if there are no packets, this is fine.
---
 drivers/net/virtio/virtio_rxtx.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 41a1366..9d2f7d6 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -571,9 +571,6 @@ 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)
-		return 0;
-
 	num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num);
 	PMD_RX_LOG(DEBUG, "used:%d dequeue:%d", nb_used, num);
 
@@ -671,9 +668,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 
 	virtio_rmb();
 
-	if (nb_used == 0)
-		return 0;
-
 	PMD_RX_LOG(DEBUG, "used:%d\n", nb_used);
 
 	hw = rxvq->hw;
-- 
1.8.3.1