Re: [dpdk-dev] [PATCH] port: add kni interface support

[Ethan <zhuangwj@gmail.com>]

Hi Cristian,

The new patch has been submitted just now. Please note that I do ignore
some check patch errors this time.

B.R.
Ethan


2016-06-13 21:18 GMT+08:00 Dumitrescu, Cristian <
cristian.dumitrescu@intel.com>:

> Hi Ethan,
>
>
>
> Great, we’ll wait for your patch later this week then. I recommend you add
> any other changes that you might have on top of the latest code that I just
> send, as this will minimize your work, my work to further code reviews and
> number of future iterations to merge this patch.
>
>
>
> Answers to your questions are inlined below.
>
>
>
> Regards,
>
> Cristian
>
>
>
> *From:* zhuangweijie@gmail.com [mailto:zhuangweijie@gmail.com] *On Behalf
> Of *Ethan
> *Sent:* Monday, June 13, 2016 11:48 AM
> *To:* Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> *Cc:* dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>; Yigit,
> Ferruh <ferruh.yigit@intel.com>
> *Subject:* Re: [PATCH] port: add kni interface support
>
>
>
> Hi Cristian,
>
>
>
> I've got your comments. Thank you for review the code from a DPDK newbie.
> :-)
>
> I plan to submit a new patch to fix all during this week hopefully.
>
>
>
> There are four places I'd like to discuss further:
>
>
>
> 1. Dedicated lcore for kni kernel thread
>
> First of all, it is a bug to add kni kernel core to the user space core
> mask. What I want is just to check if the kni kernel thread has a dedicated
> core.
>
> The reason I prefer to allocate a dedicated core to kni kernel thread is
> that my application is latency sensitive. I worry the context switch and
> cache miss will cause the latency increasing if the kni kernel thread and
> application thread share one core.
>
> Anyway, I think I should remove the hard coded check because it will be
> more generic. Users who has the similar usage like mine can achieve so
> through configuration file.
>
>
>
> [Cristian] I agree with you that the user should be able to specify the
> core where the kernel thread should run, and this requirement is fully met
> by the latest code I sent, but implemented in a slightly different way,
> which I think it is a cleaner way.
>
>
>
> In your initial solution, the application redefines the meaning of the
> core mask as the reunion of cores used by the user space application (cores
> running the pipelines) and the cores used to run the kernel space KNI
> threads. This does not make sense to me. The application is in user space
> and it does not start or manage any kernel threads itself, why should the
> application worry about the cores running kernel threads? The application
> should just pick up the user instructions from the config file and send
> them to the KNI kernel module transparently.
>
>
>
> In the code that I just sent, the application preserves the current
> definition of the core mask, i.e. just the collection of cores running the
> pipelines. This leads to simpler code that meets all the requirements for
> kernel threads affinity:
>
> i) The user wants to affinitize the kernel thread to a CPU core that is
> not used to run any pipeline (this core will run just KNI kernel threads):
> Core entry in KNI section is set to be different than the core entry of any
> PIPELINE section in the config file;
>
> ii) The user affinitizes the kernel thread to a CPU core that also runs
> some of the pipelines (this core will run both user space and kernel space
> threads): Core entry in KNI section is equal to the core entry in one or
> several of the PIPELINE sections in the config file;
>
> iii) The user does not affinitize the kernel thread to any CPU core, so
> the kernel decides the scheduling policy for the KNI threads: Core entry of
> the KNI section is not present; this results in force_bind KNI parameter to
> be set to 0.
>
>
>
> Makes sense?
>
>
>
> 2. The compiler error of the Macro RTE_PORT_KNI_WRITER_STATS_PKTS_IN_ADD
>
> Actually I implements the macro similar
> to RTE_PORT_RING_READER_STATS_PKTS_IN_ADD first. But the
> scripts/checkpatches.sh fails: ERROR:COMPLEX_MACRO: Macros with complex
> values should be enclosed in parentheses
>
> I'm not share either I have done something wrong or the checkpatches
> script need an update.
>
>
>
> [Cristian] Let’s use the same consistent rule to create the stats macros
> for all the ports, i.e. follow the existing rule used for other ports. You
> can ignore this check patch issue.
>
>
>
> 3. KNI kernel operations callback
>
> To be  honest, I made reference to the the KNI sample application.
>
> Since there is very little docs tell the difference between link up call
> and device start call, I am not sure which one is better here.
>
> Any help will be appreciate. :-)
>
>
>
> [Cristian] I suggest you use the ones from the code that I just sent.
>
>
>
> 4. Shall I use DPDK_16.07 in the  librte_port/rte_port_version.map file?
>
>
>
> [Cristian] Yes.
>
>
>
>
>
> 2016-06-10 7:42 GMT+08:00 Dumitrescu, Cristian <
> cristian.dumitrescu@intel.com>:
>
> Hi Ethan,
>
> Great work! There are still several comments below that need to be
> addressed, but I am confident we can close on them quickly. Thank you!
>
> Please rebase the next version on top of the latest code on master branch.
>
> Please also update librte_port/rte_port_version.map file.
>
>
>
> Shall I use DPDK_16.07 in the  librte_port/rte_port_version.map file?
>
>
>
>
> > -----Original Message-----
> > From: WeiJie Zhuang [mailto:zhuangwj@gmail.com]
> > Sent: Saturday, May 28, 2016 12:26 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: dev@dpdk.org; WeiJie Zhuang <zhuangwj@gmail.com>
> > Subject: [PATCH] port: add kni interface support
> >
> > 1. add KNI port type to the packet framework
> > 2. add KNI support to the IP Pipeline sample Application
> >
> > Signed-off-by: WeiJie Zhuang <zhuangwj@gmail.com>
> > ---
> > v2:
> > * Fix check patch error.
> > ---
> >  doc/api/doxy-api-index.md           |   1 +
> >  examples/ip_pipeline/Makefile       |   6 +-
> >  examples/ip_pipeline/app.h          |  74 +++++++++
> >  examples/ip_pipeline/config/kni.cfg |  12 ++
> >  examples/ip_pipeline/config_check.c |  34 ++++
> >  examples/ip_pipeline/config_parse.c | 130 +++++++++++++++
> >  examples/ip_pipeline/init.c         |  79 +++++++++
> >  examples/ip_pipeline/kni/kni.c      |  80 +++++++++
> >  examples/ip_pipeline/kni/kni.h      |  16 ++
> >  examples/ip_pipeline/pipeline_be.h  |  13 ++
> >  examples/ip_pipeline/thread.c       |   9 +
> >  lib/librte_port/Makefile            |   7 +
> >  lib/librte_port/rte_port_kni.c      | 316
> > ++++++++++++++++++++++++++++++++++++
> >  lib/librte_port/rte_port_kni.h      |  81 +++++++++
> >  14 files changed, 856 insertions(+), 2 deletions(-)
> >  create mode 100644 examples/ip_pipeline/config/kni.cfg
> >  create mode 100644 examples/ip_pipeline/kni/kni.c
> >  create mode 100644 examples/ip_pipeline/kni/kni.h
> >  create mode 100644 lib/librte_port/rte_port_kni.c
> >  create mode 100644 lib/librte_port/rte_port_kni.h
> >
> > diff --git a/doc/api/doxy-api-index.md b/doc/api/doxy-api-index.md
> > index f626386..e38a959 100644
> > --- a/doc/api/doxy-api-index.md
> > +++ b/doc/api/doxy-api-index.md
> > @@ -119,6 +119,7 @@ There are many libraries, so their headers may be
> > grouped by topics:
> >      [reass]            (@ref rte_port_ras.h),
> >      [sched]            (@ref rte_port_sched.h),
> >      [src/sink]         (@ref rte_port_source_sink.h)
> > +    [kni]              (@ref rte_port_kni.h)
> >    * [table]            (@ref rte_table.h):
> >      [lpm IPv4]         (@ref rte_table_lpm.h),
> >      [lpm IPv6]         (@ref rte_table_lpm_ipv6.h),
> > diff --git a/examples/ip_pipeline/Makefile
> b/examples/ip_pipeline/Makefile
> > index 10fe1ba..848c2aa 100644
> > --- a/examples/ip_pipeline/Makefile
> > +++ b/examples/ip_pipeline/Makefile
> > @@ -43,9 +43,10 @@ include $(RTE_SDK)/mk/rte.vars.mk
> >  # binary name
> >  APP = ip_pipeline
> >
> > +VPATH += $(SRCDIR)/kni
> >  VPATH += $(SRCDIR)/pipeline
> >
> > -INC += $(wildcard *.h) $(wildcard pipeline/*.h)
> > +INC += $(wildcard *.h) $(wildcard pipeline/*.h) $(wildcard kni/*.h)
> >
> >  # all source are stored in SRCS-y
> >  SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) := main.c
> > @@ -56,6 +57,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += init.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += thread.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += thread_fe.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += cpu_core_map.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += kni.c
> >
> >  SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += pipeline_common_be.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += pipeline_common_fe.c
> > @@ -72,7 +74,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=
> > pipeline_flow_actions.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += pipeline_routing_be.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) += pipeline_routing.c
> >
> > -CFLAGS += -I$(SRCDIR) -I$(SRCDIR)/pipeline
> > +CFLAGS += -I$(SRCDIR) -I$(SRCDIR)/pipeline -I$(SRCDIR)/kni
> >  CFLAGS += -O3
> >  CFLAGS += $(WERROR_FLAGS) -Wno-error=unused-function -Wno-
> > error=unused-variable
> >
>
> I would like to avoid creating the kni subfolder. Please move the
> functions from kni/kni.c to init.c file, just before function
> app_init_kni(), where they should be declared as static functions.
>
> > diff --git a/examples/ip_pipeline/app.h b/examples/ip_pipeline/app.h
> > index e775024..a86ce57 100644
> > --- a/examples/ip_pipeline/app.h
> > +++ b/examples/ip_pipeline/app.h
> > @@ -44,7 +44,9 @@
> >  #include <cmdline_parse.h>
> >
> >  #include <rte_ethdev.h>
> > +#include <rte_kni.h>
> >
> > +#include "kni.h"
> >  #include "cpu_core_map.h"
> >  #include "pipeline.h"
> >
> > @@ -99,6 +101,18 @@ struct app_pktq_hwq_out_params {
> >       struct rte_eth_txconf conf;
> >  };
> >
> > +struct app_kni_params {
> > +     char *name;
> > +     uint32_t parsed;
> > +
> > +     uint32_t socket_id;
> > +     uint32_t core_id;
> > +     uint32_t hyper_th_id;
> > +
> > +     uint32_t mempool_id;
>
> Please add the usual comment on the same line: /* Position in the
> app->mempool_params */
>
> > +     uint32_t burst;
>
> Why having a unified value for read and write burst size? Please use
> burst_read and burst_write (both uint32_t) instead and update the
> config_parse.c and init.c code accordingly (small changes).
>
>
> > +};
> > +
> >  struct app_pktq_swq_params {
> >       char *name;
> >       uint32_t parsed;
> > @@ -172,6 +186,7 @@ enum app_pktq_in_type {
> >       APP_PKTQ_IN_SWQ,
> >       APP_PKTQ_IN_TM,
> >       APP_PKTQ_IN_SOURCE,
> > +     APP_PKTQ_IN_KNI,
> >  };
> >
> >  struct app_pktq_in_params {
> > @@ -184,6 +199,7 @@ enum app_pktq_out_type {
> >       APP_PKTQ_OUT_SWQ,
> >       APP_PKTQ_OUT_TM,
> >       APP_PKTQ_OUT_SINK,
> > +     APP_PKTQ_OUT_KNI,
> >  };
> >
> >  struct app_pktq_out_params {
> > @@ -434,6 +450,10 @@ struct app_eal_params {
> >  #define APP_THREAD_HEADROOM_STATS_COLLECT        1
> >  #endif
> >
> > +#ifndef APP_MAX_KNI
> > +#define APP_MAX_KNI                              8
> > +#endif
> > +
> >  struct app_params {
> >       /* Config */
> >       char app_name[APP_APPNAME_SIZE];
> > @@ -457,6 +477,7 @@ struct app_params {
> >       struct app_pktq_sink_params sink_params[APP_MAX_PKTQ_SINK];
> >       struct app_msgq_params msgq_params[APP_MAX_MSGQ];
> >       struct app_pipeline_params pipeline_params[APP_MAX_PIPELINES];
> > +     struct app_kni_params kni_params[APP_MAX_KNI];
> >
> >       uint32_t n_mempools;
> >       uint32_t n_links;
> > @@ -468,6 +489,7 @@ struct app_params {
> >       uint32_t n_pktq_sink;
> >       uint32_t n_msgq;
> >       uint32_t n_pipelines;
> > +     uint32_t n_kni;
> >
> >       /* Init */
> >       char *eal_argv[1 + APP_EAL_ARGC];
> > @@ -480,6 +502,7 @@ struct app_params {
> >       struct pipeline_type pipeline_type[APP_MAX_PIPELINE_TYPES];
> >       struct app_pipeline_data pipeline_data[APP_MAX_PIPELINES];
> >       struct app_thread_data thread_data[APP_MAX_THREADS];
> > +     struct rte_kni *kni[APP_MAX_KNI];
> >       cmdline_parse_ctx_t cmds[APP_MAX_CMDS + 1];
> >
> >       int eal_argc;
> > @@ -738,6 +761,31 @@ app_msgq_get_readers(struct app_params *app,
> > struct app_msgq_params *msgq)
> >  }
> >
> >  static inline uint32_t
> > +app_kni_get_readers(struct app_params *app, struct app_kni_params
> > *kni)
> > +{
> > +     uint32_t pos = kni - app->kni_params;
> > +     uint32_t n_pipelines = RTE_MIN(app->n_pipelines,
> > +             RTE_DIM(app->pipeline_params));
> > +     uint32_t n_readers = 0, i;
> > +
> > +     for (i = 0; i < n_pipelines; i++) {
> > +             struct app_pipeline_params *p = &app->pipeline_params[i];
> > +             uint32_t n_pktq_in = RTE_MIN(p->n_pktq_in, RTE_DIM(p-
> > >pktq_in));
> > +             uint32_t j;
> > +
> > +             for (j = 0; j < n_pktq_in; j++) {
> > +                     struct app_pktq_in_params *pktq = &p->pktq_in[j];
> > +
> > +                     if ((pktq->type == APP_PKTQ_IN_KNI) &&
> > +                             (pktq->id == pos))
> > +                             n_readers++;
> > +             }
> > +     }
> > +
> > +     return n_readers;
> > +}
> > +
> > +static inline uint32_t
> >  app_txq_get_writers(struct app_params *app, struct
> > app_pktq_hwq_out_params *txq)
> >  {
> >       uint32_t pos = txq - app->hwq_out_params;
> > @@ -863,6 +911,32 @@ app_msgq_get_writers(struct app_params *app,
> > struct app_msgq_params *msgq)
> >       return n_writers;
> >  }
> >
> > +static inline uint32_t
> > +app_kni_get_writers(struct app_params *app, struct app_kni_params *kni)
> > +{
> > +     uint32_t pos = kni - app->kni_params;
> > +     uint32_t n_pipelines = RTE_MIN(app->n_pipelines,
> > +             RTE_DIM(app->pipeline_params));
> > +     uint32_t n_writers = 0, i;
> > +
> > +     for (i = 0; i < n_pipelines; i++) {
> > +             struct app_pipeline_params *p = &app->pipeline_params[i];
> > +             uint32_t n_pktq_out = RTE_MIN(p->n_pktq_out,
> > +                     RTE_DIM(p->pktq_out));
> > +             uint32_t j;
> > +
> > +             for (j = 0; j < n_pktq_out; j++) {
> > +                     struct app_pktq_out_params *pktq = &p-
> > >pktq_out[j];
> > +
> > +                     if ((pktq->type == APP_PKTQ_OUT_KNI) &&
> > +                             (pktq->id == pos))
> > +                             n_writers++;
> > +             }
> > +     }
> > +
> > +     return n_writers;
> > +}
> > +
> >  static inline struct app_link_params *
> >  app_get_link_for_rxq(struct app_params *app, struct
> > app_pktq_hwq_in_params *p)
> >  {
> > diff --git a/examples/ip_pipeline/config/kni.cfg
> > b/examples/ip_pipeline/config/kni.cfg
> > new file mode 100644
> > index 0000000..30466b0
> > --- /dev/null
> > +++ b/examples/ip_pipeline/config/kni.cfg
> > @@ -0,0 +1,12 @@
> > +[KNI0]
> > +core = 2
> > +
> > +[PIPELINE0]
> > +type = MASTER
> > +core = 0
> > +
> > +[PIPELINE1]
> > +type = PASS-THROUGH
> > +core = 1
> > +pktq_in = RXQ0.0 KNI0
> > +pktq_out = KNI0 TXQ0.0
>
> I don't think this new config file config/kni.cfg is really useful, so I
> would like to remove it. We should avoid the proliferation of
> straightforward config files.
>
>
> > diff --git a/examples/ip_pipeline/config_check.c
> > b/examples/ip_pipeline/config_check.c
> > index fd9ff49..3e300f9 100644
> > --- a/examples/ip_pipeline/config_check.c
> > +++ b/examples/ip_pipeline/config_check.c
> > @@ -426,6 +426,39 @@ check_pipelines(struct app_params *app)
> >       }
> >  }
> >
> > +static void
> > +check_kni(struct app_params *app) {
> > +     uint32_t i;
> > +     uint32_t port_id;
> > +
> > +     for (i = 0; i < app->n_kni; i++) {
> > +             struct app_kni_params *p = &app->kni_params[i];
> > +             uint32_t n_readers = app_kni_get_readers(app, p);
> > +             uint32_t n_writers = app_kni_get_writers(app, p);
> > +
> > +             APP_CHECK((n_readers != 0),
> > +                               "%s has no reader\n", p->name);
> > +
> > +             if (n_readers > 1)
> > +                     APP_LOG(app, LOW,
> > +                                     "%s has more than one reader", p-
> > >name);
> > +
> > +             APP_CHECK((n_writers != 0),
> > +                               "%s has no writer\n", p->name);
> > +
> > +             if (n_writers > 1)
> > +                     APP_LOG(app, LOW,
> > +                                     "%s has more than one writer", p-
> > >name);
> > +
>
> We should remove the next two checks. The reason is that in the latest
> code in config_parse.c (already merged on master branch), we automatically
> add LINKx for every object associated with it , such as RXQx.y, TXQx.y,
> TMx. This is the same for KNI, as KNIx is associated with LINKx. As also
> commented below, we should implement this in config_parse.c. Basically, due
> to this change in config_parse.c, it is always guaranteed that for every
> KNIx object, the LINKx object exists as well, so no need to check this here
> in config_check.c or in init.c.
>
>
> > +             APP_CHECK(sscanf(p->name, "KNI%" PRIu32, &port_id) == 1,
> > +                               "%s's port id is invalid\n", p->name);
> > +
> > +             APP_CHECK(port_id < app->n_links,
> > +                               "kni %s is not associated with a valid
> link\n",
> > +                               p->name);
> > +     }
> > +}
> > +
> >  int
> >  app_config_check(struct app_params *app)
> >  {
> > @@ -439,6 +472,7 @@ app_config_check(struct app_params *app)
> >       check_sinks(app);
> >       check_msgqs(app);
> >       check_pipelines(app);
> > +     check_kni(app);
> >
> >       return 0;
> >  }
> > diff --git a/examples/ip_pipeline/config_parse.c
> > b/examples/ip_pipeline/config_parse.c
> > index e5efd03..e9cd5a4 100644
> > --- a/examples/ip_pipeline/config_parse.c
> > +++ b/examples/ip_pipeline/config_parse.c
> > @@ -209,6 +209,15 @@ struct app_pipeline_params
> > default_pipeline_params = {
> >       .n_args = 0,
> >  };
> >
> > +struct app_kni_params default_kni_params = {
> > +     .parsed = 0,
> > +     .socket_id = 0,
> > +     .core_id = 0,
> > +     .hyper_th_id = 0,
> > +     .mempool_id = 0,
> > +     .burst = 32,
>
> .burst_read = 32,
> .burst_write = 32,
>
>
> > +};
> > +
> >  static const char app_usage[] =
> >       "Usage: %s [-f CONFIG_FILE] [-s SCRIPT_FILE] [-p PORT_MASK] "
> >       "[-l LOG_LEVEL] [--preproc PREPROCESSOR] [--preproc-args
> > ARGS]\n"
> > @@ -1169,6 +1178,9 @@ parse_pipeline_pktq_in(struct app_params *app,
> >               } else if (validate_name(name, "SOURCE", 1) == 0) {
> >                       type = APP_PKTQ_IN_SOURCE;
> >                       id = APP_PARAM_ADD(app->source_params, name);
> > +             } else if (validate_name(name, "KNI", 1) == 0) {
> > +                     type = APP_PKTQ_IN_KNI;
> > +                     id = APP_PARAM_ADD(app->kni_params, name);
> >               } else
> >                       return -EINVAL;
> >
> > @@ -1240,6 +1252,9 @@ parse_pipeline_pktq_out(struct app_params *app,
> >               } else if (validate_name(name, "SINK", 1) == 0) {
> >                       type = APP_PKTQ_OUT_SINK;
> >                       id = APP_PARAM_ADD(app->sink_params, name);
> > +             } else if (validate_name(name, "KNI", 1) == 0) {
> > +                     type = APP_PKTQ_OUT_KNI;
> > +                     id = APP_PARAM_ADD(app->kni_params, name);
> >               } else
> >                       return -EINVAL;
> >
> > @@ -2459,6 +2474,76 @@ parse_msgq(struct app_params *app,
> >       free(entries);
> >  }
> >
>
> Please rework the below parse_kni() function based on the latest code
> (rebase). For example, the PARSER_PARAM_ADD_CHECK macro has been removed.
>
> Also, as mentioned above, please add LINKx automatically for KNIx, same as
> the latest code adds LINKx automatically every time RXQx.y, TXQx.y, TMx
> objects are met. This has to be done in several places: once here, in
> function parse_kni(), which is executed for KNI sections, but also in
> parse_pipeline_pktq_in() and parse_pipeline_pktq_out() functions (please
> check latest code).
>
>
> > +static void
> > +parse_kni(struct app_params *app,
> > +                const char *section_name,
> > +                struct rte_cfgfile *cfg)
> > +{
> > +     struct app_kni_params *param;
> > +     struct rte_cfgfile_entry *entries;
> > +     int n_entries, i;
> > +     ssize_t param_idx;
> > +
> > +     n_entries = rte_cfgfile_section_num_entries(cfg, section_name);
> > +     PARSE_ERROR_SECTION_NO_ENTRIES((n_entries > 0),
> > section_name);
> > +
> > +     entries = malloc(n_entries * sizeof(struct rte_cfgfile_entry));
> > +     PARSE_ERROR_MALLOC(entries != NULL);
> > +
> > +     rte_cfgfile_section_entries(cfg, section_name, entries, n_entries);
> > +
> > +     param_idx = APP_PARAM_ADD(app->kni_params, section_name);
> > +     PARSER_PARAM_ADD_CHECK(param_idx, app->kni_params,
> > section_name);
> > +
> > +     param = &app->kni_params[param_idx];
> > +
> > +     for (i = 0; i < n_entries; i++) {
> > +             struct rte_cfgfile_entry *ent = &entries[i];
> > +
> > +             if (strcmp(ent->name, "core") == 0) {
> > +                     int status = parse_pipeline_core(
> > +                             &param->socket_id, &param->core_id,
> > +                             &param->hyper_th_id, ent->value);
> > +
> > +                     PARSE_ERROR((status == 0), section_name,
> > +                             ent->name);
> > +                     continue;
> > +             }
> > +
> > +             if (strcmp(ent->name, "mempool") == 0) {
> > +                     int status = validate_name(ent->value,
> > +                             "MEMPOOL", 1);
> > +                     ssize_t idx;
> > +
> > +                     PARSE_ERROR((status == 0), section_name,
> > +                             ent->name);
> > +                     idx = APP_PARAM_ADD(app->mempool_params,
> > +                             ent->value);
> > +                     PARSER_PARAM_ADD_CHECK(idx,
> > +                             app->mempool_params,
> > +                             section_name);
> > +                     param->mempool_id = idx;
> > +                     continue;
> > +             }
> > +
> > +             if (strcmp(ent->name, "burst") == 0) {
> > +                     int status = parser_read_uint32(&param->burst,
> > +                             ent->value);
> > +
> > +                     PARSE_ERROR((status == 0), section_name,
> > +                             ent->name);
> > +                     continue;
> > +             }
>
> As discussed above, we should parse two different entries in KNI section:
> burst_read and burst_write.
>
>
> > +
> > +             /* unrecognized */
> > +             PARSE_ERROR_INVALID(0, section_name, ent->name);
> > +     }
> > +
> > +     param->parsed = 1;
> > +
> > +     free(entries);
> > +}
> > +
> >  typedef void (*config_section_load)(struct app_params *p,
> >       const char *section_name,
> >       struct rte_cfgfile *cfg);
> > @@ -2483,6 +2568,7 @@ static const struct config_section
> cfg_file_scheme[]
> > = {
> >       {"MSGQ-REQ-PIPELINE", 1, parse_msgq_req_pipeline},
> >       {"MSGQ-RSP-PIPELINE", 1, parse_msgq_rsp_pipeline},
> >       {"MSGQ", 1, parse_msgq},
> > +     {"KNI", 1, parse_kni},
> >  };
> >
> >  static void
> > @@ -2619,6 +2705,7 @@ app_config_parse(struct app_params *app, const
> > char *file_name)
> >       APP_PARAM_COUNT(app->sink_params, app->n_pktq_sink);
> >       APP_PARAM_COUNT(app->msgq_params, app->n_msgq);
> >       APP_PARAM_COUNT(app->pipeline_params, app->n_pipelines);
> > +     APP_PARAM_COUNT(app->kni_params, app->n_kni);
> >
> >  #ifdef RTE_PORT_PCAP
> >       for (i = 0; i < (int)app->n_pktq_source; i++) {
> > @@ -3025,6 +3112,9 @@ save_pipeline_params(struct app_params *app,
> > FILE *f)
> >                               case APP_PKTQ_IN_SOURCE:
> >                                       name = app->source_params[pp-
> > >id].name;
> >                                       break;
> > +                             case APP_PKTQ_IN_KNI:
> > +                                     name = app->kni_params[pp-
> > >id].name;
> > +                                     break;
> >                               default:
> >                                       APP_CHECK(0, "System error "
> >                                               "occurred while saving "
> > @@ -3059,6 +3149,9 @@ save_pipeline_params(struct app_params *app,
> > FILE *f)
> >                               case APP_PKTQ_OUT_SINK:
> >                                       name = app->sink_params[pp-
> > >id].name;
> >                                       break;
> > +                             case APP_PKTQ_OUT_KNI:
> > +                                     name = app->kni_params[pp-
> > >id].name;
> > +                                     break;
> >                               default:
> >                                       APP_CHECK(0, "System error "
> >                                               "occurred while saving "
> > @@ -3114,6 +3207,37 @@ save_pipeline_params(struct app_params *app,
> > FILE *f)
> >       }
> >  }
> >
> > +static void
> > +save_kni_params(struct app_params *app, FILE *f)
> > +{
> > +     struct app_kni_params *p;
> > +     size_t i, count;
> > +
> > +     count = RTE_DIM(app->kni_params);
> > +     for (i = 0; i < count; i++) {
> > +             p = &app->kni_params[i];
> > +             if (!APP_PARAM_VALID(p))
> > +                     continue;
> > +
> > +             /* section name */
> > +             fprintf(f, "[%s]\n", p->name);
> > +
> > +             /* core */
> > +             fprintf(f, "core = s%" PRIu32 "c%" PRIu32 "%s\n",
> > +                             p->socket_id,
> > +                             p->core_id,
> > +                             (p->hyper_th_id) ? "h" : "");
> > +
> > +             /* mempool */
> > +             fprintf(f, "%s = %" PRIu32 "\n", "mempool_id", p-
> > >mempool_id);
>
> The name of the entry is "mempool" instead of "mempool_id". The value of
> the entry is app->mempool_params[p->mempool_id].name (which is a string)
> instead of p->mempool_id (which is a number).
>
> > +
> > +             /* burst */
> > +             fprintf(f, "%s = %" PRIu32 "\n", "burst", p->burst);
> > +
>
> We should save both p->burst_read and p->burst_write.
>
>
> > +             fputc('\n', f);
> > +     }
> > +}
> > +
> >  void
> >  app_config_save(struct app_params *app, const char *file_name)
> >  {
> > @@ -3144,6 +3268,7 @@ app_config_save(struct app_params *app, const
> > char *file_name)
> >       save_source_params(app, file);
> >       save_sink_params(app, file);
> >       save_msgq_params(app, file);
> > +     save_kni_params(app, file);
> >
> >       fclose(file);
> >       free(name);
> > @@ -3206,6 +3331,11 @@ app_config_init(struct app_params *app)
> >                       &default_pipeline_params,
> >                       sizeof(default_pipeline_params));
> >
> > +     for (i = 0; i < RTE_DIM(app->kni_params); i++)
> > +             memcpy(&app->kni_params[i],
> > +                     &default_kni_params,
> > +                     sizeof(default_kni_params));
> > +
> >       return 0;
> >  }
> >
> > diff --git a/examples/ip_pipeline/init.c b/examples/ip_pipeline/init.c
> > index 02351f6..8ff9118 100644
> > --- a/examples/ip_pipeline/init.c
> > +++ b/examples/ip_pipeline/init.c
>
> This is run-time code, let's have all the KNI code in init.c file enabled
> only when KNI library is part of the build:
> #ifdef RTE_LIBRTE_KNI
> ...
> #endif /* RTE_LIBRTE_KNI */
>
> > @@ -89,6 +89,24 @@ app_init_core_mask(struct app_params *app)
> >               mask |= 1LLU << lcore_id;
> >       }
> >
> > +     for (i = 0; i < app->n_kni; i++) {
> > +             struct app_kni_params *p = &app->kni_params[i];
> > +             int lcore_id;
> > +
> > +             lcore_id = cpu_core_map_get_lcore_id(app->core_map,
> > +                     p->socket_id,
> > +                     p->core_id,
> > +                     p->hyper_th_id);
> > +
> > +             if (lcore_id < 0)
> > +                     rte_panic("Cannot create CPU core mask\n");
> > +
> > +             if (mask & 1LLU << lcore_id)
>
> Please use parenthesis for improved readability: if (mask & (1LLU <<
> lcore_id)).
>
> > +                     rte_panic("KNI interface must use a dedicated
> > lcore\n");
>
> The bigger questions are:
> - Why do we need to dedicate separate CPU core(s) for KNI interface(s)?
> Isn't KNI code running in kernel space, why do we need to dedicate separate
> user space core for it and why do we need to add this core to the
> user-space application core mask?
> - Even if we need to add this to the core mask (maybe I am missing
> something here ...), why do we need to dedicate this core entirely to KNI?
> Can't we have KNI (kernel) code sharing this core with user-space
> application code (e.g. some pipeline instances?)
>
>
>
> First of all, it is a bug to add KNI kernel core to the user space core
> mask. What I want is just to check if the KNI kernel thread has a dedicated
> core.
>
> The reason I prefer to allocate a dedicated core to KNI kernel thread is
> that my application is latency sensitive. I worry the context switch and
> cache miss will cause the latency increasing if the KNI kernel thread and
> application thread share one core.
>
> Anyway, I think I should remove the hard coded check because it will be
> more generic. Users who has the similar usage like mine can achieve so
> through configuration file.
>
>
>
> > +
> > +             mask |= 1LLU << lcore_id;
> > +     }
> > +
> >       app->core_mask = mask;
> >       APP_LOG(app, HIGH, "CPU core mask = 0x%016" PRIx64, app-
> > >core_mask);
> >  }
> > @@ -1236,6 +1254,11 @@ static void app_pipeline_params_get(struct
> > app_params *app,
> >                                       n_bytes_per_pkt;
> >                       }
> >                       break;
> > +             case APP_PKTQ_IN_KNI:
> > +                     out->type = PIPELINE_PORT_IN_KNI;
> > +                     out->params.kni.kni = app->kni[in->id];
> > +                     out->burst_size = app->kni_params[in->id].burst;
> > +                     break;
> >               default:
> >                       break;
> >               }
> > @@ -1374,6 +1397,12 @@ static void app_pipeline_params_get(struct
> > app_params *app,
> >                               out->params.sink.max_n_pkts = 0;
> >                       }
> >                       break;
> > +             case APP_PKTQ_OUT_KNI:
> > +                     out->type = PIPELINE_PORT_OUT_KNI;
> > +                     out->params.kni.kni = app->kni[in->id];
> > +                     out->params.kni.tx_burst_sz =
> > +                                     app->kni_params[in->id].burst;
> > +                     break;
> >               default:
> >                       break;
> >               }
> > @@ -1397,6 +1426,55 @@ static void app_pipeline_params_get(struct
> > app_params *app,
> >  }
> >
> >  static void
> > +app_init_kni(struct app_params *app) {
> > +     uint32_t i;
> > +     struct rte_kni_conf conf;
> > +
>
> Avoid calling rte_kni_init() when there is no KNI device:
>
> if (app->n_kni == 0)
>         return;
>
> > +     rte_kni_init((unsigned int)app->n_kni);
> > +
> > +     for (i = 0; i < app->n_kni; i++) {
> > +             struct app_kni_params *p_kni = &app->kni_params[i];
> > +             uint32_t port_id;
>
> Please rename port_id with link_id, as this index is really the x from
> LINKx objects.
>
> > +             struct app_mempool_params *mempool_params;
> > +             struct rte_mempool *mempool;
> > +
> > +             if (sscanf(p_kni->name, "KNI%" PRIu32, &port_id) != 1)
> > +                     rte_panic("%s's port id is invalid\n",
> p_kni->name);
>
> Same comment as above: we do not need to check the x in KNIx, as LINKx is
> (should be, after you adjust the config_parse.c code) added automatically
> for KNIx.
>
> > +
> > +             mempool_params = &app->mempool_params[p_kni-
> > >mempool_id];
> > +             mempool = app->mempool[p_kni->mempool_id];
> > +
> > +             memset(&conf, 0, sizeof(conf));
> > +             snprintf(conf.name, RTE_KNI_NAMESIZE,
> > +                              "vEth%u", port_id);
> > +             conf.core_id = p_kni->core_id;
>
> The way the conf.core_id is set here is wrong, right?
>
>
> conf.core_id = cpu_core_map_get_lcore_id(app->core_map,
>         p->socket_id,
>         p->core_id,
>         p->hyper_th_id);
>
> > +             conf.force_bind = 1;
> > +
> > +             conf.group_id = (uint16_t) port_id;
> > +             conf.mbuf_size = mempool_params->buffer_size;
> > +
> > +             struct rte_kni_ops ops;
> > +             struct rte_eth_dev_info dev_info;
> > +
>
> Please move these definitions at the top of the for loop block rather than
> having them in the middle of the for loop block.
>
> > +             memset(&dev_info, 0, sizeof(dev_info));
> > +             rte_eth_dev_info_get(app->link_params[port_id].pmd_id,
> > +                                                      &dev_info);
> > +             conf.addr = dev_info.pci_dev->addr;
> > +             conf.id = dev_info.pci_dev->id;
> > +
> > +             memset(&ops, 0, sizeof(ops));
> > +             ops.port_id = app->link_params[port_id].pmd_id;
> > +             ops.change_mtu = kni_change_mtu;
> > +             ops.config_network_if = kni_config_network_interface;
> > +
> > +             app->kni[i] = rte_kni_alloc(mempool,
> > +                     &conf, &ops);
> > +             if (!app->kni[i])
> > +                     rte_panic("Fail to create kni for port: %d\n",
> port_id);
>
> rte_panic("Failed to create %s", p->name);
>
> This should print e.g. "Failed to create KNI5", which users should know it
> is the KNI associated with LINK5.
>
> > +     }
> > +}
> > +
> > +static void
> >  app_init_pipelines(struct app_params *app)
> >  {
> >       uint32_t p_id;
> > @@ -1531,6 +1609,7 @@ int app_init(struct app_params *app)
> >       app_init_swq(app);
> >       app_init_tm(app);
> >       app_init_msgq(app);
> > +     app_init_kni(app);
> >
> >       app_pipeline_common_cmd_push(app);
> >       app_pipeline_thread_cmd_push(app);
> > diff --git a/examples/ip_pipeline/kni/kni.c
> b/examples/ip_pipeline/kni/kni.c
> > new file mode 100644
> > index 0000000..c58e146
>
> Please move the two functions from kni/kni.c to init.c (just before
> app_init_kni() function) and make them static functions, then remove
> kni/kni.h and kni/kni.c files.
>
>
> > --- /dev/null
> > +++ b/examples/ip_pipeline/kni/kni.c
> > @@ -0,0 +1,80 @@
> > +#include <string.h>
> > +
> > +#include <rte_common.h>
> > +#include <rte_malloc.h>
> > +#include <rte_table_array.h>
> > +#include <rte_kni.h>
> > +#include <rte_ethdev.h>
> > +
> > +#include "rte_port_kni.h"
> > +#include "kni.h"
> > +
> > +int
> > +kni_config_network_interface(uint8_t port_id, uint8_t if_up) {
> > +     int ret = 0;
> > +
> > +     if (port_id >= rte_eth_dev_count() || port_id >=
> > RTE_MAX_ETHPORTS) {
> > +             RTE_LOG(ERR, PORT, "%s: Invalid port id %d\n",
> > +                             __func__, port_id);
> > +             return -EINVAL;
> > +     }
> > +
> > +     RTE_LOG(INFO, PORT, "%s: Configure network interface of %d
> > %s\n",
> > +                     __func__, port_id, if_up ? "up" : "down");
> > +
> > +     if (if_up != 0) { /* Configure network interface up */
> > +             rte_eth_dev_stop(port_id);
>
> Why do we need to stop the device first before we start it?
>
> > +             ret = rte_eth_dev_start(port_id);
> > +     } else /* Configure network interface down */
> > +             rte_eth_dev_stop(port_id);
>
> Do we need to call rte_eth_dev_start/stop() or do we need to call
> rte_eth_dev_up/down()?
>
> To be  honest, I made reference to the the KNI sample application.
>
> Since there is very little docs tell the difference between device up call
> and device start call, I am not sure which one is better here.
>
> Any help will be appreciate. :-)
>
>
>
> > +
> > +     if (ret < 0)
> > +             RTE_LOG(ERR, PORT, "%s: Failed to start port %d\n",
> > +                             __func__, port_id);
> > +
>
> This is a callback function, I think we should completely remove any
> RTE_LOG calls from it, as link can go up and down quite frequently.
>
>
> > +     return ret;
> > +}
> > +
> > +int
> > +kni_change_mtu(uint8_t port_id, unsigned new_mtu) {
> > +     int ret;
> > +
> > +     if (port_id >= rte_eth_dev_count()) {
> > +             RTE_LOG(ERR, PORT, "%s: Invalid port id %d\n",
> > +                             __func__, port_id);
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (new_mtu > ETHER_MAX_LEN) {
> > +             RTE_LOG(ERR, PORT,
> > +                             "%s: Fail to reconfigure port %d, the new
> > MTU is too big\n",
> > +                             __func__, port_id);
> > +             return -EINVAL;
> > +     }
> > +
> > +     RTE_LOG(INFO, PORT, "%s: Change MTU of port %d to %u\n",
> > +                     __func__, port_id,
> > +                     new_mtu);
> > +
> > +     /* Stop specific port */
> > +     rte_eth_dev_stop(port_id);
> > +
> > +     /* Set new MTU */
> > +     ret = rte_eth_dev_set_mtu(port_id, new_mtu);
> > +     if (ret < 0) {
> > +             RTE_LOG(ERR, PORT, "%s: Fail to reconfigure port %d\n",
> > +                             __func__, port_id);
> > +             return ret;
> > +     }
> > +
> > +     /* Restart specific port */
> > +     ret = rte_eth_dev_start(port_id);
> > +     if (ret < 0) {
> > +             RTE_LOG(ERR, PORT, "%s: Fail to restart port %d\n",
> > +                             __func__, port_id);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
>
> This is a callback function, I think we should completely remove all the
> RTE_LOG calls from it.
>
> > +
> > diff --git a/examples/ip_pipeline/kni/kni.h
> b/examples/ip_pipeline/kni/kni.h
> > new file mode 100644
> > index 0000000..04c8429
> > --- /dev/null
> > +++ b/examples/ip_pipeline/kni/kni.h
> > @@ -0,0 +1,16 @@
> > +#ifndef __INCLUDE_KNI_H__
> > +#define __INCLUDE_KNI_H__
> > +
> > +#include <rte_common.h>
> > +
> > +/* Total octets in ethernet header */
> > +#define KNI_ENET_HEADER_SIZE    14
>
> Are we actually using this macro? I would like to remove it if not needed.
>
> > +
> > +/* Total octets in the FCS */
> > +#define KNI_ENET_FCS_SIZE       4
>
> Are we actually using this macro? I would like to remove it if not needed.
>
>
> > +
> > +int kni_config_network_interface(uint8_t port_id, uint8_t if_up);
> > +
> > +int kni_change_mtu(uint8_t port_id, unsigned new_mtu);
> > +
> > +#endif
> > diff --git a/examples/ip_pipeline/pipeline_be.h
> > b/examples/ip_pipeline/pipeline_be.h
> > index f4ff262..23f0438 100644
> > --- a/examples/ip_pipeline/pipeline_be.h
> > +++ b/examples/ip_pipeline/pipeline_be.h
> > @@ -40,6 +40,7 @@
> >  #include <rte_port_ras.h>
> >  #include <rte_port_sched.h>
> >  #include <rte_port_source_sink.h>
> > +#include <rte_port_kni.h>
> >  #include <rte_pipeline.h>
> >
> >  enum pipeline_port_in_type {
> > @@ -50,6 +51,7 @@ enum pipeline_port_in_type {
> >       PIPELINE_PORT_IN_RING_READER_IPV6_FRAG,
> >       PIPELINE_PORT_IN_SCHED_READER,
> >       PIPELINE_PORT_IN_SOURCE,
> > +     PIPELINE_PORT_IN_KNI,
> >  };
> >
> >  struct pipeline_port_in_params {
> > @@ -62,6 +64,7 @@ struct pipeline_port_in_params {
> >               struct rte_port_ring_reader_ipv6_frag_params
> > ring_ipv6_frag;
> >               struct rte_port_sched_reader_params sched;
> >               struct rte_port_source_params source;
> > +             struct rte_port_kni_reader_params kni;
> >       } params;
> >       uint32_t burst_size;
> >  };
> > @@ -84,6 +87,8 @@ pipeline_port_in_params_convert(struct
> > pipeline_port_in_params  *p)
> >               return (void *) &p->params.sched;
> >       case PIPELINE_PORT_IN_SOURCE:
> >               return (void *) &p->params.source;
> > +     case PIPELINE_PORT_IN_KNI:
> > +             return (void *) &p->params.kni;
> >       default:
> >               return NULL;
> >       }
> > @@ -107,6 +112,8 @@ pipeline_port_in_params_get_ops(struct
> > pipeline_port_in_params  *p)
> >               return &rte_port_sched_reader_ops;
> >       case PIPELINE_PORT_IN_SOURCE:
> >               return &rte_port_source_ops;
> > +     case PIPELINE_PORT_IN_KNI:
> > +             return &rte_port_kni_reader_ops;
> >       default:
> >               return NULL;
> >       }
> > @@ -123,6 +130,7 @@ enum pipeline_port_out_type {
> >       PIPELINE_PORT_OUT_RING_WRITER_IPV6_RAS,
> >       PIPELINE_PORT_OUT_SCHED_WRITER,
> >       PIPELINE_PORT_OUT_SINK,
> > +     PIPELINE_PORT_OUT_KNI,
> >  };
> >
> >  struct pipeline_port_out_params {
> > @@ -138,6 +146,7 @@ struct pipeline_port_out_params {
> >               struct rte_port_ring_writer_ipv6_ras_params ring_ipv6_ras;
> >               struct rte_port_sched_writer_params sched;
> >               struct rte_port_sink_params sink;
> > +             struct rte_port_kni_writer_params kni;
> >       } params;
> >  };
> >
> > @@ -165,6 +174,8 @@ pipeline_port_out_params_convert(struct
> > pipeline_port_out_params  *p)
> >               return (void *) &p->params.sched;
> >       case PIPELINE_PORT_OUT_SINK:
> >               return (void *) &p->params.sink;
> > +     case PIPELINE_PORT_OUT_KNI:
> > +             return (void *) &p->params.kni;
> >       default:
> >               return NULL;
> >       }
> > @@ -194,6 +205,8 @@ pipeline_port_out_params_get_ops(struct
> > pipeline_port_out_params  *p)
> >               return &rte_port_sched_writer_ops;
> >       case PIPELINE_PORT_OUT_SINK:
> >               return &rte_port_sink_ops;
> > +     case PIPELINE_PORT_OUT_KNI:
> > +             return &rte_port_kni_writer_ops;
> >       default:
> >               return NULL;
> >       }
> > diff --git a/examples/ip_pipeline/thread.c
> b/examples/ip_pipeline/thread.c
> > index a0f1f12..534864a 100644
> > --- a/examples/ip_pipeline/thread.c
> > +++ b/examples/ip_pipeline/thread.c
> > @@ -239,6 +239,15 @@ app_thread(void *arg)
> >       uint32_t core_id = rte_lcore_id(), i, j;
> >       struct app_thread_data *t = &app->thread_data[core_id];
> >
> > +     for (i = 0; i < app->n_kni; i++) {
> > +             if (core_id == (uint32_t)cpu_core_map_get_lcore_id(
> > +                     app->core_map,
> > +                     app->kni_params[i].socket_id,
> > +                     app->kni_params[i].core_id,
> > +                     app->kni_params[i].hyper_th_id))
> > +                     return 0;
> > +     }
> > +
>
> Same questions as above:
> - Why do we need to dedicate separate CPU core(s) for KNI interface(s)?
> Isn't KNI code running in kernel space, why do we need to dedicate separate
> user space core for it and why do we need to add this core to the
> user-space application core mask?
> - Even if we need to add this to the core mask (maybe I am missing
> something here ...), why do we need to dedicate this core entirely to KNI?
> Can't we have KNI (kernel) code sharing this core with user-space
> application code (e.g. some pipeline instances?)
>
> This is run-time code, let's have it enabled only when KNI library is part
> of the build:
>
> #ifdef RTE_LIBRTE_KNI
> ...
> #endif /* RTE_LIBRTE_KNI */
>
>
> >       for (i = 0; ; i++) {
> >               uint32_t n_regular = RTE_MIN(t->n_regular, RTE_DIM(t-
> > >regular));
> >               uint32_t n_custom = RTE_MIN(t->n_custom, RTE_DIM(t-
> > >custom));
> > diff --git a/lib/librte_port/Makefile b/lib/librte_port/Makefile
> > index d4de5af..f18253d 100644
> > --- a/lib/librte_port/Makefile
> > +++ b/lib/librte_port/Makefile
> > @@ -57,6 +57,9 @@ SRCS-$(CONFIG_RTE_LIBRTE_PORT) += rte_port_ras.c
> >  endif
> >  SRCS-$(CONFIG_RTE_LIBRTE_PORT) += rte_port_sched.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_PORT) += rte_port_source_sink.c
> > +ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
> > +SRCS-$(CONFIG_RTE_LIBRTE_PORT) += rte_port_kni.c
> > +endif
> >
> >  # install includes
> >  SYMLINK-$(CONFIG_RTE_LIBRTE_PORT)-include += rte_port.h
> > @@ -68,6 +71,9 @@ SYMLINK-$(CONFIG_RTE_LIBRTE_PORT)-include +=
> > rte_port_ras.h
> >  endif
> >  SYMLINK-$(CONFIG_RTE_LIBRTE_PORT)-include += rte_port_sched.h
> >  SYMLINK-$(CONFIG_RTE_LIBRTE_PORT)-include += rte_port_source_sink.h
> > +ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
> > +SYMLINK-$(CONFIG_RTE_LIBRTE_PORT)-include += rte_port_kni.h
> > +endif
> >
> >  # this lib depends upon:
> >  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) := lib/librte_eal
> > @@ -75,5 +81,6 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) +=
> > lib/librte_mbuf
> >  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_mempool
> >  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ether
> >  DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_ip_frag
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) += lib/librte_kni
> >
> >  include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/lib/librte_port/rte_port_kni.c
> b/lib/librte_port/rte_port_kni.c
> > new file mode 100644
> > index 0000000..8c5e404
> > --- /dev/null
> > +++ b/lib/librte_port/rte_port_kni.c
> > @@ -0,0 +1,316 @@
> > +/*-
> > + *   BSD LICENSE
> > + *
> > + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
>
> Please fix the year as 2016.
>
>
> > + *   All rights reserved.
> > + *
> > + *   Redistribution and use in source and binary forms, with or without
> > + *   modification, are permitted provided that the following conditions
> > + *   are met:
> > + *
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above
> copyright
> > + *       notice, this list of conditions and the following disclaimer in
> > + *       the documentation and/or other materials provided with the
> > + *       distribution.
> > + *     * Neither the name of Intel Corporation nor the names of its
> > + *       contributors may be used to endorse or promote products derived
> > + *       from this software without specific prior written permission.
> > + *
> > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> > CONTRIBUTORS
> > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
> > NOT
> > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> > FITNESS FOR
> > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> > COPYRIGHT
> > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> > INCIDENTAL,
> > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> > NOT
> > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
> > OF USE,
> > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
> > AND ON ANY
> > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> > TORT
> > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> > THE USE
> > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> > DAMAGE.
> > + */
> > +#include <string.h>
> > +
> > +#include <rte_common.h>
> > +#include <rte_malloc.h>
> > +#include <rte_kni.h>
> > +
> > +#include "rte_port_kni.h"
> > +
> > +/*
> > + * Port KNI Reader
> > + */
> > +#ifdef RTE_PORT_STATS_COLLECT
> > +
> > +#define RTE_PORT_KNI_READER_STATS_PKTS_IN_ADD(port, val) \
> > +     {port->stats.n_pkts_in += val}
> > +#define RTE_PORT_KNI_READER_STATS_PKTS_DROP_ADD(port, val) \
> > +     {port->stats.n_pkts_drop += val}
>
> This actually results in compiler error when built with
> RTE_PORT_STATS_COLLECT = ON.
>
> Please add semicolon and remove curly braces (as in e.g. rte_port_ring.c):
> #define RTE_PORT_KNI_READER_STATS_PKTS_IN_ADD(port, val) \
>         port->stats.n_pkts_in += val;
> #define RTE_PORT_KNI_READER_STATS_PKTS_DROP_ADD(port, val) \
>         port->stats.n_pkts_drop += val;
>
>
> > +#else
> > +
> > +#define RTE_PORT_KNI_READER_STATS_PKTS_IN_ADD(port, val)
> > +#define RTE_PORT_KNI_READER_STATS_PKTS_DROP_ADD(port, val)
> > +
> > +#endif
> > +
> > +struct rte_port_kni_reader {
> > +     struct rte_port_in_stats stats;
> > +
> > +     struct rte_kni *kni;
> > +};
> > +
> > +static void *
> > +rte_port_kni_reader_create(void *params, int socket_id) {
> > +     struct rte_port_kni_reader_params *conf =
> > +                     (struct rte_port_kni_reader_params *) params;
> > +     struct rte_port_kni_reader *port;
> > +
> > +     /* Check input parameters */
> > +     if (conf == NULL) {
> > +             RTE_LOG(ERR, PORT, "%s: params is NULL\n", __func__);
> > +             return NULL;
> > +     }
> > +
> > +     /* Memory allocation */
> > +     port = rte_zmalloc_socket("PORT", sizeof(*port),
> > +             RTE_CACHE_LINE_SIZE, socket_id);
> > +     if (port == NULL) {
> > +             RTE_LOG(ERR, PORT, "%s: Failed to allocate port\n",
> > __func__);
> > +             return NULL;
> > +     }
> > +
> > +     /* Initialization */
> > +     port->kni = conf->kni;
> > +
> > +     return port;
> > +}
> > +
> > +static int
> > +rte_port_kni_reader_rx(void *port, struct rte_mbuf **pkts, uint32_t
> > n_pkts) {
> > +     struct rte_port_kni_reader *p =
> > +                     (struct rte_port_kni_reader *) port;
> > +     uint16_t rx_pkt_cnt;
> > +
> > +     rx_pkt_cnt = rte_kni_rx_burst(p->kni, pkts, n_pkts);
> > +     RTE_PORT_KNI_READER_STATS_PKTS_IN_ADD(p, rx_pkt_cnt);
> > +     return rx_pkt_cnt;
> > +}
> > +
> > +static int
> > +rte_port_kni_reader_free(void *port) {
> > +     if (port == NULL) {
> > +             RTE_LOG(ERR, PORT, "%s: port is NULL\n", __func__);
> > +             return -EINVAL;
> > +     }
> > +
> > +     rte_free(port);
> > +
> > +     return 0;
> > +}
> > +
> > +static int rte_port_kni_reader_stats_read(void *port,
> > +     struct rte_port_in_stats *stats, int clear)
> > +{
> > +     struct rte_port_kni_reader *p =
> > +                     (struct rte_port_kni_reader *) port;
> > +
> > +     if (stats != NULL)
> > +             memcpy(stats, &p->stats, sizeof(p->stats));
> > +
> > +     if (clear)
> > +             memset(&p->stats, 0, sizeof(p->stats));
> > +
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Port KNI Writer
> > + */
> > +#ifdef RTE_PORT_STATS_COLLECT
> > +
> > +#define RTE_PORT_KNI_WRITER_STATS_PKTS_IN_ADD(port, val) \
> > +     {port->stats.n_pkts_in += val}
> > +#define RTE_PORT_KNI_WRITER_STATS_PKTS_DROP_ADD(port, val) \
> > +     {port->stats.n_pkts_drop += val}
> > +
>
> This actually results in compiler error when built with
> RTE_PORT_STATS_COLLECT = ON.
>
> Please add semicolon and remove curly braces (as in e.g. rte_port_ring.c).
>
> Actually I implements the macro similar
> to RTE_PORT_RING_READER_STATS_PKTS_IN_ADD first. But the
> scripts/checkpatches.sh fails: ERROR:COMPLEX_MACRO: Macros with complex
> values should be enclosed in parentheses
>
> I'm not share either I have done something wrong or
> the checkpatches script need an update.
>
>
>
> > +#else
> > +
> > +#define RTE_PORT_KNI_WRITER_STATS_PKTS_IN_ADD(port, val)
> > +#define RTE_PORT_KNI_WRITER_STATS_PKTS_DROP_ADD(port, val)
> > +
> > +#endif
> > +
> > +struct rte_port_kni_writer {
> > +     struct rte_port_out_stats stats;
> > +
> > +     struct rte_mbuf *tx_buf[2 * RTE_PORT_IN_BURST_SIZE_MAX];
> > +     uint32_t tx_burst_sz;
> > +     uint16_t tx_buf_count;
>
> Can we actually make tx_buf_count to be uint32_t rather than uint16_t? I
> see some computation between tx_buf_count and some other variable of type
> uint32_t later in the code ...
>
>
> > +     uint64_t bsz_mask;
> > +     struct rte_kni *kni;
> > +};
> > +
> > +static void *
> > +rte_port_kni_writer_create(void *params, int socket_id) {
> > +     struct rte_port_kni_writer_params *conf =
> > +                     (struct rte_port_kni_writer_params *) params;
> > +     struct rte_port_kni_writer *port;
> > +
> > +     /* Check input parameters */
> > +     if ((conf == NULL) ||
> > +             (conf->tx_burst_sz == 0) ||
> > +             (conf->tx_burst_sz > RTE_PORT_IN_BURST_SIZE_MAX) ||
> > +             (!rte_is_power_of_2(conf->tx_burst_sz))) {
> > +             RTE_LOG(ERR, PORT, "%s: Invalid input parameters\n",
> > __func__);
> > +             return NULL;
> > +     }
> > +
> > +     /* Memory allocation */
> > +     port = rte_zmalloc_socket("PORT", sizeof(*port),
> > +             RTE_CACHE_LINE_SIZE, socket_id);
> > +     if (port == NULL) {
> > +             RTE_LOG(ERR, PORT, "%s: Failed to allocate port\n",
> > __func__);
> > +             return NULL;
> > +     }
> > +
> > +     /* Initialization */
> > +     port->kni = conf->kni;
> > +     port->tx_burst_sz = conf->tx_burst_sz;
> > +     port->tx_buf_count = 0;
> > +     port->bsz_mask = 1LLU << (conf->tx_burst_sz - 1);
> > +
> > +     return port;
> > +}
> > +
> > +static inline void
> > +send_burst(struct rte_port_kni_writer *p) {
> > +     uint32_t nb_tx;
> > +
> > +     nb_tx = rte_kni_tx_burst(p->kni, p->tx_buf, p->tx_buf_count);
> > +     rte_kni_handle_request(p->kni);
> > +
> > +     RTE_PORT_KNI_WRITER_STATS_PKTS_DROP_ADD(p, p-
> > >tx_buf_count - nb_tx);
> > +     for (; nb_tx < p->tx_buf_count; nb_tx++)
> > +             rte_pktmbuf_free(p->tx_buf[nb_tx]);
> > +
> > +     p->tx_buf_count = 0;
> > +}
> > +
> > +static int
> > +rte_port_kni_writer_tx(void *port, struct rte_mbuf *pkt) {
> > +     struct rte_port_kni_writer *p =
> > +                     (struct rte_port_kni_writer *) port;
> > +
> > +     p->tx_buf[p->tx_buf_count++] = pkt;
> > +     RTE_PORT_KNI_WRITER_STATS_PKTS_IN_ADD(p, 1);
> > +     if (p->tx_buf_count >= p->tx_burst_sz)
> > +             send_burst(p);
> > +
> > +     return 0;
> > +}
> > +
> > +static int
> > +rte_port_kni_writer_tx_bulk(void *port,
> > +                                                     struct rte_mbuf
> > **pkts,
> > +                                                     uint64_t
> pkts_mask) {
> > +     struct rte_port_kni_writer *p =
> > +                     (struct rte_port_kni_writer *) port;
> > +     uint64_t bsz_mask = p->bsz_mask;
> > +     uint32_t tx_buf_count = p->tx_buf_count;
> > +     uint64_t expr = (pkts_mask & (pkts_mask + 1)) |
> > +                                     ((pkts_mask & bsz_mask) ^
> > bsz_mask);
> > +
> > +     if (expr == 0) {
> > +             uint64_t n_pkts = __builtin_popcountll(pkts_mask);
> > +             uint32_t n_pkts_ok;
> > +
> > +             if (tx_buf_count)
> > +                     send_burst(p);
> > +
> > +             RTE_PORT_KNI_WRITER_STATS_PKTS_IN_ADD(p, n_pkts);
> > +             n_pkts_ok = rte_kni_tx_burst(p->kni, pkts, n_pkts);
> > +
> > +             RTE_PORT_KNI_WRITER_STATS_PKTS_DROP_ADD(p, n_pkts
> > - n_pkts_ok);
> > +             for (; n_pkts_ok < n_pkts; n_pkts_ok++) {
> > +                     struct rte_mbuf *pkt = pkts[n_pkts_ok];
> > +
> > +                     rte_pktmbuf_free(pkt);
> > +             }
> > +     } else {
> > +             for (; pkts_mask;) {
> > +                     uint32_t pkt_index = __builtin_ctzll(pkts_mask);
> > +                     uint64_t pkt_mask = 1LLU << pkt_index;
> > +                     struct rte_mbuf *pkt = pkts[pkt_index];
> > +
> > +                     p->tx_buf[tx_buf_count++] = pkt;
> > +                     RTE_PORT_KNI_WRITER_STATS_PKTS_IN_ADD(p, 1);
> > +                     pkts_mask &= ~pkt_mask;
> > +             }
> > +
> > +             p->tx_buf_count = tx_buf_count;
> > +             if (tx_buf_count >= p->tx_burst_sz)
> > +                     send_burst(p);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int
> > +rte_port_kni_writer_flush(void *port) {
> > +     struct rte_port_kni_writer *p =
> > +                     (struct rte_port_kni_writer *) port;
> > +
> > +     if (p->tx_buf_count > 0)
> > +             send_burst(p);
> > +
> > +     return 0;
> > +}
> > +
> > +static int
> > +rte_port_kni_writer_free(void *port) {
> > +     if (port == NULL) {
> > +             RTE_LOG(ERR, PORT, "%s: Port is NULL\n", __func__);
> > +             return -EINVAL;
> > +     }
> > +
> > +     rte_port_kni_writer_flush(port);
> > +     rte_free(port);
> > +
> > +     return 0;
> > +}
> > +
> > +static int rte_port_kni_writer_stats_read(void *port,
> > +     struct rte_port_out_stats *stats, int clear)
> > +{
> > +     struct rte_port_kni_writer *p =
> > +                     (struct rte_port_kni_writer *) port;
> > +
> > +     if (stats != NULL)
> > +             memcpy(stats, &p->stats, sizeof(p->stats));
> > +
> > +     if (clear)
> > +             memset(&p->stats, 0, sizeof(p->stats));
> > +
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Summary of port operations
> > + */
> > +struct rte_port_in_ops rte_port_kni_reader_ops = {
> > +     .f_create = rte_port_kni_reader_create,
> > +     .f_free = rte_port_kni_reader_free,
> > +     .f_rx = rte_port_kni_reader_rx,
> > +     .f_stats = rte_port_kni_reader_stats_read,
> > +};
> > +
> > +struct rte_port_out_ops rte_port_kni_writer_ops = {
> > +     .f_create = rte_port_kni_writer_create,
> > +     .f_free = rte_port_kni_writer_free,
> > +     .f_tx = rte_port_kni_writer_tx,
> > +     .f_tx_bulk = rte_port_kni_writer_tx_bulk,
> > +     .f_flush = rte_port_kni_writer_flush,
> > +     .f_stats = rte_port_kni_writer_stats_read,
> > +};
>
> Do we need a KNI writer no-drop version as well? Would it be useful?
>
> > diff --git a/lib/librte_port/rte_port_kni.h
> b/lib/librte_port/rte_port_kni.h
> > new file mode 100644
> > index 0000000..7623798
> > --- /dev/null
> > +++ b/lib/librte_port/rte_port_kni.h
> > @@ -0,0 +1,81 @@
> > +/*-
> > + *   BSD LICENSE
> > + *
> > + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
>
> Please fix the year to 2016.
>
>
> > + *   All rights reserved.
> > + *
> > + *   Redistribution and use in source and binary forms, with or without
> > + *   modification, are permitted provided that the following conditions
> > + *   are met:
> > + *
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above
> copyright
> > + *       notice, this list of conditions and the following disclaimer in
> > + *       the documentation and/or other materials provided with the
> > + *       distribution.
> > + *     * Neither the name of Intel Corporation nor the names of its
> > + *       contributors may be used to endorse or promote products derived
> > + *       from this software without specific prior written permission.
> > + *
> > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> > CONTRIBUTORS
> > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
> > NOT
> > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> > FITNESS FOR
> > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> > COPYRIGHT
> > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> > INCIDENTAL,
> > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> > NOT
> > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
> > OF USE,
> > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
> > AND ON ANY
> > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> > TORT
> > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> > THE USE
> > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> > DAMAGE.
> > + */
> > +
> > +#ifndef __INCLUDE_RTE_PORT_KNI_H__
> > +#define __INCLUDE_RTE_PORT_KNI_H__
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/**
> > + * @file
> > + * RTE Port KNI Interface
> > + *
> > + * kni_reader: input port built on top of pre-initialized KNI interface
> > + * kni_writer: output port built on top of pre-initialized KNI interface
> > + *
> > + ***/
> > +
> > +#include <stdint.h>
> > +
> > +#include <rte_kni.h>
> > +
> > +#include "rte_port.h"
> > +
> > +/** kni_reader port parameters */
> > +struct rte_port_kni_reader_params {
> > +     /** KNI interface reference */
> > +     struct rte_kni *kni;
> > +};
> > +
> > +/** kni_reader port operations */
> > +extern struct rte_port_in_ops rte_port_kni_reader_ops;
> > +
> > +
> > +/** kni_writer port parameters */
> > +struct rte_port_kni_writer_params {
> > +     /** KNI interface reference */
> > +     struct rte_kni *kni;
> > +     /** Burst size to KNI interface. */
> > +     uint32_t tx_burst_sz;
> > +};
> > +
> > +/** kni_writer port operations */
> > +extern struct rte_port_out_ops rte_port_kni_writer_ops;
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif
> > --
> > 2.7.4
>
> Thank you!
>
> Regards,
> Cristian
>
>
>
>
>
> B.R.
>
>
>
> Ethan
>
>
>