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 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 > > Ping.
Tom and Huawei, what is the status of this patch? Will there be a V2? /Bruce
