Re: [dpdk-dev] [PATCH v2] kni: dynamically allocate memory for each KNI

[Igor Ryzhov <iryzhov@nfware.com>]

Hi Ferruh,

Will fix it today.

Igor

On Wed, Sep 26, 2018 at 1:41 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 9/23/2018 8:12 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,
>
> Good cleanup, thanks.
> +1 to eal_tailq for ctx
>
> A few minor comments below, but they are not significant enough to block
> the
> patch, please let us know if you don't have bandwidth for a new version.
>
> > Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> <...>
>
> > @@ -294,41 +180,52 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
> >  {
> >       int ret;
> >       struct rte_kni_device_info dev_info;
> > -     struct rte_kni *ctx;
> > -     char intf_name[RTE_KNI_NAMESIZE];
> > -     const struct rte_memzone *mz;
> > -     struct rte_kni_memzone_slot *slot = NULL;
> > +     struct rte_kni *kni;
> > +     struct rte_tailq_entry *te = NULL;
> > +     struct rte_kni_list *kni_list;
> > +
> > +     kni_list = RTE_TAILQ_CAST(rte_kni_tailq.head, rte_kni_list);
>
> Can you move this below input validation, no need this assignment if API
> will
> fail because of wrong input.
>
> >       if (!pktmbuf_pool || !conf || !conf->name[0])
> >               return NULL;
> >
> >       /* Check if KNI subsystem has been initialized */
> > -     if (kni_memzone_pool.initialized != 1) {
> > +     if (kni_fd < 0) {
> >               RTE_LOG(ERR, KNI, "KNI subsystem has not been initialized.
> Invoke rte_kni_init() first\n");
> >               return NULL;
> >       }
> >
> > -     /* Get an available slot from the pool */
> > -     slot = kni_memzone_pool_alloc();
> > -     if (!slot) {
> > -             RTE_LOG(ERR, KNI, "Cannot allocate more KNI interfaces;
> increase the number of max_kni_ifaces(current %d) or release unused
> ones.\n",
> > -                     kni_memzone_pool.max_ifaces);
> > -             return NULL;
> > +     rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
> > +
> > +     TAILQ_FOREACH(te, kni_list, next) {
> > +             kni = (struct rte_kni *) te->data;
> > +             if (strncmp(conf->name, kni->name, RTE_KNI_NAMESIZE) == 0)
> > +                     break;
> >       }
>
> This is rte_kni_get(), why not reuse it. You can create an version of it
> without
> lock.
>
> like _rte_kni_get() which you can call here.
>
> And
> rte_kni_get()
>         rte_rwlock_read_lock(RTE_EAL_TAILQ_RWLOCK);
>         _rte_kni_get()
>         rte_rwlock_read_unlock(RTE_EAL_TAILQ_RWLOCK);
>
> <...>
>
> > +
> > +     if (te == NULL) {
> > +             goto unlock;
> > +     }
>
> No need {} for single line.
> One more below.
>