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

[Bruce Richardson <bruce.richardson@intel.com>]

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