From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <eomereadig@gmail.com> Received: from mail-ob0-f174.google.com (mail-ob0-f174.google.com [209.85.214.174]) by dpdk.org (Postfix) with ESMTP id 8BAF3C38A for <dev@dpdk.org>; Thu, 18 Feb 2016 15:03:18 +0100 (CET) Received: by mail-ob0-f174.google.com with SMTP id xk3so67784216obc.2 for <dev@dpdk.org>; Thu, 18 Feb 2016 06:03:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=oZ8a9MkhaEdUEkqHZk6lB4NxMEb2DsHgdhAShQOizhc=; b=Ryt56K/VQsq1AcZnuujn6r6x+mnukg+EvCUOoNHz3PYXx82FcbApp/S1pGkrEqPpHS HGqoDs2Pi2O1XIWCKwoYPmDHaBEHI+q1jm0Yu+MN/oU7nF5H7nD/NrT22LOycU7PtzyT zmFz0Aykjo+bu2+zBBYwmf3mT0UXl7IvQCeqXFVNXFm2yWlW/ghB7fUMtJy6e9gwWOE1 VRr1VzpQRbFvUENQBNX9gZrTyhLhNyOtcQH+4Xql98PE1w68U2+oZFEYRklm2iuh5buW nEjVI80DwNIEWybpQcX5dgM2IclFNe9l1nqSCPBjUpXMLmQaqdN+aynRNTd5+I2Db2qk EQZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=oZ8a9MkhaEdUEkqHZk6lB4NxMEb2DsHgdhAShQOizhc=; b=ke7IFRLFEDXGwIY/udnWo1EwZZbwBImzQ63ElchmyP2f6HeWO3ATkpf7clCjjtRS+G qktxtBlIDinklGUT4p01l63DwpNHXR0soytiFleBUtJwfZ2QnQpExv/gEBL3cW0LSFwc RwJ2Cb0SG8rDFKJOux73sBmA6jv/a7cHxuEDVxDfI+yZc75ntDMtThFY0E9eE/Kzw0iG 7zpGUNIOkzAGcNivbeYG+bYierXYfyEJI4fJtKh0qtiaLYE9G1cWNh2q4wDv4UlJ/0FS 0k/N2X4hkdU9VYSfX3+tRyJ4OSFvlmXJ3+fJ3Yb+SBV7xCba1BvNTKdL/szRslLqY0Vg RIhw== X-Gm-Message-State: AG10YOQ5sXMzHVcLk/jD/aQvsSLiqOkUw9pOmrvYC8o5TlQmQzJZdu4lTUb8edFgNi9DD641l2enTlBTvL1elg== MIME-Version: 1.0 X-Received: by 10.202.196.17 with SMTP id u17mr6233853oif.122.1455804196097; Thu, 18 Feb 2016 06:03:16 -0800 (PST) Received: by 10.76.68.7 with HTTP; Thu, 18 Feb 2016 06:03:16 -0800 (PST) In-Reply-To: <C37D651A908B024F974696C65296B57B4C574D68@SHSMSX101.ccr.corp.intel.com> References: <1447407013-6986-1-git-send-email-tkiely@brocade.com> <C37D651A908B024F974696C65296B57B4B1B2353@SHSMSX101.ccr.corp.intel.com> <567299E9.60000@brocade.com> <C37D651A908B024F974696C65296B57B4C574D68@SHSMSX101.ccr.corp.intel.com> Date: Thu, 18 Feb 2016 09:03:16 -0500 Message-ID: <CAMFWN9kznNQJ=gUsDLqdAyTh-Lp+y8+7R-CP-pgyckrDU7M5+w@mail.gmail.com> From: Kyle Larose <eomereadig@gmail.com> To: "Xie, Huawei" <huawei.xie@intel.com> Content-Type: text/plain; charset=UTF-8 Cc: "dev@dpdk.org" <dev@dpdk.org> Subject: Re: [dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Thu, 18 Feb 2016 14:03:18 -0000 On Tue, Jan 5, 2016 at 2:13 AM, Xie, Huawei <huawei.xie@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@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); >> >> >