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
