DPDK patches and discussions
 help / color / mirror / Atom feed
From: Marc Sune <marc.sune@bisdn.de>
To: "Zhang, Helin" <helin.zhang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] KNI: use a memzone pool for KNI alloc/release
Date: Thu, 09 Oct 2014 09:52:53 +0200	[thread overview]
Message-ID: <54363ED5.9060304@bisdn.de> (raw)
In-Reply-To: <F35DEAC7BCE34641BA9FAC6BCA4A12E70A79A1BA@SHSMSX104.ccr.corp.intel.com>

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)?

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.

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?
>
>>>> Signed-off-by: Marc Sune <marc.sune@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!!
>
>>>>    	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]
>

  reply	other threads:[~2014-10-09  7:45 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-29 10:15 Marc Sune
2014-10-09  6:01 ` Zhang, Helin
2014-10-09  7:05   ` Marc Sune
2014-10-09  7:32     ` Zhang, Helin
2014-10-09  7:52       ` Marc Sune [this message]
2014-10-09  8:33         ` Zhang, Helin
2014-10-09  8:45           ` Marc Sune
2014-10-09  8:57             ` Zhang, Helin
2014-10-09 10:15               ` Marc Sune
2014-10-10  5:25                 ` Zhang, Helin
2014-10-10  6:37                   ` Marc Sune
2014-10-10  7:35                     ` Zhang, Helin
2014-10-10  9:02                       ` Marc Sune

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54363ED5.9060304@bisdn.de \
    --to=marc.sune@bisdn.de \
    --cc=dev@dpdk.org \
    --cc=helin.zhang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).