From: Tom Kiely <tkiely@brocade.com>
To: Kyle Larose <eomereadig@gmail.com>, "Xie, Huawei" <huawei.xie@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
Date: Mon, 22 Feb 2016 16:23:05 +0000 [thread overview]
Message-ID: <56CB35E9.50309@brocade.com> (raw)
In-Reply-To: <CAMFWN9kznNQJ=gUsDLqdAyTh-Lp+y8+7R-CP-pgyckrDU7M5+w@mail.gmail.com>
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@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);
>>>
next prev parent reply other threads:[~2016-02-22 16:23 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-13 9:30 Tom Kiely
2015-11-24 21:20 ` Thomas Monjalon
2015-11-25 1:50 ` Yuanhan Liu
2015-11-25 2:51 ` Xie, Huawei
2015-11-25 17:32 ` Xie, Huawei
[not found] ` <C37D651A908B024F974696C65296B57B4C545F75@SHSMSX101.ccr.corp.intel.com>
2015-12-17 9:22 ` Tom Kiely
2015-12-17 11:18 ` Tom Kiely
2016-01-05 7:13 ` Xie, Huawei
2016-02-10 15:07 ` Bruce Richardson
2016-02-18 14:03 ` Kyle Larose
2016-02-22 16:23 ` Tom Kiely [this message]
2016-02-23 8:26 ` Xie, Huawei
2016-03-04 6:16 ` Xie, Huawei
2016-03-04 8:11 ` Tom Kiely
2016-03-04 13:25 ` Kyle Larose
2016-03-09 21:37 ` Bruce Richardson
2016-03-10 14:46 ` Kyle Larose
2016-02-26 18:58 Kyle Larose
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56CB35E9.50309@brocade.com \
--to=tkiely@brocade.com \
--cc=dev@dpdk.org \
--cc=eomereadig@gmail.com \
--cc=huawei.xie@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).