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

[Zhang, Helin]

Hi Marc

> -----Original Message-----
> From: Marc Sune [mailto:marc.sune at bisdn.de]
> Sent: Friday, October 10, 2014 2:38 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,
> 
> On 10/10/14 07:25, Zhang, Helin wrote:
> > Hi Marc
> >
> > More comments added.
> >
> >> [snip]
> >>>>>> 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.
> rte_kni_init() should be staying. Actually the asymmetry of the API nowadays
> (no rte_kni_init, because fd is created during first alloc but an rte_kni_close)
> looks weird to me. Just an aside question, not related to this patch, why was
> the KNI fd not closed in the last rte_kni_release to be consistent?
Initially there was no rte_kni_close() which was later added for supporting XEN
specifically.
If KNI needs an init function, it might need to be put in rte_eal_init() like other
modules do.

> >
> >> 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.
> This is what I meant, all the config_*.h
> >> 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.
> 
> I can understand the reluctance for adding new API calls, but, let me double
> check, as I am not sure you understood my point:
> 
> If we set it in the config_*.h, and we set MAX_NUM_OF_KNI to a value
> whatever, 8, 16... 128..., it is quite possible that a lot of users of DPDK that will
> use the KNI (only those) get run-time errors since the memzones cannot be
> pre-allocated (out of memory). Memzones are preallocated at rte_kni_init() (or
> at first alloc, as per what you are suggesting). Moreover, the user would have
> to go and change (e.g.
> reduce) the MAX_NUM_OF_KNI in the config_*.h and recompile DPDK. I don't
> think that's what we want.
Actually the end user doesn't know the mapping between hugepage memory size
and the maximum number of KNI. Right? Giving 1G memory, end user doesn't know
how many KNI interfaces it can support. So the maximum number of KNI interfaces
still needs to be tuned.
I think the default number can be tuned to a reasonable small to support most of
cases, e.g. 4 or 8. And I don't think that number needs to be changed at runtime.

> 
> Marc

Regards,
Helin