| Message ID | 20191204144345.5736-2-pbhagavatula@marvell.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jerin Jacob |
| Headers | show |
| Series | example/l3fwd: introduce event device support | expand |
| Context | Check | Description |
|---|---|---|
| ci/iol-mellanox-Performance | success | Performance Testing PASS |
| ci/iol-compilation | success | Compile Testing PASS |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/Intel-compilation | success | Compilation OK |
| ci/checkpatch | success | coding style OK |
> Add framework to enable event device as a producer of packets. > To switch between event mode and poll mode the following options > have been added: > `--mode="eventdev"` or `--mode="poll"` > Also, allow the user to select the schedule type to be either > RTE_SCHED_TYPE_ORDERED, RTE_SCHED_TYPE_ATOMIC or RTE_SCHED_TYPE_PARALLEL > through: > `--eventq-sched="ordered"` or `--eventq-sched="atomic"` or > `--eventq-sched="parallel"` > > Poll mode is still the default operation mode. > > Signed-off-by: Sunil Kumar Kori <skori@marvell.com> > --- > examples/l3fwd/Makefile | 2 +- > examples/l3fwd/l3fwd.h | 6 +++ > examples/l3fwd/l3fwd_event.c | 75 ++++++++++++++++++++++++++++++++++++ > examples/l3fwd/l3fwd_event.h | 54 ++++++++++++++++++++++++++ > examples/l3fwd/main.c | 41 +++++++++++++++++--- > examples/l3fwd/meson.build | 4 +- > 6 files changed, 174 insertions(+), 8 deletions(-) > create mode 100644 examples/l3fwd/l3fwd_event.c > create mode 100644 examples/l3fwd/l3fwd_event.h > > diff --git a/examples/l3fwd/Makefile b/examples/l3fwd/Makefile > index b2dbf2607..c892b867b 100644 > --- a/examples/l3fwd/Makefile > +++ b/examples/l3fwd/Makefile > @@ -5,7 +5,7 @@ > APP = l3fwd > > # all source are stored in SRCS-y > -SRCS-y := main.c l3fwd_lpm.c l3fwd_em.c > +SRCS-y := main.c l3fwd_lpm.c l3fwd_em.c l3fwd_event.c > > # Build using pkg-config variables if possible > ifeq ($(shell pkg-config --exists libdpdk && echo 0),0) > diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h > index 293fb1fa2..cd17a41b3 100644 > --- a/examples/l3fwd/l3fwd.h > +++ b/examples/l3fwd/l3fwd.h > @@ -5,6 +5,9 @@ > #ifndef __L3_FWD_H__ > #define __L3_FWD_H__ > > +#include <stdbool.h> > + Why do we need it here? > +#include <rte_ethdev.h> > #include <rte_vect.h> > > #define DO_RFC_1812_CHECKS > @@ -169,6 +172,9 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len) > } > #endif /* DO_RFC_1812_CHECKS */ > > +void > +print_usage(const char *prgname); > + > /* Function pointers for LPM or EM functionality. */ > void > setup_lpm(const int socketid); > diff --git a/examples/l3fwd/l3fwd_event.c b/examples/l3fwd/l3fwd_event.c > new file mode 100644 > index 000000000..3892720be > --- /dev/null > +++ b/examples/l3fwd/l3fwd_event.c > @@ -0,0 +1,75 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(C) 2019 Marvell International Ltd. > + */ > + > +#include <stdbool.h> > +#include <getopt.h> > + > +#include "l3fwd.h" > +#include "l3fwd_event.h" > + > +static void > +parse_mode(const char *optarg) > +{ > + struct l3fwd_event_resources *evt_rsrc = l3fwd_get_eventdev_rsrc(); > + > + if (!strncmp(optarg, "poll", 4)) That looks a bit clumsy and error-prone. Just strcmp(optarg, "poll") seems enough here. Same for other similar places. > + evt_rsrc->enabled = false; > + else if (!strncmp(optarg, "eventdev", 8)) > + evt_rsrc->enabled = true; > +} > + > +static void > +parse_eventq_sync(const char *optarg) > +{ > + struct l3fwd_event_resources *evt_rsrc = l3fwd_get_eventdev_rsrc(); > + > + if (!strncmp(optarg, "ordered", 7)) > + evt_rsrc->sched_type = RTE_SCHED_TYPE_ORDERED; > + if (!strncmp(optarg, "atomic", 6)) > + evt_rsrc->sched_type = RTE_SCHED_TYPE_ATOMIC; > + if (!strncmp(optarg, "parallel", 8)) > + evt_rsrc->sched_type = RTE_SCHED_TYPE_PARALLEL; > +} > + > +static void > +l3fwd_parse_eventdev_args(char **argv, int argc) > +{ > + const struct option eventdev_lgopts[] = { > + {CMD_LINE_OPT_MODE, 1, 0, CMD_LINE_OPT_MODE_NUM}, > + {CMD_LINE_OPT_EVENTQ_SYNC, 1, 0, CMD_LINE_OPT_EVENTQ_SYNC_NUM}, > + {NULL, 0, 0, 0} > + }; > + char *prgname = argv[0]; > + char **argvopt = argv; > + int32_t option_index; > + int32_t opt; > + > + while ((opt = getopt_long(argc, argvopt, "", eventdev_lgopts, > + &option_index)) != EOF) { > + switch (opt) { > + case CMD_LINE_OPT_MODE_NUM: > + parse_mode(optarg); > + break; > + > + case CMD_LINE_OPT_EVENTQ_SYNC_NUM: > + parse_eventq_sync(optarg); > + break; > + > + default: > + print_usage(prgname); > + exit(1); > + } > + } > +} > + > +void > +l3fwd_event_resource_setup(void) > +{ > + struct l3fwd_event_resources *evt_rsrc = l3fwd_get_eventdev_rsrc(); > + > + /* Parse eventdev command line options */ > + l3fwd_parse_eventdev_args(evt_rsrc->args, evt_rsrc->nb_args); > + if (!evt_rsrc->enabled) > + return; > +} > diff --git a/examples/l3fwd/l3fwd_event.h b/examples/l3fwd/l3fwd_event.h > new file mode 100644 > index 000000000..c95296c38 > --- /dev/null > +++ b/examples/l3fwd/l3fwd_event.h > @@ -0,0 +1,54 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(C) 2019 Marvell International Ltd. > + */ > + > +#ifndef __L3FWD_EVENTDEV_H__ > +#define __L3FWD_EVENTDEV_H__ > + > +#include <rte_common.h> > +#include <rte_eventdev.h> > +#include <rte_spinlock.h> > + > +#include "l3fwd.h" > + > +#define CMD_LINE_OPT_MODE "mode" > +#define CMD_LINE_OPT_EVENTQ_SYNC "eventq-sched" > + > +enum { > + CMD_LINE_OPT_MODE_NUM = 265, > + CMD_LINE_OPT_EVENTQ_SYNC_NUM, > +}; > + > +struct l3fwd_event_resources { > + uint8_t sched_type; > + uint8_t enabled; > + uint8_t nb_args; > + char **args; > +}; > + > +static inline struct l3fwd_event_resources * > +l3fwd_get_eventdev_rsrc(void) > +{ > + static const char name[RTE_MEMZONE_NAMESIZE] = "l3fwd_event_rsrc"; > + const struct rte_memzone *mz; > + > + mz = rte_memzone_lookup(name); > + > + if (mz != NULL) > + return mz->addr; > + > + mz = rte_memzone_reserve(name, sizeof(struct l3fwd_event_resources), > + 0, 0); > + if (mz != NULL) { > + memset(mz->addr, 0, sizeof(struct l3fwd_event_resources)); > + return mz->addr; > + } > + > + rte_exit(EXIT_FAILURE, "Unable to allocate memory for eventdev cfg\n"); > + > + return NULL; > +} Does this function really need to be inline? It wouldn't be fast anyway. Another question - do you really need memzone here? Wouldn't just rte_malloc() be enough? > + > +void l3fwd_event_resource_setup(void); > + > +#endif /* __L3FWD_EVENTDEV_H__ */ > diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c > index 4dea12a65..19ca4483c 100644 > --- a/examples/l3fwd/main.c > +++ b/examples/l3fwd/main.c > @@ -13,12 +13,12 @@ > #include <errno.h> > #include <getopt.h> > #include <signal.h> > -#include <stdbool.h> > > #include <rte_common.h> > #include <rte_vect.h> > #include <rte_byteorder.h> > #include <rte_log.h> > +#include <rte_malloc.h> > #include <rte_memory.h> > #include <rte_memcpy.h> > #include <rte_eal.h> > @@ -33,7 +33,6 @@ > #include <rte_random.h> > #include <rte_debug.h> > #include <rte_ether.h> > -#include <rte_ethdev.h> > #include <rte_mempool.h> > #include <rte_mbuf.h> > #include <rte_ip.h> > @@ -46,6 +45,7 @@ > #include <cmdline_parse_etheraddr.h> > > #include "l3fwd.h" > +#include "l3fwd_event.h" > > /* > * Configurable number of RX/TX ring descriptors > @@ -274,7 +274,7 @@ init_lcore_rx_queues(void) > } > > /* display usage */ > -static void > +void > print_usage(const char *prgname) > { > fprintf(stderr, "%s [EAL options] --" > @@ -289,7 +289,9 @@ print_usage(const char *prgname) > " [--hash-entry-num]" > " [--ipv6]" > " [--parse-ptype]" > - " [--per-port-pool]\n\n" > + " [--per-port-pool]" > + " [--mode]" > + " [--eventq-sched]\n\n" > > " -p PORTMASK: Hexadecimal bitmask of ports to configure\n" > " -P : Enable promiscuous mode\n" > @@ -304,7 +306,13 @@ print_usage(const char *prgname) > " --hash-entry-num: Specify the hash entry number in hexadecimal to be setup\n" > " --ipv6: Set if running ipv6 packets\n" > " --parse-ptype: Set to use software to analyze packet type\n" > - " --per-port-pool: Use separate buffer pool per port\n\n", > + " --per-port-pool: Use separate buffer pool per port\n" > + " --mode: Packet transfer mode for I/O, poll or eventdev\n" > + " Default mode = poll\n" > + " --eventq-sched: Event queue synchronization method " > + " ordered, atomic or parallel.\n\t\t" > + " Default: atomic\n\t\t" > + " Valid only if --mode=eventdev\n\n", > prgname); > } > > @@ -504,11 +512,19 @@ static const struct option lgopts[] = { > static int > parse_args(int argc, char **argv) > { > + struct l3fwd_event_resources *evt_rsrc; > int opt, ret; > char **argvopt; > int option_index; > char *prgname = argv[0]; > > + evt_rsrc = l3fwd_get_eventdev_rsrc(); > + evt_rsrc->args = rte_zmalloc("l3fwd_event_args", sizeof(char *), 0); > + if (evt_rsrc->args == NULL) > + rte_exit(EXIT_FAILURE, > + "Unable to allocate memory for eventdev arg"); > + evt_rsrc->args[0] = argv[0]; > + evt_rsrc->nb_args++; > argvopt = argv; > > /* Error or normal output strings. */ > @@ -538,6 +554,15 @@ parse_args(int argc, char **argv) > l3fwd_lpm_on = 1; > break; > > + case '?': > + /* Eventdev options are encountered skip for > + * now and processed later. > + */ > + evt_rsrc->args[evt_rsrc->nb_args] = > + argv[optind - 1]; > + evt_rsrc->nb_args++; > + break; > + All this construction with first allocating space for eventdev specific params copying them and parsing in a special function - looks like an overkill to me. Why not just to call parse_mode()/parse_eventq.. functions straight from here? > /* long options */ > case CMD_LINE_OPT_CONFIG_NUM: > ret = parse_config(optarg); > @@ -811,6 +836,7 @@ prepare_ptype_parser(uint16_t portid, uint16_t queueid) > int > main(int argc, char **argv) > { > + struct l3fwd_event_resources *evt_rsrc; > struct lcore_conf *qconf; > struct rte_eth_dev_info dev_info; > struct rte_eth_txconf *txconf; > @@ -839,11 +865,16 @@ main(int argc, char **argv) > *(uint64_t *)(val_eth + portid) = dest_eth_addr[portid]; > } > > + evt_rsrc = l3fwd_get_eventdev_rsrc(); > + RTE_SET_USED(evt_rsrc); > /* parse application arguments (after the EAL ones) */ > ret = parse_args(argc, argv); > if (ret < 0) > rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n"); > > + /* Configure eventdev parameters if user has requested */ > + l3fwd_event_resource_setup(); > + > if (check_lcore_params() < 0) > rte_exit(EXIT_FAILURE, "check_lcore_params failed\n"); > > diff --git a/examples/l3fwd/meson.build b/examples/l3fwd/meson.build > index 6dd4b9022..864327c7b 100644 > --- a/examples/l3fwd/meson.build > +++ b/examples/l3fwd/meson.build > @@ -6,7 +6,7 @@ > # To build this example as a standalone application with an already-installed > # DPDK instance, use 'make' > > -deps += ['hash', 'lpm'] > +deps += ['hash', 'lpm', 'eventdev'] > sources = files( > - 'l3fwd_em.c', 'l3fwd_lpm.c', 'main.c' > + 'l3fwd_em.c', 'l3fwd_lpm.c', 'l3fwd_event.c', 'main.c' > ) > -- > 2.17.1
>-----Original Message----- >From: dev <dev-bounces@dpdk.org> On Behalf Of Ananyev, >Konstantin >Sent: Friday, January 3, 2020 6:19 PM >To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Jerin >Jacob Kollanukkaran <jerinj@marvell.com>; Kovacevic, Marko ><marko.kovacevic@intel.com>; Ori Kam <orika@mellanox.com>; >Richardson, Bruce <bruce.richardson@intel.com>; Nicolau, Radu ><radu.nicolau@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>; >Kantecki, Tomasz <tomasz.kantecki@intel.com>; Sunil Kumar Kori ><skori@marvell.com> >Cc: dev@dpdk.org >Subject: Re: [dpdk-dev] [PATCH v2 01/11] examples/l3fwd: add >framework for event device > > >> Add framework to enable event device as a producer of packets. >> To switch between event mode and poll mode the following options >> have been added: >> `--mode="eventdev"` or `--mode="poll"` >> Also, allow the user to select the schedule type to be either >> RTE_SCHED_TYPE_ORDERED, RTE_SCHED_TYPE_ATOMIC or >RTE_SCHED_TYPE_PARALLEL >> through: >> `--eventq-sched="ordered"` or `--eventq-sched="atomic"` or >> `--eventq-sched="parallel"` >> >> Poll mode is still the default operation mode. >> >> Signed-off-by: Sunil Kumar Kori <skori@marvell.com> >> --- >> examples/l3fwd/Makefile | 2 +- >> examples/l3fwd/l3fwd.h | 6 +++ >> examples/l3fwd/l3fwd_event.c | 75 >++++++++++++++++++++++++++++++++++++ >> examples/l3fwd/l3fwd_event.h | 54 >++++++++++++++++++++++++++ >> examples/l3fwd/main.c | 41 +++++++++++++++++--- >> examples/l3fwd/meson.build | 4 +- >> 6 files changed, 174 insertions(+), 8 deletions(-) >> create mode 100644 examples/l3fwd/l3fwd_event.c >> create mode 100644 examples/l3fwd/l3fwd_event.h >> >> diff --git a/examples/l3fwd/Makefile b/examples/l3fwd/Makefile >> index b2dbf2607..c892b867b 100644 >> --- a/examples/l3fwd/Makefile >> +++ b/examples/l3fwd/Makefile >> @@ -5,7 +5,7 @@ >> APP = l3fwd >> >> # all source are stored in SRCS-y >> -SRCS-y := main.c l3fwd_lpm.c l3fwd_em.c >> +SRCS-y := main.c l3fwd_lpm.c l3fwd_em.c l3fwd_event.c >> >> # Build using pkg-config variables if possible >> ifeq ($(shell pkg-config --exists libdpdk && echo 0),0) >> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h >> index 293fb1fa2..cd17a41b3 100644 >> --- a/examples/l3fwd/l3fwd.h >> +++ b/examples/l3fwd/l3fwd.h >> @@ -5,6 +5,9 @@ >> #ifndef __L3_FWD_H__ >> #define __L3_FWD_H__ >> >> +#include <stdbool.h> >> + > >Why do we need it here? Looks like an artifact I will remove it in next version. > >> +#include <rte_ethdev.h> >> #include <rte_vect.h> >> >> #define DO_RFC_1812_CHECKS >> @@ -169,6 +172,9 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, >uint32_t link_len) >> } >> #endif /* DO_RFC_1812_CHECKS */ >> >> +void >> +print_usage(const char *prgname); >> + >> /* Function pointers for LPM or EM functionality. */ >> void >> setup_lpm(const int socketid); >> diff --git a/examples/l3fwd/l3fwd_event.c >b/examples/l3fwd/l3fwd_event.c >> new file mode 100644 >> index 000000000..3892720be >> --- /dev/null >> +++ b/examples/l3fwd/l3fwd_event.c >> @@ -0,0 +1,75 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright(C) 2019 Marvell International Ltd. >> + */ >> + >> +#include <stdbool.h> >> +#include <getopt.h> >> + >> +#include "l3fwd.h" >> +#include "l3fwd_event.h" >> + >> +static void >> +parse_mode(const char *optarg) >> +{ >> + struct l3fwd_event_resources *evt_rsrc = >l3fwd_get_eventdev_rsrc(); >> + >> + if (!strncmp(optarg, "poll", 4)) > >That looks a bit clumsy and error-prone. >Just strcmp(optarg, "poll") seems enough here. >Same for other similar places. Will simplify in next version. > >> + evt_rsrc->enabled = false; >> + else if (!strncmp(optarg, "eventdev", 8)) >> + evt_rsrc->enabled = true; >> +} >> + >> +static void >> +parse_eventq_sync(const char *optarg) >> +{ >> + struct l3fwd_event_resources *evt_rsrc = >l3fwd_get_eventdev_rsrc(); >> + >> + if (!strncmp(optarg, "ordered", 7)) >> + evt_rsrc->sched_type = RTE_SCHED_TYPE_ORDERED; >> + if (!strncmp(optarg, "atomic", 6)) >> + evt_rsrc->sched_type = RTE_SCHED_TYPE_ATOMIC; >> + if (!strncmp(optarg, "parallel", 8)) >> + evt_rsrc->sched_type = RTE_SCHED_TYPE_PARALLEL; >> +} >> + >> +static void >> +l3fwd_parse_eventdev_args(char **argv, int argc) >> +{ >> + const struct option eventdev_lgopts[] = { >> + {CMD_LINE_OPT_MODE, 1, 0, >CMD_LINE_OPT_MODE_NUM}, >> + {CMD_LINE_OPT_EVENTQ_SYNC, 1, 0, >CMD_LINE_OPT_EVENTQ_SYNC_NUM}, >> + {NULL, 0, 0, 0} >> + }; >> + char *prgname = argv[0]; >> + char **argvopt = argv; >> + int32_t option_index; >> + int32_t opt; >> + >> + while ((opt = getopt_long(argc, argvopt, "", eventdev_lgopts, >> + &option_index)) != EOF) { >> + switch (opt) { >> + case CMD_LINE_OPT_MODE_NUM: >> + parse_mode(optarg); >> + break; >> + >> + case CMD_LINE_OPT_EVENTQ_SYNC_NUM: >> + parse_eventq_sync(optarg); >> + break; >> + >> + default: >> + print_usage(prgname); >> + exit(1); >> + } >> + } >> +} >> + >> +void >> +l3fwd_event_resource_setup(void) >> +{ >> + struct l3fwd_event_resources *evt_rsrc = >l3fwd_get_eventdev_rsrc(); >> + >> + /* Parse eventdev command line options */ >> + l3fwd_parse_eventdev_args(evt_rsrc->args, evt_rsrc- >>nb_args); >> + if (!evt_rsrc->enabled) >> + return; >> +} >> diff --git a/examples/l3fwd/l3fwd_event.h >b/examples/l3fwd/l3fwd_event.h >> new file mode 100644 >> index 000000000..c95296c38 >> --- /dev/null >> +++ b/examples/l3fwd/l3fwd_event.h >> @@ -0,0 +1,54 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright(C) 2019 Marvell International Ltd. >> + */ >> + >> +#ifndef __L3FWD_EVENTDEV_H__ >> +#define __L3FWD_EVENTDEV_H__ >> + >> +#include <rte_common.h> >> +#include <rte_eventdev.h> >> +#include <rte_spinlock.h> >> + >> +#include "l3fwd.h" >> + >> +#define CMD_LINE_OPT_MODE "mode" >> +#define CMD_LINE_OPT_EVENTQ_SYNC "eventq-sched" >> + >> +enum { >> + CMD_LINE_OPT_MODE_NUM = 265, >> + CMD_LINE_OPT_EVENTQ_SYNC_NUM, >> +}; >> + >> +struct l3fwd_event_resources { >> + uint8_t sched_type; >> + uint8_t enabled; >> + uint8_t nb_args; >> + char **args; >> +}; >> + >> +static inline struct l3fwd_event_resources * >> +l3fwd_get_eventdev_rsrc(void) >> +{ >> + static const char name[RTE_MEMZONE_NAMESIZE] = >"l3fwd_event_rsrc"; >> + const struct rte_memzone *mz; >> + >> + mz = rte_memzone_lookup(name); >> + >> + if (mz != NULL) >> + return mz->addr; >> + >> + mz = rte_memzone_reserve(name, sizeof(struct >l3fwd_event_resources), >> + 0, 0); >> + if (mz != NULL) { >> + memset(mz->addr, 0, sizeof(struct >l3fwd_event_resources)); >> + return mz->addr; >> + } >> + >> + rte_exit(EXIT_FAILURE, "Unable to allocate memory for >eventdev cfg\n"); >> + >> + return NULL; >> +} > >Does this function really need to be inline? >It wouldn't be fast anyway. >Another question - do you really need memzone here? >Wouldn't just rte_malloc() be enough? Will remove inline in next version. rte_malloc would call for a global variable which I'm trying to avoid. I don't think there is any harm in using named memzone. > >> + >> +void l3fwd_event_resource_setup(void); >> + >> +#endif /* __L3FWD_EVENTDEV_H__ */ >> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c >> index 4dea12a65..19ca4483c 100644 >> --- a/examples/l3fwd/main.c >> +++ b/examples/l3fwd/main.c >> @@ -13,12 +13,12 @@ >> #include <errno.h> >> #include <getopt.h> >> #include <signal.h> >> -#include <stdbool.h> >> >> #include <rte_common.h> >> #include <rte_vect.h> >> #include <rte_byteorder.h> >> #include <rte_log.h> >> +#include <rte_malloc.h> >> #include <rte_memory.h> >> #include <rte_memcpy.h> >> #include <rte_eal.h> >> @@ -33,7 +33,6 @@ >> #include <rte_random.h> >> #include <rte_debug.h> >> #include <rte_ether.h> >> -#include <rte_ethdev.h> >> #include <rte_mempool.h> >> #include <rte_mbuf.h> >> #include <rte_ip.h> >> @@ -46,6 +45,7 @@ >> #include <cmdline_parse_etheraddr.h> >> >> #include "l3fwd.h" >> +#include "l3fwd_event.h" >> >> /* >> * Configurable number of RX/TX ring descriptors >> @@ -274,7 +274,7 @@ init_lcore_rx_queues(void) >> } >> >> /* display usage */ >> -static void >> +void >> print_usage(const char *prgname) >> { >> fprintf(stderr, "%s [EAL options] --" >> @@ -289,7 +289,9 @@ print_usage(const char *prgname) >> " [--hash-entry-num]" >> " [--ipv6]" >> " [--parse-ptype]" >> - " [--per-port-pool]\n\n" >> + " [--per-port-pool]" >> + " [--mode]" >> + " [--eventq-sched]\n\n" >> >> " -p PORTMASK: Hexadecimal bitmask of ports to >configure\n" >> " -P : Enable promiscuous mode\n" >> @@ -304,7 +306,13 @@ print_usage(const char *prgname) >> " --hash-entry-num: Specify the hash entry number in >hexadecimal to be setup\n" >> " --ipv6: Set if running ipv6 packets\n" >> " --parse-ptype: Set to use software to analyze packet >type\n" >> - " --per-port-pool: Use separate buffer pool per >port\n\n", >> + " --per-port-pool: Use separate buffer pool per port\n" >> + " --mode: Packet transfer mode for I/O, poll or >eventdev\n" >> + " Default mode = poll\n" >> + " --eventq-sched: Event queue synchronization >method " >> + " ordered, atomic or parallel.\n\t\t" >> + " Default: atomic\n\t\t" >> + " Valid only if --mode=eventdev\n\n", >> prgname); >> } >> >> @@ -504,11 +512,19 @@ static const struct option lgopts[] = { >> static int >> parse_args(int argc, char **argv) >> { >> + struct l3fwd_event_resources *evt_rsrc; >> int opt, ret; >> char **argvopt; >> int option_index; >> char *prgname = argv[0]; >> >> + evt_rsrc = l3fwd_get_eventdev_rsrc(); >> + evt_rsrc->args = rte_zmalloc("l3fwd_event_args", sizeof(char >*), 0); >> + if (evt_rsrc->args == NULL) >> + rte_exit(EXIT_FAILURE, >> + "Unable to allocate memory for >eventdev arg"); >> + evt_rsrc->args[0] = argv[0]; >> + evt_rsrc->nb_args++; >> argvopt = argv; >> >> /* Error or normal output strings. */ >> @@ -538,6 +554,15 @@ parse_args(int argc, char **argv) >> l3fwd_lpm_on = 1; >> break; >> >> + case '?': >> + /* Eventdev options are encountered skip for >> + * now and processed later. >> + */ >> + evt_rsrc->args[evt_rsrc->nb_args] = >> + argv[optind - 1]; >> + evt_rsrc->nb_args++; >> + break; >> + > >All this construction with first allocating space for eventdev specific >params >copying them and parsing in a special function - looks like an overkill to >me. >Why not just to call parse_mode()/parse_eventq.. functions straight >from here? We were trying to make minimal changes to l3fwd but I guess we could have common cmdline parse functions. > > >> /* long options */ >> case CMD_LINE_OPT_CONFIG_NUM: >> ret = parse_config(optarg); >> @@ -811,6 +836,7 @@ prepare_ptype_parser(uint16_t portid, >uint16_t queueid) >> int >> main(int argc, char **argv) >> { >> + struct l3fwd_event_resources *evt_rsrc; >> struct lcore_conf *qconf; >> struct rte_eth_dev_info dev_info; >> struct rte_eth_txconf *txconf; >> @@ -839,11 +865,16 @@ main(int argc, char **argv) >> *(uint64_t *)(val_eth + portid) = >dest_eth_addr[portid]; >> } >> >> + evt_rsrc = l3fwd_get_eventdev_rsrc(); >> + RTE_SET_USED(evt_rsrc); >> /* parse application arguments (after the EAL ones) */ >> ret = parse_args(argc, argv); >> if (ret < 0) >> rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n"); >> >> + /* Configure eventdev parameters if user has requested */ >> + l3fwd_event_resource_setup(); >> + >> if (check_lcore_params() < 0) >> rte_exit(EXIT_FAILURE, "check_lcore_params >failed\n"); >> >> diff --git a/examples/l3fwd/meson.build >b/examples/l3fwd/meson.build >> index 6dd4b9022..864327c7b 100644 >> --- a/examples/l3fwd/meson.build >> +++ b/examples/l3fwd/meson.build >> @@ -6,7 +6,7 @@ >> # To build this example as a standalone application with an already- >installed >> # DPDK instance, use 'make' >> >> -deps += ['hash', 'lpm'] >> +deps += ['hash', 'lpm', 'eventdev'] >> sources = files( >> - 'l3fwd_em.c', 'l3fwd_lpm.c', 'main.c' >> + 'l3fwd_em.c', 'l3fwd_lpm.c', 'l3fwd_event.c', 'main.c' >> ) >> -- >> 2.17.1
> >> +struct l3fwd_event_resources { > >> + uint8_t sched_type; > >> + uint8_t enabled; > >> + uint8_t nb_args; > >> + char **args; > >> +}; > >> + > >> +static inline struct l3fwd_event_resources * > >> +l3fwd_get_eventdev_rsrc(void) > >> +{ > >> + static const char name[RTE_MEMZONE_NAMESIZE] = > >"l3fwd_event_rsrc"; > >> + const struct rte_memzone *mz; > >> + > >> + mz = rte_memzone_lookup(name); > >> + > >> + if (mz != NULL) > >> + return mz->addr; > >> + > >> + mz = rte_memzone_reserve(name, sizeof(struct > >l3fwd_event_resources), > >> + 0, 0); > >> + if (mz != NULL) { > >> + memset(mz->addr, 0, sizeof(struct > >l3fwd_event_resources)); > >> + return mz->addr; > >> + } > >> + > >> + rte_exit(EXIT_FAILURE, "Unable to allocate memory for > >eventdev cfg\n"); > >> + > >> + return NULL; > >> +} > > > >Does this function really need to be inline? > >It wouldn't be fast anyway. > >Another question - do you really need memzone here? > >Wouldn't just rte_malloc() be enough? > > Will remove inline in next version. > rte_malloc would call for a global variable which I'm > trying to avoid. If you plan to move that function into .c file, you don't need a global var. it could be static local one. > I don't think there is any harm in using > named memzone. I don't see any harm, though malloc+var will be faster I think. Though up to you - no strong opinion here.
>> >> +struct l3fwd_event_resources { >> >> + uint8_t sched_type; >> >> + uint8_t enabled; >> >> + uint8_t nb_args; >> >> + char **args; >> >> +}; >> >> + >> >> +static inline struct l3fwd_event_resources * >> >> +l3fwd_get_eventdev_rsrc(void) >> >> +{ >> >> + static const char name[RTE_MEMZONE_NAMESIZE] = >> >"l3fwd_event_rsrc"; >> >> + const struct rte_memzone *mz; >> >> + >> >> + mz = rte_memzone_lookup(name); >> >> + >> >> + if (mz != NULL) >> >> + return mz->addr; >> >> + >> >> + mz = rte_memzone_reserve(name, sizeof(struct >> >l3fwd_event_resources), >> >> + 0, 0); >> >> + if (mz != NULL) { >> >> + memset(mz->addr, 0, sizeof(struct >> >l3fwd_event_resources)); >> >> + return mz->addr; >> >> + } >> >> + >> >> + rte_exit(EXIT_FAILURE, "Unable to allocate memory for >> >eventdev cfg\n"); >> >> + >> >> + return NULL; >> >> +} >> > >> >Does this function really need to be inline? >> >It wouldn't be fast anyway. >> >Another question - do you really need memzone here? >> >Wouldn't just rte_malloc() be enough? >> >> Will remove inline in next version. >> rte_malloc would call for a global variable which I'm >> trying to avoid. > >If you plan to move that function into .c file, >you don't need a global var. it could be static local one. > >> I don't think there is any harm in using >> named memzone. > >I don't see any harm, though malloc+var will be faster I think. >Though up to you - no strong opinion here. Maybe we can cut down some init time. I will move it to .c next version. Thanks, Pavan
diff --git a/examples/l3fwd/Makefile b/examples/l3fwd/Makefile index b2dbf2607..c892b867b 100644 --- a/examples/l3fwd/Makefile +++ b/examples/l3fwd/Makefile @@ -5,7 +5,7 @@ APP = l3fwd # all source are stored in SRCS-y -SRCS-y := main.c l3fwd_lpm.c l3fwd_em.c +SRCS-y := main.c l3fwd_lpm.c l3fwd_em.c l3fwd_event.c # Build using pkg-config variables if possible ifeq ($(shell pkg-config --exists libdpdk && echo 0),0) diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h index 293fb1fa2..cd17a41b3 100644 --- a/examples/l3fwd/l3fwd.h +++ b/examples/l3fwd/l3fwd.h @@ -5,6 +5,9 @@ #ifndef __L3_FWD_H__ #define __L3_FWD_H__ +#include <stdbool.h> + +#include <rte_ethdev.h> #include <rte_vect.h> #define DO_RFC_1812_CHECKS @@ -169,6 +172,9 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len) } #endif /* DO_RFC_1812_CHECKS */ +void +print_usage(const char *prgname); + /* Function pointers for LPM or EM functionality. */ void setup_lpm(const int socketid); diff --git a/examples/l3fwd/l3fwd_event.c b/examples/l3fwd/l3fwd_event.c new file mode 100644 index 000000000..3892720be --- /dev/null +++ b/examples/l3fwd/l3fwd_event.c @@ -0,0 +1,75 @@ +/* SPDX-License-Identifier: BSD-3-Clause + * Copyright(C) 2019 Marvell International Ltd. + */ + +#include <stdbool.h> +#include <getopt.h> + +#include "l3fwd.h" +#include "l3fwd_event.h" + +static void +parse_mode(const char *optarg) +{ + struct l3fwd_event_resources *evt_rsrc = l3fwd_get_eventdev_rsrc(); + + if (!strncmp(optarg, "poll", 4)) + evt_rsrc->enabled = false; + else if (!strncmp(optarg, "eventdev", 8)) + evt_rsrc->enabled = true; +} + +static void +parse_eventq_sync(const char *optarg) +{ + struct l3fwd_event_resources *evt_rsrc = l3fwd_get_eventdev_rsrc(); + + if (!strncmp(optarg, "ordered", 7)) + evt_rsrc->sched_type = RTE_SCHED_TYPE_ORDERED; + if (!strncmp(optarg, "atomic", 6)) + evt_rsrc->sched_type = RTE_SCHED_TYPE_ATOMIC; + if (!strncmp(optarg, "parallel", 8)) + evt_rsrc->sched_type = RTE_SCHED_TYPE_PARALLEL; +} + +static void +l3fwd_parse_eventdev_args(char **argv, int argc) +{ + const struct option eventdev_lgopts[] = { + {CMD_LINE_OPT_MODE, 1, 0, CMD_LINE_OPT_MODE_NUM}, + {CMD_LINE_OPT_EVENTQ_SYNC, 1, 0, CMD_LINE_OPT_EVENTQ_SYNC_NUM}, + {NULL, 0, 0, 0} + }; + char *prgname = argv[0]; + char **argvopt = argv; + int32_t option_index; + int32_t opt; + + while ((opt = getopt_long(argc, argvopt, "", eventdev_lgopts, + &option_index)) != EOF) { + switch (opt) { + case CMD_LINE_OPT_MODE_NUM: + parse_mode(optarg); + break; + + case CMD_LINE_OPT_EVENTQ_SYNC_NUM: + parse_eventq_sync(optarg); + break; + + default: + print_usage(prgname); + exit(1); + } + } +} + +void +l3fwd_event_resource_setup(void) +{ + struct l3fwd_event_resources *evt_rsrc = l3fwd_get_eventdev_rsrc(); + + /* Parse eventdev command line options */ + l3fwd_parse_eventdev_args(evt_rsrc->args, evt_rsrc->nb_args); + if (!evt_rsrc->enabled) + return; +} diff --git a/examples/l3fwd/l3fwd_event.h b/examples/l3fwd/l3fwd_event.h new file mode 100644 index 000000000..c95296c38 --- /dev/null +++ b/examples/l3fwd/l3fwd_event.h @@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: BSD-3-Clause + * Copyright(C) 2019 Marvell International Ltd. + */ + +#ifndef __L3FWD_EVENTDEV_H__ +#define __L3FWD_EVENTDEV_H__ + +#include <rte_common.h> +#include <rte_eventdev.h> +#include <rte_spinlock.h> + +#include "l3fwd.h" + +#define CMD_LINE_OPT_MODE "mode" +#define CMD_LINE_OPT_EVENTQ_SYNC "eventq-sched" + +enum { + CMD_LINE_OPT_MODE_NUM = 265, + CMD_LINE_OPT_EVENTQ_SYNC_NUM, +}; + +struct l3fwd_event_resources { + uint8_t sched_type; + uint8_t enabled; + uint8_t nb_args; + char **args; +}; + +static inline struct l3fwd_event_resources * +l3fwd_get_eventdev_rsrc(void) +{ + static const char name[RTE_MEMZONE_NAMESIZE] = "l3fwd_event_rsrc"; + const struct rte_memzone *mz; + + mz = rte_memzone_lookup(name); + + if (mz != NULL) + return mz->addr; + + mz = rte_memzone_reserve(name, sizeof(struct l3fwd_event_resources), + 0, 0); + if (mz != NULL) { + memset(mz->addr, 0, sizeof(struct l3fwd_event_resources)); + return mz->addr; + } + + rte_exit(EXIT_FAILURE, "Unable to allocate memory for eventdev cfg\n"); + + return NULL; +} + +void l3fwd_event_resource_setup(void); + +#endif /* __L3FWD_EVENTDEV_H__ */ diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c index 4dea12a65..19ca4483c 100644 --- a/examples/l3fwd/main.c +++ b/examples/l3fwd/main.c @@ -13,12 +13,12 @@ #include <errno.h> #include <getopt.h> #include <signal.h> -#include <stdbool.h> #include <rte_common.h> #include <rte_vect.h> #include <rte_byteorder.h> #include <rte_log.h> +#include <rte_malloc.h> #include <rte_memory.h> #include <rte_memcpy.h> #include <rte_eal.h> @@ -33,7 +33,6 @@ #include <rte_random.h> #include <rte_debug.h> #include <rte_ether.h> -#include <rte_ethdev.h> #include <rte_mempool.h> #include <rte_mbuf.h> #include <rte_ip.h> @@ -46,6 +45,7 @@ #include <cmdline_parse_etheraddr.h> #include "l3fwd.h" +#include "l3fwd_event.h" /* * Configurable number of RX/TX ring descriptors @@ -274,7 +274,7 @@ init_lcore_rx_queues(void) } /* display usage */ -static void +void print_usage(const char *prgname) { fprintf(stderr, "%s [EAL options] --" @@ -289,7 +289,9 @@ print_usage(const char *prgname) " [--hash-entry-num]" " [--ipv6]" " [--parse-ptype]" - " [--per-port-pool]\n\n" + " [--per-port-pool]" + " [--mode]" + " [--eventq-sched]\n\n" " -p PORTMASK: Hexadecimal bitmask of ports to configure\n" " -P : Enable promiscuous mode\n" @@ -304,7 +306,13 @@ print_usage(const char *prgname) " --hash-entry-num: Specify the hash entry number in hexadecimal to be setup\n" " --ipv6: Set if running ipv6 packets\n" " --parse-ptype: Set to use software to analyze packet type\n" - " --per-port-pool: Use separate buffer pool per port\n\n", + " --per-port-pool: Use separate buffer pool per port\n" + " --mode: Packet transfer mode for I/O, poll or eventdev\n" + " Default mode = poll\n" + " --eventq-sched: Event queue synchronization method " + " ordered, atomic or parallel.\n\t\t" + " Default: atomic\n\t\t" + " Valid only if --mode=eventdev\n\n", prgname); } @@ -504,11 +512,19 @@ static const struct option lgopts[] = { static int parse_args(int argc, char **argv) { + struct l3fwd_event_resources *evt_rsrc; int opt, ret; char **argvopt; int option_index; char *prgname = argv[0]; + evt_rsrc = l3fwd_get_eventdev_rsrc(); + evt_rsrc->args = rte_zmalloc("l3fwd_event_args", sizeof(char *), 0); + if (evt_rsrc->args == NULL) + rte_exit(EXIT_FAILURE, + "Unable to allocate memory for eventdev arg"); + evt_rsrc->args[0] = argv[0]; + evt_rsrc->nb_args++; argvopt = argv; /* Error or normal output strings. */ @@ -538,6 +554,15 @@ parse_args(int argc, char **argv) l3fwd_lpm_on = 1; break; + case '?': + /* Eventdev options are encountered skip for + * now and processed later. + */ + evt_rsrc->args[evt_rsrc->nb_args] = + argv[optind - 1]; + evt_rsrc->nb_args++; + break; + /* long options */ case CMD_LINE_OPT_CONFIG_NUM: ret = parse_config(optarg); @@ -811,6 +836,7 @@ prepare_ptype_parser(uint16_t portid, uint16_t queueid) int main(int argc, char **argv) { + struct l3fwd_event_resources *evt_rsrc; struct lcore_conf *qconf; struct rte_eth_dev_info dev_info; struct rte_eth_txconf *txconf; @@ -839,11 +865,16 @@ main(int argc, char **argv) *(uint64_t *)(val_eth + portid) = dest_eth_addr[portid]; } + evt_rsrc = l3fwd_get_eventdev_rsrc(); + RTE_SET_USED(evt_rsrc); /* parse application arguments (after the EAL ones) */ ret = parse_args(argc, argv); if (ret < 0) rte_exit(EXIT_FAILURE, "Invalid L3FWD parameters\n"); + /* Configure eventdev parameters if user has requested */ + l3fwd_event_resource_setup(); + if (check_lcore_params() < 0) rte_exit(EXIT_FAILURE, "check_lcore_params failed\n"); diff --git a/examples/l3fwd/meson.build b/examples/l3fwd/meson.build index 6dd4b9022..864327c7b 100644 --- a/examples/l3fwd/meson.build +++ b/examples/l3fwd/meson.build @@ -6,7 +6,7 @@ # To build this example as a standalone application with an already-installed # DPDK instance, use 'make' -deps += ['hash', 'lpm'] +deps += ['hash', 'lpm', 'eventdev'] sources = files( - 'l3fwd_em.c', 'l3fwd_lpm.c', 'main.c' + 'l3fwd_em.c', 'l3fwd_lpm.c', 'l3fwd_event.c', 'main.c' )