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

[Zhang, Helin]

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