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