Hi Marc Thanks for your clarification! I added more comments.
> -----Original Message----- > From: Marc Sune [mailto:marc.sune at bisdn.de] > Sent: Thursday, October 9, 2014 3:53 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, > > Thanks for the comments. Inline and snipped > > On 09/10/14 09:32, Zhang, Helin wrote: > > Hi Marc > > > > Good explanation! Thank you very much! I add more comments for your code > changes. > > One common comment for annotation, in DPDK, we do not use "//" to start > annotation. > > OK, "//" => "/* */" > > > [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 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. > > 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. > > > >>>> Signed-off-by: Marc Sune <marc.sune at bisdn.de> > >>>> --- > >>>> lib/librte_kni/rte_kni.c | 302 > >>>> ++++++++++++++++++++++++++++++++++++++-------- > >>>> lib/librte_kni/rte_kni.h | 18 +++ > >>>> 2 files changed, 269 insertions(+), 51 deletions(-) > >>>> > >>>> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c > >>>> index > >>>> 76feef4..df55789 100644 > >>>> --- a/lib/librte_kni/rte_kni.c > >>>> +++ b/lib/librte_kni/rte_kni.c > >>>> @@ -40,6 +40,7 @@ > >>>> #include <unistd.h> > >>>> #include <sys/ioctl.h> > >>>> > >>>> +#include <rte_spinlock.h> > >>>> #include <rte_string_fns.h> > >>>> #include <rte_ethdev.h> > >>>> #include <rte_malloc.h> > >>>> @@ -58,7 +59,7 @@ > >>>> > >>>> #define KNI_REQUEST_MBUF_NUM_MAX 32 > >>>> > >>>> -#define KNI_MZ_CHECK(mz) do { if (mz) goto fail; } while (0) > >>>> +#define KNI_MEM_CHECK(cond) do { if (cond) goto kni_fail; } while > >>>> +(0) > >>>> > >>>> /** > >>>> * KNI context > >>>> @@ -66,6 +67,7 @@ > >>>> struct rte_kni { > >>>> char name[RTE_KNI_NAMESIZE]; /**< KNI interface > name > >> */ > >>>> uint16_t group_id; /**< Group ID of KNI > devices > >> */ > >>>> + unsigned slot_id; /**< KNI pool slot ID */ > > It would be better to use uint16_t or similar, as that's DPDK style. > I usually use uint16_t (or unsigned int, not unsinged) but the original source > code (rte_kni.c I think, but I could have looked up something > else) was using already unsigned, so I tried to mimic that. Please clarify > which > is the standard!! I can see most of code lines are using uint16_t/uint32_t or similar, but some may use others which is possibly required by kernel space. KNI will transfer a struct from user space to kernel space. My observation is that DPDK tries to use uint16_t like types as most as possible, though no rule for that I guess. > > > >>>> struct rte_mempool *pktmbuf_pool; /**< pkt mbuf mempool > */ > >>>> unsigned mbuf_size; /**< mbuf size */ > >>>> > >>>> @@ -88,10 +90,48 @@ enum kni_ops_status { > >>>> KNI_REQ_REGISTERED, > >>>> }; > >>>> > >>>> +/** > >>>> +* KNI memzone pool slot > >>>> +*/ > >>>> +struct rte_kni_memzone_slot{ > >>>> + unsigned id; > >>>> + uint8_t in_use : 1; /**< slot in use */ > >>>> + > >>>> + //Memzones > > The comments style is not DPDK style, please try to use DPDK style as > > others. > > I will use in v2: > > /* > * > */ > > and /* */ for inline. > > > >>>> + const struct rte_memzone *m_ctx; /**< KNI ctx */ > >>>> + const struct rte_memzone *m_tx_q; /**< TX queue */ > >>>> + const struct rte_memzone *m_rx_q; /**< RX queue */ > >>>> + const struct rte_memzone *m_alloc_q; /**< Allocated mbufs > queue */ > >>>> + const struct rte_memzone *m_free_q; /**< To be freed mbufs > queue > >>>> */ > >>>> + const struct rte_memzone *m_req_q; /**< Request queue */ > >>>> + const struct rte_memzone *m_resp_q; /**< Response queue */ > >>>> + const struct rte_memzone *m_sync_addr; > >>>> + > >>>> + /* Free linked list */ > >>>> + struct rte_kni_memzone_slot *next; /**< Next slot link.list > >>>> */ > > For the linked list management, "TAILQ_" might be a better choice. > > Please check if it can be usable here. > > I will check it for patch v2 > > > > >>>> +}; > >>>> + > >>>> +/** > >>>> +* KNI memzone pool > >>>> +*/ > >>>> +struct rte_kni_memzone_pool{ > >>>> + uint8_t initialized : 1; /**< Global KNI pool init > >>>> flag */ > >>>> + > >>>> + unsigned max_ifaces; /**< Max. num of KNI ifaces > */ > >>>> + struct rte_kni_memzone_slot *slots; /**< Pool slots */ > >>>> + rte_spinlock_t mutex; /**< alloc/relase mutex */ > >>>> + > >>>> + //Free memzone slots linked-list > >>>> + struct rte_kni_memzone_slot *free; /**< First empty slot > */ > >>>> + struct rte_kni_memzone_slot *free_tail; /**< Last empty slot > >>>> */ > >>>> +}; > >>>> + > >>>> + > >>>> static void kni_free_mbufs(struct rte_kni *kni); static void > >>>> kni_allocate_mbufs(struct rte_kni *kni); > >>>> > >>>> static volatile int kni_fd = -1; > >>>> +static struct rte_kni_memzone_pool kni_memzone_pool = {0}; > >>>> > >>>> static const struct rte_memzone * > >>>> kni_memzone_reserve(const char *name, size_t len, int socket_id, > >>>> @@ > >>>> -105,6 +145,154 @@ kni_memzone_reserve(const char *name, size_t > >>>> len, int socket_id, > >>>> return mz; > >>>> } > >>>> > >>>> +/* Pool mgmt */ > >>>> +static struct rte_kni_memzone_slot* > >>>> +kni_memzone_pool_alloc(void) > >>>> +{ > >>>> + struct rte_kni_memzone_slot* slot; > >>>> + > >>>> + rte_spinlock_lock(&kni_memzone_pool.mutex); > >>>> + > >>>> + if(!kni_memzone_pool.free) { > >>>> + rte_spinlock_unlock(&kni_memzone_pool.mutex); > >>>> + return NULL; > >>>> + } > >>>> + > >>>> + slot = kni_memzone_pool.free; > >>>> + kni_memzone_pool.free = slot->next; > >>>> + > >>>> + if(!kni_memzone_pool.free) > >>>> + kni_memzone_pool.free_tail = NULL; > >>>> + > >>>> + rte_spinlock_unlock(&kni_memzone_pool.mutex); > >>>> + > >>>> + return slot; > >>>> +} > >>>> + > >>>> +static void > >>>> +kni_memzone_pool_dealloc(struct rte_kni_memzone_slot* slot) { > > Generally we don't use "dealloc" like, how about "release"? Just want > > to get it be similar to the existing code. > > I have no problem on changing that. v2 > > > Thanks and regards > > > [snip] > > Regards, Helin
