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. 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. 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. :-) 4. Shall I use DPDK_16.07 in the librte_port/rte_port_version.map file? 2016-06-10 7:42 GMT+08:00 Dumitrescu, Cristian < cristian.dumitrescu at 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 at gmail.com] > > Sent: Saturday, May 28, 2016 12:26 PM > > To: Dumitrescu, Cristian <cristian.dumitrescu at intel.com> > > Cc: dev at dpdk.org; WeiJie Zhuang <zhuangwj at 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 at 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( > > + ¶m->socket_id, ¶m->core_id, > > + ¶m->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(¶m->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
