From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <iryzhov@nfware.com>
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 <dev@dpdk.org>; Wed, 29 Aug 2018 11:52:49 +0200 (CEST)
Received: by mail-pg1-f193.google.com with SMTP id g20-v6so2094720pgv.6
for <dev@dpdk.org>; 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 <iryzhov@nfware.com>
Date: Wed, 29 Aug 2018 12:52:48 +0300
Message-ID: <CAF+s_Fwx4rrfaA4FrkH2CmamwiOxWmv3Sau_8TDRwu=bPgTS9Q@mail.gmail.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <ferruh.yigit@intel.com>
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 <iryzhov@nfware.com>
> > ---
> > 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