[dpdk-dev] [PATCH] KNI: use a memzone pool for KNI alloc/release

[Zhang, Helin]

Hi Marc

More comments added.

> -----Original Message-----
> From: Marc Sune [mailto:marc.sune at bisdn.de]
> Sent: Thursday, October 9, 2014 6:16 PM
> To: Zhang, Helin
> Cc: dev at 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