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

[Marc Sune <marc.sune@bisdn.de>]

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

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

Many thanks
Marc