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