Hello Ferruh,
Thanks for the review, comments inline.
On Mon, Aug 27, 2018 at 8:06 PM, Ferruh Yigit <ferruh.yi...@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 <iryz...@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 APIs
> 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" – 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 – I think it's better to save that function, 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 = 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?
>
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
