From: Marc Sune <marc.sune@bisdn.de>
To: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] kni:optimization of rte_kni_rx_burst
Date: Wed, 25 Feb 2015 13:38:18 +0100 [thread overview]
Message-ID: <54EDC23A.2080302@bisdn.de> (raw)
In-Reply-To: <BY2PR0301MB069369AA0C7436E5F9A63AE9C2170@BY2PR0301MB0693.namprd03.prod.outlook.com>
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.
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
> alloc_q can be empty in this case and kernel will be striving.
>
>> Olivier.
>>
>> On 25/02/15 11:48, Hemant Agrawal wrote:
>>> From: Hemant Agrawal <hemant@freescale.com>
>>>
>>> if any buffer is read from the tx_q, MAX_BURST buffers will be allocated and
>> attempted to be added to to the alloc_q.
>>> This seems terribly inefficient and it also looks like the alloc_q will quickly fill
>> to its maximum capacity. If the system buffers are low in number, it will reach
>> "out of memory" situation.
>>> This patch allocates the number of buffers as many dequeued from tx_q.
>>>
>>> Signed-off-by: Hemant Agrawal <hemant@freescale.com>
>>> ---
>>> lib/librte_kni/rte_kni.c | 13 ++++++++-----
>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c index
>>> 4e70fa0..4cf8e30 100644
>>> --- a/lib/librte_kni/rte_kni.c
>>> +++ b/lib/librte_kni/rte_kni.c
>>> @@ -128,7 +128,7 @@ struct rte_kni_memzone_pool {
>>>
>>>
>>> static void kni_free_mbufs(struct rte_kni *kni); -static void
>>> kni_allocate_mbufs(struct rte_kni *kni);
>>> +static void kni_allocate_mbufs(struct rte_kni *kni, int num);
>>>
>>> static volatile int kni_fd = -1;
>>> static struct rte_kni_memzone_pool kni_memzone_pool = { @@ -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);
>>>
>>> return ret;
>>> }
>>> @@ -594,7 +594,7 @@ kni_free_mbufs(struct rte_kni *kni)
>>> }
>>>
>>> static void
>>> -kni_allocate_mbufs(struct rte_kni *kni)
>>> +kni_allocate_mbufs(struct rte_kni *kni, int num)
>>> {
>>> int i, ret;
>>> struct rte_mbuf *pkts[MAX_MBUF_BURST_NUM]; @@ -620,7 +620,10
>> @@
>>> kni_allocate_mbufs(struct rte_kni *kni)
>>> return;
>>> }
>>>
>>> - for (i = 0; i < MAX_MBUF_BURST_NUM; i++) {
>>> + if (num == 0 || num > MAX_MBUF_BURST_NUM)
>>> + num = MAX_MBUF_BURST_NUM;
>>> +
>>> + for (i = 0; i < num; i++) {
>>> pkts[i] = rte_pktmbuf_alloc(kni->pktmbuf_pool);
>>> if (unlikely(pkts[i] == NULL)) {
>>> /* Out of memory */
>>> @@ -636,7 +639,7 @@ kni_allocate_mbufs(struct rte_kni *kni)
>>> ret = kni_fifo_put(kni->alloc_q, (void **)pkts, i);
>>>
>>> /* Check if any mbufs not put into alloc_q, and then free them */
>>> - if (ret >= 0 && ret < i && ret < MAX_MBUF_BURST_NUM)
>> {MAX_MBUF_BURST_NUM
>>> + if (ret >= 0 && ret < i && ret < num) {
>>> int j;
>>>
>>> for (j = ret; j < i; j++)
>> --
>> *Olivier Demé*
>> *Druid Software Ltd.*
>> *Tel: +353 1 202 1831*
>> *Email: odeme@druidsoftware.com <mailto:odeme@druidsoftware.com>*
>> *URL: http://www.druidsoftware.com*
>> *Hall 7, stand 7F70.*
>> Druid Software: Monetising enterprise small cells solutions.
next prev parent reply other threads:[~2015-02-25 12:38 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 [this message]
2015-02-25 12:51 ` Olivier Deme
2015-02-25 13:29 ` Jay Rolette
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=54EDC23A.2080302@bisdn.de \
--to=marc.sune@bisdn.de \
--cc=dev@dpdk.org \
/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).