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

[Zhang, Helin]

Hi Marc

Ohh, I made a stupid proposal of adding a field per port. Sorry! I still have more comments inlined.

> -----Original Message-----
> From: Marc Sune [mailto:marc.sune at bisdn.de]
> Sent: Thursday, October 9, 2014 4:45 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. Thanks for the additional comments.
> 
> On 09/10/14 10:33, Zhang, Helin wrote:
> > [snip]
> >>> [snip]
> >>>>>> It adds a new API call, rte_kni_init(max_kni_ifaces) that shall
> >>>>>> be called before any call to rte_kni_alloc() if KNI is used.
> >>> To avoid the additional interface, this initialization works can be
> >>> done during the first time of calling rte_kni_alloc(), please refer
> >>> to how it
> >> opens kni_fd ("/dev/kni").
> >>> Also I think there should be some de-initialization works should be
> >>> done in
> >> rte_kni_close().
> >> How is rte_kni_alloc() supposed to know the size of the pool that has
> >> to be pre-allocated (max_kni_ifaces)?
> > Add it into 'struct rte_kni_conf', also a default one might be needed
> > if 0 is configured by the user app.
> 
> I disagree with this approach :) . struct rte_kni_conf is a per-interface
> configuration struct, and the mempool is shared between all the alloc/release
> of the KNI interfaces.
> 
> I don't like the approach to mix one-time-use (first alloc) parameters that affect
> the entire KNI system into the struct rte_kni_conf.
I agree with you, this approach is a bit stupid.

> 
> >> 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. :)
> 
> >
> >> 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. :)

> 
> Many thanks
> Marc

Thanks!

Regards,
Helin