From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx.bisdn.de (mx.bisdn.de [185.27.182.31]) by dpdk.org (Postfix) with ESMTP id DED4A68CE for ; Thu, 9 Oct 2014 09:45:10 +0200 (CEST) Received: from [192.168.1.101] (f053064184.adsl.alicedsl.de [78.53.64.184]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx.bisdn.de (Postfix) with ESMTPSA id 42EFFA2700; Thu, 9 Oct 2014 09:52:31 +0200 (CEST) Message-ID: <54363ED5.9060304@bisdn.de> Date: Thu, 09 Oct 2014 09:52:53 +0200 From: Marc Sune User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20131103 Icedove/17.0.10 MIME-Version: 1.0 To: "Zhang, Helin" References: <1411985756-2744-1-git-send-email-marc.sune@bisdn.de> <543633B5.3020101@bisdn.de> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] KNI: use a memzone pool for KNI alloc/release X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Oct 2014 07:45:11 -0000 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 >>>> --- >>>> 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 >>>> #include >>>> >>>> +#include >>>> #include >>>> #include >>>> #include >>>> @@ -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] >