From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f67.google.com (mail-lf0-f67.google.com [209.85.215.67]) by dpdk.org (Postfix) with ESMTP id B86FCC63C for ; Thu, 16 Jun 2016 13:34:20 +0200 (CEST) Received: by mail-lf0-f67.google.com with SMTP id a2so5336314lfe.3 for ; Thu, 16 Jun 2016 04:34:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=1X09tY3jm6K9j8TJKonuuOduMU+oz38JVfhsawxAWdY=; b=eGhY5JqCSUKR5fVusv88td/sgnKITUoU/HIRr9ILyKF4fjAY/dzg9yOCeAcBBlvrvB Tu5btf3KjDPPhaO5vexbZ+r6B/Jl0wo9mDka7QNuO+OmOz6tC+cH4BV0KwY3VQ1ndTkL tKsH26K7PpYY4EKLoZPvLccLOuqk0jBD9WNYzxgPLMpAGKcE6+AFmYp6OwQSSYQeN6J7 4zrLmXL3p4WtvF6ephZ3oTmYsucEN+5V6f+s3yfO0gF11g4hvD7kAub299NTh+8vWs4U 0tdyLHJx/VgQqAy6kmup2d6klOLbh4qDp1YIuRZUgkTt0lBBK6gN+DA/OQq8iVf9P38a A8ZQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=1X09tY3jm6K9j8TJKonuuOduMU+oz38JVfhsawxAWdY=; b=JoUdHlCWNrDXwt2H/xGGsQdRLFs/ELLizIA8O60oHHO4PbbjHt3YkAwIYy1DDkIWTT Yuklx52rxzOMf05/PvecnGMhwV3qCYlHHrp87oYeg+sfeJQBYtqfQ7hhS+hrPoPS+leC MPx207hOC+KMZBKdsR4pKUTVXX5oxZ0CtE4L2C9ty8ogbwnYbiftwkDVU+fbFyI20It2 nC6XnBkKabQBBKisNo+BDlXcuuTO6xx1mV28ZqjC6Qts5fxwgs38EGqWaJKvTM3bY3a3 tEprnVed6QawNpszhaDUXzaAsY8S2IJkJUt/Znq56O+YBc7HDDFBSiJMSMkhklvQazCt aLfw== X-Gm-Message-State: ALyK8tLgBhpsbIyWOrtuTYcgR0ql44YhOYmcTB2zOYo2yZIM2CJi5KiRbC8pb03nsTj8DwcWr84DnbWzGXVDIg== X-Received: by 10.46.1.147 with SMTP id f19mr1103281lji.44.1466076859983; Thu, 16 Jun 2016 04:34:19 -0700 (PDT) MIME-Version: 1.0 Sender: zhuangweijie@gmail.com Received: by 10.114.182.239 with HTTP; Thu, 16 Jun 2016 04:34:18 -0700 (PDT) In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D8912647A03BCB@IRSMSX108.ger.corp.intel.com> References: <1464325653-6912-1-git-send-email-zhuangwj@gmail.com> <1464434757-10860-1-git-send-email-zhuangwj@gmail.com> <3EB4FA525960D640B5BDFFD6A3D8912647A00F57@IRSMSX108.ger.corp.intel.com> <3EB4FA525960D640B5BDFFD6A3D8912647A03BCB@IRSMSX108.ger.corp.intel.com> From: Ethan Date: Thu, 16 Jun 2016 19:34:18 +0800 X-Google-Sender-Auth: nwmJ14g15xC0LL8IkYE5-Qu9D8I Message-ID: To: "Dumitrescu, Cristian" Cc: "dev@dpdk.org" , "Singh, Jasvinder" , "Yigit, Ferruh" 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] port: add kni interface support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Jun 2016 11:34:21 -0000 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=E2=80=99ll wait for your patch later this week then. I recommen= d you add > any other changes that you might have on top of the latest code that I ju= st > send, as this will minimize your work, my work to further code reviews an= d > 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 > *Cc:* dev@dpdk.org; Singh, Jasvinder ; Yigit, > Ferruh > *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 dedicat= ed > 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 me= t > 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 (cor= es > 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 th= e > 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 a= ny > 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 spac= e > 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=E2=80=99s use the same consistent rule to create the stats= macros > for all the ports, i.e. follow the existing rule used for other ports. Yo= u > 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 > > Cc: dev@dpdk.org; WeiJie Zhuang > > 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 > > --- > > 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 =3D ip_pipeline > > > > +VPATH +=3D $(SRCDIR)/kni > > VPATH +=3D $(SRCDIR)/pipeline > > > > -INC +=3D $(wildcard *.h) $(wildcard pipeline/*.h) > > +INC +=3D $(wildcard *.h) $(wildcard pipeline/*.h) $(wildcard kni/*.h) > > > > # all source are stored in SRCS-y > > SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) :=3D main.c > > @@ -56,6 +57,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D init.c > > SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D thread.c > > SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D thread_fe.c > > SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D cpu_core_map.c > > +SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D kni.c > > > > SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D pipeline_common_be.c > > SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D pipeline_common_fe.c > > @@ -72,7 +74,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D > > pipeline_flow_actions.c > > SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D pipeline_routing_be.c > > SRCS-$(CONFIG_RTE_LIBRTE_PIPELINE) +=3D pipeline_routing.c > > > > -CFLAGS +=3D -I$(SRCDIR) -I$(SRCDIR)/pipeline > > +CFLAGS +=3D -I$(SRCDIR) -I$(SRCDIR)/pipeline -I$(SRCDIR)/kni > > CFLAGS +=3D -O3 > > CFLAGS +=3D $(WERROR_FLAGS) -Wno-error=3Dunused-function -Wno- > > error=3Dunused-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 > > > > #include > > +#include > > > > +#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 =3D kni - app->kni_params; > > + uint32_t n_pipelines =3D RTE_MIN(app->n_pipelines, > > + RTE_DIM(app->pipeline_params)); > > + uint32_t n_readers =3D 0, i; > > + > > + for (i =3D 0; i < n_pipelines; i++) { > > + struct app_pipeline_params *p =3D &app->pipeline_params[i= ]; > > + uint32_t n_pktq_in =3D RTE_MIN(p->n_pktq_in, RTE_DIM(p- > > >pktq_in)); > > + uint32_t j; > > + > > + for (j =3D 0; j < n_pktq_in; j++) { > > + struct app_pktq_in_params *pktq =3D &p->pktq_in[j= ]; > > + > > + if ((pktq->type =3D=3D APP_PKTQ_IN_KNI) && > > + (pktq->id =3D=3D 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 =3D 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 =3D kni - app->kni_params; > > + uint32_t n_pipelines =3D RTE_MIN(app->n_pipelines, > > + RTE_DIM(app->pipeline_params)); > > + uint32_t n_writers =3D 0, i; > > + > > + for (i =3D 0; i < n_pipelines; i++) { > > + struct app_pipeline_params *p =3D &app->pipeline_params[i= ]; > > + uint32_t n_pktq_out =3D RTE_MIN(p->n_pktq_out, > > + RTE_DIM(p->pktq_out)); > > + uint32_t j; > > + > > + for (j =3D 0; j < n_pktq_out; j++) { > > + struct app_pktq_out_params *pktq =3D &p- > > >pktq_out[j]; > > + > > + if ((pktq->type =3D=3D APP_PKTQ_OUT_KNI) && > > + (pktq->id =3D=3D 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 =3D 2 > > + > > +[PIPELINE0] > > +type =3D MASTER > > +core =3D 0 > > + > > +[PIPELINE1] > > +type =3D PASS-THROUGH > > +core =3D 1 > > +pktq_in =3D RXQ0.0 KNI0 > > +pktq_out =3D 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 =3D 0; i < app->n_kni; i++) { > > + struct app_kni_params *p =3D &app->kni_params[i]; > > + uint32_t n_readers =3D app_kni_get_readers(app, p); > > + uint32_t n_writers =3D app_kni_get_writers(app, p); > > + > > + APP_CHECK((n_readers !=3D 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 !=3D 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 automaticall= y > 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, d= ue > 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 he= re > in config_check.c or in init.c. > > > > + APP_CHECK(sscanf(p->name, "KNI%" PRIu32, &port_id) =3D=3D= 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 =3D { > > .n_args =3D 0, > > }; > > > > +struct app_kni_params default_kni_params =3D { > > + .parsed =3D 0, > > + .socket_id =3D 0, > > + .core_id =3D 0, > > + .hyper_th_id =3D 0, > > + .mempool_id =3D 0, > > + .burst =3D 32, > > .burst_read =3D 32, > .burst_write =3D 32, > > > > +}; > > + > > static const char app_usage[] =3D > > "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) =3D=3D 0) { > > type =3D APP_PKTQ_IN_SOURCE; > > id =3D APP_PARAM_ADD(app->source_params, name); > > + } else if (validate_name(name, "KNI", 1) =3D=3D 0) { > > + type =3D APP_PKTQ_IN_KNI; > > + id =3D 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) =3D=3D 0) { > > type =3D APP_PKTQ_OUT_SINK; > > id =3D APP_PARAM_ADD(app->sink_params, name); > > + } else if (validate_name(name, "KNI", 1) =3D=3D 0) { > > + type =3D APP_PKTQ_OUT_KNI; > > + id =3D 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 a= s > 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 =3D rte_cfgfile_section_num_entries(cfg, section_name); > > + PARSE_ERROR_SECTION_NO_ENTRIES((n_entries > 0), > > section_name); > > + > > + entries =3D malloc(n_entries * sizeof(struct rte_cfgfile_entry)); > > + PARSE_ERROR_MALLOC(entries !=3D NULL); > > + > > + rte_cfgfile_section_entries(cfg, section_name, entries, n_entries= ); > > + > > + param_idx =3D APP_PARAM_ADD(app->kni_params, section_name); > > + PARSER_PARAM_ADD_CHECK(param_idx, app->kni_params, > > section_name); > > + > > + param =3D &app->kni_params[param_idx]; > > + > > + for (i =3D 0; i < n_entries; i++) { > > + struct rte_cfgfile_entry *ent =3D &entries[i]; > > + > > + if (strcmp(ent->name, "core") =3D=3D 0) { > > + int status =3D parse_pipeline_core( > > + ¶m->socket_id, ¶m->core_id, > > + ¶m->hyper_th_id, ent->value); > > + > > + PARSE_ERROR((status =3D=3D 0), section_name, > > + ent->name); > > + continue; > > + } > > + > > + if (strcmp(ent->name, "mempool") =3D=3D 0) { > > + int status =3D validate_name(ent->value, > > + "MEMPOOL", 1); > > + ssize_t idx; > > + > > + PARSE_ERROR((status =3D=3D 0), section_name, > > + ent->name); > > + idx =3D APP_PARAM_ADD(app->mempool_params, > > + ent->value); > > + PARSER_PARAM_ADD_CHECK(idx, > > + app->mempool_params, > > + section_name); > > + param->mempool_id =3D idx; > > + continue; > > + } > > + > > + if (strcmp(ent->name, "burst") =3D=3D 0) { > > + int status =3D parser_read_uint32(¶m->burst, > > + ent->value); > > + > > + PARSE_ERROR((status =3D=3D 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 =3D 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[] > > =3D { > > {"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 =3D 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 =3D app->source_params[pp- > > >id].name; > > break; > > + case APP_PKTQ_IN_KNI: > > + name =3D 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 =3D app->sink_params[pp- > > >id].name; > > break; > > + case APP_PKTQ_OUT_KNI: > > + name =3D 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 =3D RTE_DIM(app->kni_params); > > + for (i =3D 0; i < count; i++) { > > + p =3D &app->kni_params[i]; > > + if (!APP_PARAM_VALID(p)) > > + continue; > > + > > + /* section name */ > > + fprintf(f, "[%s]\n", p->name); > > + > > + /* core */ > > + fprintf(f, "core =3D s%" PRIu32 "c%" PRIu32 "%s\n", > > + p->socket_id, > > + p->core_id, > > + (p->hyper_th_id) ? "h" : ""); > > + > > + /* mempool */ > > + fprintf(f, "%s =3D %" 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 =3D %" 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 =3D 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 |=3D 1LLU << lcore_id; > > } > > > > + for (i =3D 0; i < app->n_kni; i++) { > > + struct app_kni_params *p =3D &app->kni_params[i]; > > + int lcore_id; > > + > > + lcore_id =3D 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 separa= te > 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 dedicat= ed > 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 |=3D 1LLU << lcore_id; > > + } > > + > > app->core_mask =3D mask; > > APP_LOG(app, HIGH, "CPU core mask =3D 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 =3D PIPELINE_PORT_IN_KNI; > > + out->params.kni.kni =3D app->kni[in->id]; > > + out->burst_size =3D 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 =3D 0; > > } > > break; > > + case APP_PKTQ_OUT_KNI: > > + out->type =3D PIPELINE_PORT_OUT_KNI; > > + out->params.kni.kni =3D app->kni[in->id]; > > + out->params.kni.tx_burst_sz =3D > > + 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 =3D=3D 0) > return; > > > + rte_kni_init((unsigned int)app->n_kni); > > + > > + for (i =3D 0; i < app->n_kni; i++) { > > + struct app_kni_params *p_kni =3D &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) !=3D 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 =3D &app->mempool_params[p_kni- > > >mempool_id]; > > + mempool =3D app->mempool[p_kni->mempool_id]; > > + > > + memset(&conf, 0, sizeof(conf)); > > + snprintf(conf.name, RTE_KNI_NAMESIZE, > > + "vEth%u", port_id); > > + conf.core_id =3D p_kni->core_id; > > The way the conf.core_id is set here is wrong, right? > > > conf.core_id =3D cpu_core_map_get_lcore_id(app->core_map, > p->socket_id, > p->core_id, > p->hyper_th_id); > > > + conf.force_bind =3D 1; > > + > > + conf.group_id =3D (uint16_t) port_id; > > + conf.mbuf_size =3D 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 tha= n > 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 =3D dev_info.pci_dev->addr; > > + conf.id =3D dev_info.pci_dev->id; > > + > > + memset(&ops, 0, sizeof(ops)); > > + ops.port_id =3D app->link_params[port_id].pmd_id; > > + ops.change_mtu =3D kni_change_mtu; > > + ops.config_network_if =3D kni_config_network_interface; > > + > > + app->kni[i] =3D 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 i= t > 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 > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "rte_port_kni.h" > > +#include "kni.h" > > + > > +int > > +kni_config_network_interface(uint8_t port_id, uint8_t if_up) { > > + int ret =3D 0; > > + > > + if (port_id >=3D rte_eth_dev_count() || port_id >=3D > > 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 !=3D 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 =3D 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 cal= l > 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 >=3D 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 =3D 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 =3D 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 > > + > > +/* 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 > > #include > > #include > > +#include > > #include > > > > 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 =3D rte_lcore_id(), i, j; > > struct app_thread_data *t =3D &app->thread_data[core_id]; > > > > + for (i =3D 0; i < app->n_kni; i++) { > > + if (core_id =3D=3D (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 separa= te > 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 par= t > of the build: > > #ifdef RTE_LIBRTE_KNI > ... > #endif /* RTE_LIBRTE_KNI */ > > > > for (i =3D 0; ; i++) { > > uint32_t n_regular =3D RTE_MIN(t->n_regular, RTE_DIM(t- > > >regular)); > > uint32_t n_custom =3D 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) +=3D rte_port_ras.c > > endif > > SRCS-$(CONFIG_RTE_LIBRTE_PORT) +=3D rte_port_sched.c > > SRCS-$(CONFIG_RTE_LIBRTE_PORT) +=3D rte_port_source_sink.c > > +ifeq ($(CONFIG_RTE_LIBRTE_KNI),y) > > +SRCS-$(CONFIG_RTE_LIBRTE_PORT) +=3D rte_port_kni.c > > +endif > > > > # install includes > > SYMLINK-$(CONFIG_RTE_LIBRTE_PORT)-include +=3D rte_port.h > > @@ -68,6 +71,9 @@ SYMLINK-$(CONFIG_RTE_LIBRTE_PORT)-include +=3D > > rte_port_ras.h > > endif > > SYMLINK-$(CONFIG_RTE_LIBRTE_PORT)-include +=3D rte_port_sched.h > > SYMLINK-$(CONFIG_RTE_LIBRTE_PORT)-include +=3D rte_port_source_sink.h > > +ifeq ($(CONFIG_RTE_LIBRTE_KNI),y) > > +SYMLINK-$(CONFIG_RTE_LIBRTE_PORT)-include +=3D rte_port_kni.h > > +endif > > > > # this lib depends upon: > > DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) :=3D lib/librte_eal > > @@ -75,5 +81,6 @@ DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) +=3D > > lib/librte_mbuf > > DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) +=3D lib/librte_mempool > > DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) +=3D lib/librte_ether > > DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) +=3D lib/librte_ip_frag > > +DEPDIRS-$(CONFIG_RTE_LIBRTE_PORT) +=3D 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 withou= t > > + * modification, are permitted provided that the following condition= s > > + * are met: > > + * > > + * * Redistributions of source code must retain the above copyrigh= t > > + * 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 deriv= ed > > + * 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 > > + > > +#include > > +#include > > +#include > > + > > +#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 +=3D val} > > +#define RTE_PORT_KNI_READER_STATS_PKTS_DROP_ADD(port, val) \ > > + {port->stats.n_pkts_drop +=3D val} > > This actually results in compiler error when built with > RTE_PORT_STATS_COLLECT =3D 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 +=3D val; > #define RTE_PORT_KNI_READER_STATS_PKTS_DROP_ADD(port, val) \ > port->stats.n_pkts_drop +=3D 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 =3D > > + (struct rte_port_kni_reader_params *) params; > > + struct rte_port_kni_reader *port; > > + > > + /* Check input parameters */ > > + if (conf =3D=3D NULL) { > > + RTE_LOG(ERR, PORT, "%s: params is NULL\n", __func__); > > + return NULL; > > + } > > + > > + /* Memory allocation */ > > + port =3D rte_zmalloc_socket("PORT", sizeof(*port), > > + RTE_CACHE_LINE_SIZE, socket_id); > > + if (port =3D=3D NULL) { > > + RTE_LOG(ERR, PORT, "%s: Failed to allocate port\n", > > __func__); > > + return NULL; > > + } > > + > > + /* Initialization */ > > + port->kni =3D 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 =3D > > + (struct rte_port_kni_reader *) port; > > + uint16_t rx_pkt_cnt; > > + > > + rx_pkt_cnt =3D 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 =3D=3D 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 =3D > > + (struct rte_port_kni_reader *) port; > > + > > + if (stats !=3D 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 +=3D val} > > +#define RTE_PORT_KNI_WRITER_STATS_PKTS_DROP_ADD(port, val) \ > > + {port->stats.n_pkts_drop +=3D val} > > + > > This actually results in compiler error when built with > RTE_PORT_STATS_COLLECT =3D 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 =3D > > + (struct rte_port_kni_writer_params *) params; > > + struct rte_port_kni_writer *port; > > + > > + /* Check input parameters */ > > + if ((conf =3D=3D NULL) || > > + (conf->tx_burst_sz =3D=3D 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 =3D rte_zmalloc_socket("PORT", sizeof(*port), > > + RTE_CACHE_LINE_SIZE, socket_id); > > + if (port =3D=3D NULL) { > > + RTE_LOG(ERR, PORT, "%s: Failed to allocate port\n", > > __func__); > > + return NULL; > > + } > > + > > + /* Initialization */ > > + port->kni =3D conf->kni; > > + port->tx_burst_sz =3D conf->tx_burst_sz; > > + port->tx_buf_count =3D 0; > > + port->bsz_mask =3D 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 =3D 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 =3D 0; > > +} > > + > > +static int > > +rte_port_kni_writer_tx(void *port, struct rte_mbuf *pkt) { > > + struct rte_port_kni_writer *p =3D > > + (struct rte_port_kni_writer *) port; > > + > > + p->tx_buf[p->tx_buf_count++] =3D pkt; > > + RTE_PORT_KNI_WRITER_STATS_PKTS_IN_ADD(p, 1); > > + if (p->tx_buf_count >=3D 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 =3D > > + (struct rte_port_kni_writer *) port; > > + uint64_t bsz_mask =3D p->bsz_mask; > > + uint32_t tx_buf_count =3D p->tx_buf_count; > > + uint64_t expr =3D (pkts_mask & (pkts_mask + 1)) | > > + ((pkts_mask & bsz_mask) ^ > > bsz_mask); > > + > > + if (expr =3D=3D 0) { > > + uint64_t n_pkts =3D __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 =3D 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 =3D pkts[n_pkts_ok]; > > + > > + rte_pktmbuf_free(pkt); > > + } > > + } else { > > + for (; pkts_mask;) { > > + uint32_t pkt_index =3D __builtin_ctzll(pkts_mask)= ; > > + uint64_t pkt_mask =3D 1LLU << pkt_index; > > + struct rte_mbuf *pkt =3D pkts[pkt_index]; > > + > > + p->tx_buf[tx_buf_count++] =3D pkt; > > + RTE_PORT_KNI_WRITER_STATS_PKTS_IN_ADD(p, 1); > > + pkts_mask &=3D ~pkt_mask; > > + } > > + > > + p->tx_buf_count =3D tx_buf_count; > > + if (tx_buf_count >=3D p->tx_burst_sz) > > + send_burst(p); > > + } > > + > > + return 0; > > +} > > + > > +static int > > +rte_port_kni_writer_flush(void *port) { > > + struct rte_port_kni_writer *p =3D > > + (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 =3D=3D 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 =3D > > + (struct rte_port_kni_writer *) port; > > + > > + if (stats !=3D 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 =3D { > > + .f_create =3D rte_port_kni_reader_create, > > + .f_free =3D rte_port_kni_reader_free, > > + .f_rx =3D rte_port_kni_reader_rx, > > + .f_stats =3D rte_port_kni_reader_stats_read, > > +}; > > + > > +struct rte_port_out_ops rte_port_kni_writer_ops =3D { > > + .f_create =3D rte_port_kni_writer_create, > > + .f_free =3D rte_port_kni_writer_free, > > + .f_tx =3D rte_port_kni_writer_tx, > > + .f_tx_bulk =3D rte_port_kni_writer_tx_bulk, > > + .f_flush =3D rte_port_kni_writer_flush, > > + .f_stats =3D 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 withou= t > > + * modification, are permitted provided that the following condition= s > > + * are met: > > + * > > + * * Redistributions of source code must retain the above copyrigh= t > > + * 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 deriv= ed > > + * 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 interfac= e > > + * kni_writer: output port built on top of pre-initialized KNI interfa= ce > > + * > > + ***/ > > + > > +#include > > + > > +#include > > + > > +#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 > > >