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