On Tue, Jan 5, 2016 at 2:13 AM, Xie, Huawei <huawei.xie at 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 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. > 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); >> >> >
