From: "Zhang, Helin" <helin.zhang@intel.com>
To: Marc Sune <marc.sune@bisdn.de>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] KNI: use a memzone pool for KNI alloc/release
Date: Fri, 10 Oct 2014 05:25:23 +0000 [thread overview]
Message-ID: <F35DEAC7BCE34641BA9FAC6BCA4A12E70A79A977@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <54366044.8020507@bisdn.de>
Hi Marc
More comments added.
> -----Original Message-----
> From: Marc Sune [mailto:marc.sune@bisdn.de]
> Sent: Thursday, October 9, 2014 6:16 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] KNI: use a memzone pool for KNI alloc/release
>
> Hi Helin,
>
> inline and snipped. Let me know after reading this mail if you believe I can
> already submit the v2 with the changes you suggested.
>
> On 09/10/14 10:57, Zhang, Helin wrote:
> > [snip]
> >>>> I don't think the approach of pre-allocating on the first
> >>>> rte_kni_alloc() would work (I already discarded this approach
> >>>> before implementing the patch), because this would imply we need a
> >>>> define of #define MAX_KNI_IFACES during compilation time of DPDK,
> >>>> and the pre-allocation is highly dependent on the amount of
> >>>> hugepages memory you have and the usage of the KNI interfaces the
> applications wants to do.
> >>>> We can easily end up with DPDK users having to tweak the default
> >>>> MAX_KNI_IFACES before compiling DPDK every time, which is
> >>>> definetely not desirable IMHO.
> >>> Your idea is good! My point is it possible to avoid adding new
> >>> interface, then no changes are needed in user app.
> >> I see the current approach the most clean and comprehensive (from the
> >> perspective of the user of the library) approach. Do you have any
> >> other proposal? I am open to discuss and eventually implement it if
> >> it turns out to be better.
> > How about add a new compile config item in config files? I still think
> > we should avoid adding more interfaces if possible. :)
>
> In my original answer to your comment here cited starting by "I don't think the
> approach of pre-allocating on the first rte_kni_alloc()..." I explain why I think
> this is not a good idea.
I understood your concern. It is not bad of adding a config item in config files
(config/config_linux), as it already has a lot of compile time configurations in them.
For a specific platform, the maximum number of KNI interfaces should be fixed,
and no need to be changed frequently.
>
> A config.g #define approach would be highly dependent on hugepages memory
> size and the usage the applications wants to do with KNI interfaces. Specially
> due to the former, I don't think it is a good idea. DPDK doesn't force any user to
> edit manually the config.h AFAIK, unless you want to do some performance
> optimizations or debug. And I think it is a good approach and I would like to
> keep it and not break it with this patch
No need to edit config.h, just modify config/config_linux or config/config_bsd.
>
> Any parameter that depends on DPDK applications so far, so really out of the
> scope of DPDK itself (like the size of the pool parameter is), is handled via an
> API call. So I see rte_kni_init() as the natural way to do so, specially by the fact
> that rte_kni_close() API call already exists.
I agree that your solution is good, I am just thinking if we can make less changes
for API level.
>
> >>>> For rte_kni_close(), the pool is static (incl. the slot struct),
> >>>> and the memzones cannot be unreserved, hence there is nothing AFAIU
> >>>> to de-initialize; what do you mean specifically?
> >>> You can see that rte_kni_close() will be called in XEN (#ifdef
> >>> RTE_LIBRTE_XEN_DOM0), XEN support is different from standard Linux
> >> support.
> >>
> >> OK it is called, but what is the (extra) state that I should
> >> de-initialize that is coming from this patch? I cannot see any state
> >> I've added I have to de-initialize here.
> > Just suggest you think about that. maybe nothing needs to be added
> > there. :)
>
> I will definitely double-check before submitting v2.
>
> Thanks for the suggestions
> Marc
Regards,
Helin
next prev parent reply other threads:[~2014-10-10 5:18 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-29 10:15 Marc Sune
2014-10-09 6:01 ` Zhang, Helin
2014-10-09 7:05 ` Marc Sune
2014-10-09 7:32 ` Zhang, Helin
2014-10-09 7:52 ` Marc Sune
2014-10-09 8:33 ` Zhang, Helin
2014-10-09 8:45 ` Marc Sune
2014-10-09 8:57 ` Zhang, Helin
2014-10-09 10:15 ` Marc Sune
2014-10-10 5:25 ` Zhang, Helin [this message]
2014-10-10 6:37 ` Marc Sune
2014-10-10 7:35 ` Zhang, Helin
2014-10-10 9:02 ` 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=F35DEAC7BCE34641BA9FAC6BCA4A12E70A79A977@SHSMSX104.ccr.corp.intel.com \
--to=helin.zhang@intel.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).