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 9D05B4CBD for ; Mon, 27 Aug 2018 19:06:36 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Aug 2018 10:06:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,296,1531810800"; d="scan'208";a="68227063" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.56]) ([10.237.221.56]) by orsmga007.jf.intel.com with ESMTP; 27 Aug 2018 10:06:34 -0700 To: Igor Ryzhov , dev@dpdk.org References: <20180802142522.57900-1-iryzhov@nfware.com> From: Ferruh Yigit Openpgp: preference=signencrypt Message-ID: <0b076a74-579e-5a63-d232-1afc791fb4cd@intel.com> Date: Mon, 27 Aug 2018 18:06:33 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180802142522.57900-1-iryzhov@nfware.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] kni: dynamically allocate memory for each KNI X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Aug 2018 17:06:37 -0000 On 8/2/2018 3:25 PM, Igor Ryzhov wrote: > Long time ago preallocation of memory for KNI was introduced in commit > 0c6bc8e. It was done because of lack of ability to free previously > allocated memzones, which led to memzone exhaustion. Currently memzones > can be freed and this patch uses this ability for dynamic KNI memory > allocation. Hi Igor, It is good to be able to allocate memory dynamically and get rid of the "max_kni_ifaces" and "kni_memzone_pool", thanks for the patch. Overall looks good, a few comments below. > > Signed-off-by: Igor Ryzhov > --- > lib/librte_kni/rte_kni.c | 392 ++++++++++++--------------------------- > lib/librte_kni/rte_kni.h | 6 +- > test/test/test_kni.c | 6 - > 3 files changed, 128 insertions(+), 276 deletions(-) > > diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c > index 8a8f6c1cc..028b44bfd 100644 > --- a/lib/librte_kni/rte_kni.c > +++ b/lib/librte_kni/rte_kni.c > @@ -36,24 +36,33 @@ > * KNI context > */ > struct rte_kni { > + const struct rte_memzone *mz; /**< KNI context memzone */ I was thinking remove the context memzone and use rte_zmalloc() to create kni objects but updated rte_kni_get() API seems relaying this. If you see any other way to get kni object from name in rte_kni_get(), I am for removing above *mz variable from rte_kni struct. <...> > +static void > +kni_ctx_release_mz(struct rte_kni *ctx) > +{ > + rte_memzone_free(ctx->m_tx_q); > + rte_memzone_free(ctx->m_rx_q); > + rte_memzone_free(ctx->m_alloc_q); > + rte_memzone_free(ctx->m_free_q); > + rte_memzone_free(ctx->m_req_q); > + rte_memzone_free(ctx->m_resp_q); > + rte_memzone_free(ctx->m_sync_addr); "ctx" sounds confusing to me, isn't this "rte_kni" object instance, why not just call it "kni" or if it is too generic "kni_obj" or similar? For other APIs as well. And this is just a detail but about order of APIs would you mind having first reserve() one, later release() one? <...> > -/* Shall be called before any allocation happens */ > -void > -rte_kni_init(unsigned int max_kni_ifaces) > +static struct rte_kni * > +kni_ctx_reserve(const char *name) > { > - uint32_t i; > - struct rte_kni_memzone_slot *it; > + struct rte_kni *ctx; > const struct rte_memzone *mz; > -#define OBJNAMSIZ 32 > - char obj_name[OBJNAMSIZ]; > char mz_name[RTE_MEMZONE_NAMESIZE]; > > - /* Immediately return if KNI is already initialized */ > - if (kni_memzone_pool.initialized) { > - RTE_LOG(WARNING, KNI, "Double call to rte_kni_init()"); > - return; > - } > + snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "kni_info_%s", name); Can you please convert memzone names, like "kni_info" to defines, for all of them? <...> > @@ -81,8 +81,12 @@ struct rte_kni_conf { > * > * @param max_kni_ifaces > * The maximum number of KNI interfaces that can coexist concurrently > + * > + * @return > + * - 0 indicates success. > + * - negative value indicates failure. > */ > -void rte_kni_init(unsigned int max_kni_ifaces); > +int rte_kni_init(unsigned int max_kni_ifaces); This changes the API. Return type changes from "void" to "int". I agree "int" makes more sense since API can fail, but this changes the ABI/API. Since existing binaries doesn't check the return type at all there may be no issue from ABI point of view but from API point of view some apps may get return value not checked warnings, not sure though. And the need of the API is questionable at this stage, it may be possible to move rte_kni_alloc() where it already has "kni_fd" check. What do you think keep API signature same for now, but add a deprecation notice to remove the API. Next release (v19.02) remove rte_kni_init() completely? <...> > /** > diff --git a/test/test/test_kni.c b/test/test/test_kni.c > index 1b876719a..56c98513a 100644 > --- a/test/test/test_kni.c > +++ b/test/test/test_kni.c > @@ -429,12 +429,6 @@ test_kni_processing(uint16_t port_id, struct rte_mempool *mp) > } > test_kni_ctx = NULL; > > - /* test of releasing a released kni device */ > - if (rte_kni_release(kni) == 0) { > - printf("should not release a released kni device\n"); > - return -1; > - } Why need to remove this?