From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-f193.google.com (mail-pg1-f193.google.com [209.85.215.193]) by dpdk.org (Postfix) with ESMTP id 91DA8F11 for ; Wed, 29 Aug 2018 11:52:49 +0200 (CEST) Received: by mail-pg1-f193.google.com with SMTP id g20-v6so2094720pgv.6 for ; Wed, 29 Aug 2018 02:52:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nfware-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=/Pz8mCjEbdAOPWNz8sPWaFog8PUb7Qm24vtb3QC8Upo=; b=yOa3WrI0JwDxg5cf9VvoPxlU8z3Jl3oZPQri4UxDtwHHK1YcUpReb+W9CQB86qVcDt SxHKSNQmb+9hAgfOx6QDMutQr5Lvl1bzgKOf7z4fqh0KdSsp4Zc7lkPDm+cNpEIqv4J7 W/w/e498gb4OR2MPhDzEyEwxYTJRLguztfVGGN68BXvZW7T+lIGF5fzkSefaf1nLvUhd XWPqEX2sdoRU5wIONuJ5zeBPuhIno7ZpLudG0HvSXIJZ0W6PD6+czyd8GB7A0kbXZ8SZ iihPTLL5uBRbR5G0nz93HN9HaEEcfbqp64Wuv6356B2p+I2NnD9bcd2HgfKtM2s2lg7J Gfag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=/Pz8mCjEbdAOPWNz8sPWaFog8PUb7Qm24vtb3QC8Upo=; b=YQqb/T3TrWd5MUq57q5AoseRI/v/gZ3guvjb7wcY15bfNh2GPEQ/0JCUl0lyovLTRF uhQ1Sbcf6Di4pxYan5c4UV4Tm8Sm1GppW8hH/rIn9YBhcp/0zZlg9UttyOuQUkZZqPSM oYfHNfn9WwjnGbw1CvuRcgzBr4C5YK0ABQ4KrRbSQ33tTpGiX5IXlBccCg3E+ZEg+AUf ziIfZTiOKZF1wLYQm2CKAgQRMC3aYtH7pf0OdLlg5l8gH90UdQ6pV/ZMUD2Di3PXqLU8 EwINHMp8Jmg8ZiFjO4Tc48/nn3KH9U5jFYZajgcWIB7JB/xzDRzygG7JZVxElqfd5mKZ vy0g== X-Gm-Message-State: APzg51CX+unRAEvlsfNYmZoBkDcYey7jMQ9v+bkrs/Q8IDjqZsVfbjyP HvABDW0zHCWW6TZf1YiFBEuPYKChL68P3WT7JU+Xrg== X-Google-Smtp-Source: ANB0VdbFGca+HKZFRk7cfg0k/wpX4mGFHAIBUG6nWmEGn2QfC4yw0FyWUgnj0gFda67gMkVw0fnA/JS2loo9HXip/BA= X-Received: by 2002:a63:f751:: with SMTP id f17-v6mr5052783pgk.410.1535536368817; Wed, 29 Aug 2018 02:52:48 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a17:90a:309:0:0:0:0 with HTTP; Wed, 29 Aug 2018 02:52:48 -0700 (PDT) In-Reply-To: <0b076a74-579e-5a63-d232-1afc791fb4cd@intel.com> References: <20180802142522.57900-1-iryzhov@nfware.com> <0b076a74-579e-5a63-d232-1afc791fb4cd@intel.com> From: Igor Ryzhov Date: Wed, 29 Aug 2018 12:52:48 +0300 Message-ID: To: Ferruh Yigit Cc: dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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: Wed, 29 Aug 2018 09:52:50 -0000 Hello Ferruh, Thanks for the review, comments inline. On Mon, Aug 27, 2018 at 8:06 PM, Ferruh Yigit wrote: > 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. > I had absolutely the same thought but didn't find a way to save rte_kni_get() API. Maybe someone has any ideas? Or maybe this API can be marked deprecated and deleted in future? > > <...> > > > +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 API= s > as well. > "ctx" was already used in the code, so I didn't change it. I also think that it's better to use "kni" =E2=80=93 will change it in v2. > > And this is just a detail but about order of APIs would you mind having > first > reserve() one, later release() one? > Ok. > > <...> > > > -/* 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? > Ok. > > <...> > > > @@ -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= ? > As I know, warnings can only be returned if the warn_unused_result attribute is used, which is not the case here. So I think that changing from void to int should not break anything. Can change it back in v2 if I'm wrong. Regarding the API removal =E2=80=93 I think it's better to save that functi= on, to have a more clear API. As we have rte_kni_close to close KNI device, we should have a function to open it. Maybe it should be renamed to rte_kni_open :) > > <...> > > > /** > > 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 =3D NULL; > > > > - /* test of releasing a released kni device */ > > - if (rte_kni_release(kni) =3D=3D 0) { > > - printf("should not release a released kni device\n"); > > - return -1; > > - } > > Why need to remove this? > Previously, rte_kni_release didn't free any memory, and the second call with the same argument just checked "in_use" flag and returned. After my changes, memory is actually freed, and rte_kni_release must not be called twice with the same argument. Will send v2 with approved changes in a couple of days. At the same time, I'll think what can we do with context memzone. Best regards, Igor