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
