From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 1FB9D6A96 for ; Thu, 9 Oct 2014 10:26:29 +0200 (CEST) Received: from azsmga001.ch.intel.com ([10.2.17.19]) by fmsmga101.fm.intel.com with ESMTP; 09 Oct 2014 01:33:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,683,1406617200"; d="scan'208";a="483004546" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by azsmga001.ch.intel.com with ESMTP; 09 Oct 2014 01:33:32 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.195.1; Thu, 9 Oct 2014 01:33:31 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.195.1; Thu, 9 Oct 2014 01:33:31 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.230]) by shsmsx102.ccr.corp.intel.com ([169.254.2.192]) with mapi id 14.03.0195.001; Thu, 9 Oct 2014 16:33:30 +0800 From: "Zhang, Helin" To: Marc Sune Thread-Topic: [dpdk-dev] [PATCH] KNI: use a memzone pool for KNI alloc/release Thread-Index: AQHP285+iUgjNWB580Cf22Ir3DYXhpwnUh3w//+O9YCAAIoo0P//gxyAgACOkEA= Date: Thu, 9 Oct 2014 08:33:29 +0000 Message-ID: References: <1411985756-2744-1-git-send-email-marc.sune@bisdn.de> <543633B5.3020101@bisdn.de> <54363ED5.9060304@bisdn.de> In-Reply-To: <54363ED5.9060304@bisdn.de> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 08:26:31 -0000 Hi Marc Thanks for your clarification! I added more comments. > -----Original Message----- > From: Marc Sune [mailto:marc.sune@bisdn.de] > Sent: Thursday, October 9, 2014 3:53 PM > To: Zhang, Helin > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] KNI: use a memzone pool for KNI alloc/rel= ease >=20 > Hi Helin, >=20 > Thanks for the comments. Inline and snipped >=20 > On 09/10/14 09:32, Zhang, Helin wrote: > > Hi Marc > > > > Good explanation! Thank you very much! I add more comments for your cod= e > changes. > > One common comment for annotation, in DPDK, we do not use "//" to start > annotation. >=20 > OK, "//" =3D> "/* */" >=20 > > [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 don= e 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. >=20 > 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 #de= fine > 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, t= hen no changes are needed in user app. >=20 > 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_X= EN_DOM0), XEN support is different from standard Linux support. > > > >>>> 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 s= ource > 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 clarif= y which > is the standard!! I can see most of code lines are using uint16_t/uint32_t or similar, but so= me 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 pos= sible, 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 o= thers. >=20 > I will use in v2: >=20 > /* > * > */ >=20 > 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. >=20 > I will check it for patch v2 >=20 > > > >>>> +}; > >>>> + > >>>> +/** > >>>> +* 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 =3D -1; > >>>> +static struct rte_kni_memzone_pool kni_memzone_pool =3D {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 =3D kni_memzone_pool.free; > >>>> + kni_memzone_pool.free =3D slot->next; > >>>> + > >>>> + if(!kni_memzone_pool.free) > >>>> + kni_memzone_pool.free_tail =3D 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. >=20 > I have no problem on changing that. v2 >=20 >=20 > Thanks and regards >=20 > > [snip] > > Regards, Helin