From: Jay Rolette <rolette@infiniteio.com>
To: Marc Sune <marc.sune@bisdn.de>
Cc: DPDK <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] kni:optimization of rte_kni_rx_burst
Date: Wed, 25 Feb 2015 07:29:59 -0600 [thread overview]
Message-ID: <CADNuJVqw4hBD1kk1Y4A2+8-5yi0A0K9_5zBQaO3zOnnwXC0nog@mail.gmail.com> (raw)
In-Reply-To: <54EDC23A.2080302@bisdn.de>
On Wed, Feb 25, 2015 at 6:38 AM, Marc Sune <marc.sune@bisdn.de> wrote:
>
> On 25/02/15 13:24, Hemant@freescale.com wrote:
>
>> Hi OIivier
>> Comments inline.
>> Regards,
>> Hemant
>>
>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Deme
>>> Sent: 25/Feb/2015 5:44 PM
>>> To: dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH] kni:optimization of rte_kni_rx_burst
>>>
>>> Thank you Hemant, I think there might be one issue left with the patch
>>> though.
>>> The alloc_q must initially be filled with mbufs before getting mbuf back
>>> on the
>>> tx_q.
>>>
>>> So the patch should allow rte_kni_rx_burst to check if alloc_q is empty.
>>> If so, it should invoke kni_allocate_mbufs(kni, 0) (to fill the alloc_q
>>> with
>>> MAX_MBUF_BURST_NUM mbufs)
>>>
>>> The patch for rte_kni_rx_burst would then look like:
>>>
>>> @@ -575,7 +575,7 @@ rte_kni_rx_burst(struct rte_kni *kni, struct rte_mbuf
>>> **mbufs, unsigned num)
>>>
>>> /* If buffers removed, allocate mbufs and then put them into
>>> alloc_q */
>>> if (ret)
>>> - kni_allocate_mbufs(kni);
>>> + kni_allocate_mbufs(kni, ret);
>>> + else if (unlikely(kni->alloc_q->write == kni->alloc_q->read))
>>> + kni_allocate_mbufs(kni, 0);
>>>
>>> [hemant] This will introduce a run-time check.
>>
>> I missed to include the other change in the patch.
>> I am doing it in kni_alloc i.e. initiate the alloc_q with default burst
>> size.
>> kni_allocate_mbufs(ctx, 0);
>>
>> In a way, we are now suggesting to reduce the size of alloc_q to only
>> default burst size.
>>
>
> As an aside comment here, I think that we should allow to tweak the
> userspace <-> kernel queue sizes (rx_q, tx_q, free_q and alloc_q) . Whether
> this should be a build configuration option or a parameter to
> rte_kni_init(), it is not completely clear to me, but I guess
> rte_kni_init() is a better option.
>
rte_kni_init() is definitely a better option. It allows things to be tuned
based on individual system config rather than requiring different builds.
> Having said that, the original mail from Hemant was describing that KNI
> was giving an out-of-memory. This to me indicates that the pool is
> incorrectly dimensioned. Even if KNI will not pre-allocate in the alloc_q,
> or not completely, in the event of high load, you will get this same "out
> of memory".
>
> We can reduce the usage of buffers by the KNI subsystem in kernel space
> and in userspace, but the kernel will always need a small cache of
> pre-allocated buffers (coming from user-space), since the KNI kernel module
> does not know where to grab the packets from (which pool). So my guess is
> that the dimensioning problem experienced by Hemant would be the same, even
> with the proposed changes.
>
>
>> Can we reach is situation, when the kernel is adding packets faster in
>> tx_q than the application is able to dequeue?
>>
>
> I think so. We cannot control much how the kernel will schedule the KNI
> thread(s), specially if the # of threads in relation to the cores is
> incorrect (not enough), hence we need at least a reasonable amount of
> buffering to prevent early dropping to those "internal" burst side effects.
>
> Marc
Strongly agree with Marc here. We *really* don't want just a single burst
worth of mbufs available to the kernel in alloc_q. That's just asking for
congestion when there's no need for it.
The original problem reported by Olivier is more of a resource tuning
problem than anything else. The number of mbufs you need in the system has
to take into account internal queue depths.
Jay
next prev parent reply other threads:[~2015-02-25 13:30 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-25 11:48 Hemant Agrawal
2015-02-25 12:13 ` Olivier Deme
2015-02-25 12:24 ` Hemant
2015-02-25 12:28 ` Olivier Deme
2015-02-25 12:38 ` Marc Sune
2015-02-25 12:51 ` Olivier Deme
2015-02-25 13:29 ` Jay Rolette [this message]
2015-02-26 7:00 ` Hemant
2015-02-26 12:56 ` Marc Sune
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=CADNuJVqw4hBD1kk1Y4A2+8-5yi0A0K9_5zBQaO3zOnnwXC0nog@mail.gmail.com \
--to=rolette@infiniteio.com \
--cc=dev@dpdk.org \
--cc=marc.sune@bisdn.de \
/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).