On 4/30/20 1:33 PM, Wisam Jaddo wrote:
> Add flow performance application skeleton.
>
> Signed-off-by: Wisam Jaddo <wis...@mellanox.com>
> ---
> MAINTAINERS | 5 +
> app/Makefile | 1 +
> app/meson.build | 1 +
> app/test-flow-perf/Makefile | 26 +++
> app/test-flow-perf/main.c | 246 +++++++++++++++++++++++++++
> app/test-flow-perf/meson.build | 11 ++
> app/test-flow-perf/user_parameters.h | 16 ++
> config/common_base | 5 +
> doc/guides/tools/flow-perf.rst | 69 ++++++++
> doc/guides/tools/index.rst | 1 +
> 10 files changed, 381 insertions(+)
> create mode 100644 app/test-flow-perf/Makefile
> create mode 100644 app/test-flow-perf/main.c
> create mode 100644 app/test-flow-perf/meson.build
> create mode 100644 app/test-flow-perf/user_parameters.h
> create mode 100644 doc/guides/tools/flow-perf.rst
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d31a809292..b5632c1bf5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1504,6 +1504,11 @@ T: git://dpdk.org/next/dpdk-next-net
> F: app/test-pmd/
> F: doc/guides/testpmd_app_ug/
>
> +Flow performance tool
> +M: Wisam Jaddo <wis...@mellanox.com>
> +F: app/test-flow-perf
> +F: doc/guides/flow-perf.rst
> +
Shouldn't it be alphabetially sorted? I think by app name.
> Compression performance test application
> T: git://dpdk.org/next/dpdk-next-crypto
> F: app/test-compress-perf/
> diff --git a/app/Makefile b/app/Makefile
> index 823771c5fc..bd823f3db7 100644
> --- a/app/Makefile
> +++ b/app/Makefile
> @@ -9,6 +9,7 @@ DIRS-$(CONFIG_RTE_PROC_INFO) += proc-info
> DIRS-$(CONFIG_RTE_LIBRTE_PDUMP) += pdump
> DIRS-$(CONFIG_RTE_LIBRTE_ACL) += test-acl
> DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += test-cmdline
> +DIRS-$(CONFIG_RTE_TEST_FLOW_PERF) += test-flow-perf
> DIRS-$(CONFIG_RTE_LIBRTE_FIB) += test-fib
> DIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += test-pipeline
> DIRS-$(CONFIG_RTE_LIBRTE_IPSEC) += test-sad
> diff --git a/app/meson.build b/app/meson.build
> index 0f7fe94649..e26f5b72f5 100644
> --- a/app/meson.build
> +++ b/app/meson.build
> @@ -14,6 +14,7 @@ apps = [
> 'test-compress-perf',
> 'test-crypto-perf',
> 'test-eventdev',
> + 'test-flow-perf',
I think 'l' goes after 'i'.
> 'test-fib',
> 'test-pipeline',
> 'test-pmd',
> diff --git a/app/test-flow-perf/Makefile b/app/test-flow-perf/Makefile
> new file mode 100644
> index 0000000000..45b1fb1464
> --- /dev/null
> +++ b/app/test-flow-perf/Makefile
> @@ -0,0 +1,26 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2020 Mellanox Technologies, Ltd
> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +ifeq ($(CONFIG_RTE_TEST_FLOW_PERF),y)
> +
> +#
> +# library name
> +#
> +APP = flow_perf
> +
> +CFLAGS += -DALLOW_EXPERIMENTAL_API
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -Wno-deprecated-declarations
> +CFLAGS += -Wno-unused-function
Why is unused function warning disabled?
> +
> +#
> +# all source are stored in SRCS-y
> +#
> +SRCS-y += main.c
> +
> +include $(RTE_SDK)/mk/rte.app.mk
> +
> +endif
> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
> new file mode 100644
> index 0000000000..156b9ef553
> --- /dev/null
> +++ b/app/test-flow-perf/main.c
> @@ -0,0 +1,246 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + *
> + * This file contain the application main file
> + * This application provides the user the ability to test the
> + * insertion rate for specific rte_flow rule under stress state ~4M rule/
> + *
> + * Then it will also provide packet per second measurement after installing
> + * all rules, the user may send traffic to test the PPS that match the rules
> + * after all rules are installed, to check performance or functionality after
> + * the stress.
> + *
> + * The flows insertion will go for all ports first, then it will print the
> + * results, after that the application will go into forwarding packets mode
> + * it will start receiving traffic if any and then forwarding it back and
> + * gives packet per second measurement.
> + *
> + * Copyright 2020 Mellanox Technologies, Ltd
> + */
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdint.h>
> +#include <inttypes.h>
> +#include <sys/types.h>
> +#include <sys/queue.h>
> +#include <netinet/in.h>
> +#include <setjmp.h>
> +#include <stdarg.h>
> +#include <ctype.h>
> +#include <errno.h>
> +#include <getopt.h>
> +#include <signal.h>
> +#include <stdbool.h>
> +#include <assert.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <sys/time.h>
> +
> +
> +#include <rte_eal.h>
> +#include <rte_common.h>
> +#include <rte_malloc.h>
> +#include <rte_ether.h>
> +#include <rte_ethdev.h>
> +#include <rte_mempool.h>
> +#include <rte_mbuf.h>
> +#include <rte_net.h>
> +#include <rte_flow.h>
> +#include <rte_cycles.h>
> +#include <rte_memory.h>
It looks like many-many above headers are actulally unused.
Please, remove unused headers.
> +
> +#include "user_parameters.h"
> +
> +static uint32_t nb_lcores;
> +static struct rte_mempool *mbuf_mp;
> +
> +static void usage(char *progname)
> +{
> + printf("\nusage: %s", progname);
Is \n missing at the end of format string?
> +}
> +
> +static void
> +args_parse(int argc, char **argv)
> +{
> + char **argvopt;
> + int opt;
> + int opt_idx;
> + static struct option lgopts[] = {
> + /* Control */
> + { "help", 0, 0, 0 },
> + };
> +
> + argvopt = argv;
> +
> + while ((opt = getopt_long(argc, argvopt, "",
> + lgopts, &opt_idx)) != EOF) {
> + switch (opt) {
> + case 0:
> + if (!strcmp(lgopts[opt_idx].name, "help")) {
> + usage(argv[0]);
> + rte_exit(EXIT_SUCCESS, "Displayed help\n");
> + }
> + break;
> + default:
> + usage(argv[0]);
> + printf("Invalid option: %s\n", argv[optind]);
I think it is more friendly to log errors to stderr and log
invalid option message first before usage.
> + rte_exit(EXIT_SUCCESS, "Invalid option\n");
> + break;
> + }
> + }
> +}
> +
> +static void
> +init_port(void)
> +{
> + int ret;
> + uint16_t i, j;
> + uint16_t port_id;
> + uint16_t nr_ports = rte_eth_dev_count_avail();
> + struct rte_eth_hairpin_conf hairpin_conf = {
> + .peer_count = 1,
> + };
> + struct rte_eth_conf port_conf = {
> + .rxmode = {
> + .split_hdr_size = 0,
I think it is not required, since compiler will
do it for you anyway having below initialization.
> + },
> + .rx_adv_conf = {
> + .rss_conf.rss_hf =
> + ETH_RSS_IP |
> + ETH_RSS_UDP |
May be it is better to remove ETH_RSS_UDP by default,
since it is less common that RSS for TCP because of
possible fragmentation and packets from the same
stream delivered to different CPU cores.
> + ETH_RSS_TCP,
> + }
> + };
> + struct rte_eth_txconf txq_conf;
> + struct rte_eth_rxconf rxq_conf;
> + struct rte_eth_dev_info dev_info;
> +
> + if (nr_ports == 0)
> + rte_exit(EXIT_FAILURE, "Error: no port detected\n");
Please, add empty line here to logically separate above
error from pool creation. Right now it looks misleading.
> + mbuf_mp = rte_pktmbuf_pool_create("mbuf_pool",
> + TOTAL_MBUF_NUM, MBUF_CACHE_SIZE,
> + 0, MBUF_SIZE,
> + rte_socket_id());
> +
> + if (mbuf_mp == NULL)
> + rte_exit(EXIT_FAILURE, "Error: can't init mbuf pool\n");
> +
> + for (port_id = 0; port_id < nr_ports; port_id++) {
> + ret = rte_eth_dev_info_get(port_id, &dev_info);
> + if (ret != 0)
> + rte_exit(EXIT_FAILURE,
> + "Error during getting device (port %u)
> info: %s\n",
The line looks long. More than 80 symbols.
Please, use just one TAB to indent relative to rte_exit().
It will make the line a bit shorter.
> + port_id, strerror(-ret));
> +
> + port_conf.txmode.offloads &= dev_info.tx_offload_capa;
Taking into account that txmode.offloads are 0 above,
it looks strange. May be it is added to early in the
patch series?
> + printf(":: initializing port: %d\n", port_id);
> + ret = rte_eth_dev_configure(port_id, RXQs + HAIRPIN_QUEUES,
> + TXQs + HAIRPIN_QUEUES, &port_conf);
RXQs and TXQs are bad since mixing upper and lower
cases letters is bad. Macros are in upper case typically.
May be RXQ_NUM? Or NR_RXQ (however, it will be very easy
to missread as NR_RXD and vise versa, so not good)?
> + if (ret < 0)
> + rte_exit(EXIT_FAILURE,
> + ":: cannot configure device: err=%d,
> port=%u\n",
Too long line, decrease indent
> + ret, port_id);
> +
> + rxq_conf = dev_info.default_rxconf;
> + rxq_conf.offloads = port_conf.rxmode.offloads;
Same here. Also it should take supported offloads
into account.
> + for (i = 0; i < RXQs; i++) {
> + ret = rte_eth_rx_queue_setup(port_id, i, NR_RXD,
> + rte_eth_dev_socket_id(port_id),
> + &rxq_conf,
> + mbuf_mp);
> + if (ret < 0)
> + rte_exit(EXIT_FAILURE,
> + ":: Rx queue setup failed:
> err=%d, port=%u\n",
Too long line, decrease indent
> + ret, port_id);
> + }
> +
> + txq_conf = dev_info.default_txconf;
> + txq_conf.offloads = port_conf.txmode.offloads;
Same here. Also it should take supported offloads
into account.
> +
> + for (i = 0; i < TXQs; i++) {
> + ret = rte_eth_tx_queue_setup(port_id, i, NR_TXD,
> + rte_eth_dev_socket_id(port_id),
> + &txq_conf);
> + if (ret < 0)
> + rte_exit(EXIT_FAILURE,
> + ":: Tx queue setup failed:
> err=%d, port=%u\n",
Too long line, decrease indent
> + ret, port_id);
> + }
> +
> + ret = rte_eth_promiscuous_enable(port_id);
> + if (ret != 0)
> + rte_exit(EXIT_FAILURE,
> + ":: promiscuous mode enable failed:
> err=%s, port=%u\n",
Too long line, decrease indent
> + rte_strerror(-ret), port_id);
> +
> + for (i = RXQs, j = 0; i < RXQs + HAIRPIN_QUEUES; i++, j++) {
> + hairpin_conf.peers[0].port = port_id;
> + hairpin_conf.peers[0].queue = j + TXQs;
> + ret = rte_eth_rx_hairpin_queue_setup(port_id, i,
> + NR_RXD, &hairpin_conf);
> + if (ret != 0)
> + rte_exit(EXIT_FAILURE,
> + ":: Hairpin rx queue setup failed:
> err=%d, port=%u\n",
> + ret, port_id);
> + }
> +
> + for (i = TXQs, j = 0; i < TXQs + HAIRPIN_QUEUES; i++, j++) {
> + hairpin_conf.peers[0].port = port_id;
> + hairpin_conf.peers[0].queue = j + RXQs;
> + ret = rte_eth_tx_hairpin_queue_setup(port_id, i,
> + NR_TXD, &hairpin_conf);
> + if (ret != 0)
> + rte_exit(EXIT_FAILURE,
> + ":: Hairpin tx queue setup failed:
> err=%d, port=%u\n",
> + ret, port_id);
> + }
> +
> + ret = rte_eth_dev_start(port_id);
> + if (ret < 0)
> + rte_exit(EXIT_FAILURE,
> + "rte_eth_dev_start:err=%d, port=%u\n",
> + ret, port_id);
> +
> + printf(":: initializing port: %d done\n", port_id);
> + }
> +}
> +
> +int
> +main(int argc, char **argv)
> +{
> + uint16_t lcore_id;
> + uint16_t port;
> + uint16_t nr_ports;
> + int ret;
> + struct rte_flow_error error;
> +
> + nr_ports = rte_eth_dev_count_avail();
Before EAL init? It is defintely unclear.
If it is done by purpose, please, add a comment
to explain why.
> + ret = rte_eal_init(argc, argv);
> + if (ret < 0)
> + rte_exit(EXIT_FAILURE, "EAL init failed\n");
> +
> + argc -= ret;
> + argv += ret;
> +
> + if (argc > 1)
> + args_parse(argc, argv);
> +
> + init_port();
> +
> + nb_lcores = rte_lcore_count();
> +
> + if (nb_lcores <= 1)
> + rte_exit(EXIT_FAILURE, "This app needs at least two cores\n");
> +
> + RTE_LCORE_FOREACH_SLAVE(lcore_id)
> +
> + if (rte_eal_wait_lcore(lcore_id) < 0)
> + break;
Break what? Is it compile tested?
> +
> + for (port = 0; port < nr_ports; port++) {
> + rte_flow_flush(port, &error);
> + rte_eth_dev_stop(port);
> + rte_eth_dev_close(port);
> + }
> + return 0;
> +}
> diff --git a/app/test-flow-perf/meson.build b/app/test-flow-perf/meson.build
> new file mode 100644
> index 0000000000..ec9bb3b3aa
> --- /dev/null
> +++ b/app/test-flow-perf/meson.build
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2020 Mellanox Technologies, Ltd
> +
> +# meson file, for building this example as part of a main DPDK build.
> +#
> +# To build this example as a standalone application with an already-installed
> +# DPDK instance, use 'make'
> +
> +sources = files(
> + 'main.c',
> +)
> diff --git a/app/test-flow-perf/user_parameters.h
> b/app/test-flow-perf/user_parameters.h
> new file mode 100644
> index 0000000000..56ec7f47b5
> --- /dev/null
> +++ b/app/test-flow-perf/user_parameters.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: BSD-3-Claus
> + *
> + * This file will hold the user parameters values
> + *
> + * Copyright 2020 Mellanox Technologies, Ltd
> + */
> +
> +/** Configuration **/
> +#define RXQs 4
> +#define TXQs 4
> +#define HAIRPIN_QUEUES 4
It makes it Mellanox-speicific from the first patch since
only mlx5 supports hairpin queues. Such things should be
specified using parameters from the very beginning.
> +#define TOTAL_MBUF_NUM 32000
> +#define MBUF_SIZE 2048
> +#define MBUF_CACHE_SIZE 512
> +#define NR_RXD 256
> +#define NR_TXD 256
> diff --git a/config/common_base b/config/common_base
> index 14000ba07e..eaaeaaaee2 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -1124,3 +1124,8 @@ CONFIG_RTE_APP_CRYPTO_PERF=y
> # Compile the eventdev application
> #
> CONFIG_RTE_APP_EVENTDEV=y
> +
> +#
> +# Compile the rte flow perf application
> +#
> +CONFIG_RTE_TEST_FLOW_PERF=y
CONFIG_RTE_APP_FLOW_PERF to follow naming conventions.
> diff --git a/doc/guides/tools/flow-perf.rst b/doc/guides/tools/flow-perf.rst
> new file mode 100644
> index 0000000000..30ce1b6cc0
> --- /dev/null
> +++ b/doc/guides/tools/flow-perf.rst
> @@ -0,0 +1,69 @@
> +.. SPDX-License-Identifier: BSD-3-Clause
> + Copyright 2020 Mellanox Technologies, Ltd
> +
> +RTE Flow performance tool
> +=========================
> +
> +Application for rte_flow performance testing.
> +
> +
> +Compiling the Application
> +=========================
> +The ``test-flow-perf`` application is compiled as part of the main
> compilation
> +of the DPDK libraries and tools.
> +
> +Refer to the DPDK Getting Started Guides for details.
> +The basic compilation steps are:
> +
> +#. Set the required environmental variables and go to the source directory:
> +
> + .. code-block:: console
> +
> + export RTE_SDK=/path/to/rte_sdk
> + cd $RTE_SDK
> +
> +#. Set the compilation target. For example:
> +
> + .. code-block:: console
> +
> + export RTE_TARGET=x86_64-native-linux-gcc
> +
> +#. Build the application:
> +
> + .. code-block:: console
> +
> + make install T=$RTE_TARGET
> +
> +#. The compiled application will be located at:
> +
> + .. code-block:: console
> +
> + $RTE_SDK/$RTE_TARGET/app/flow-perf
> +
> +
> +Running the Application
> +=======================
> +
> +EAL Command-line Options
> +------------------------
> +
> +Please refer to :doc:`EAL parameters (Linux)
> <../linux_gsg/linux_eal_parameters>`
> +or :doc:`EAL parameters (FreeBSD) <../freebsd_gsg/freebsd_eal_parameters>`
> for
> +a list of available EAL command-line options.
> +
> +
> +Flow performance Options
> +------------------------
> +
> +The following are the command-line options for the flow performance
> application.
> +They must be separated from the EAL options, shown in the previous section,
> with
> +a ``--`` separator:
> +
> +.. code-block:: console
> +
> + sudo ./test-flow-perf -n 4 -w 08:00.0,dv_flow_en=1 --
> +
> +The command line options are:
> +
> +* ``--help``
> + Display a help message and quit.
> diff --git a/doc/guides/tools/index.rst b/doc/guides/tools/index.rst
> index 782b30864e..7279daebc6 100644
> --- a/doc/guides/tools/index.rst
> +++ b/doc/guides/tools/index.rst
> @@ -16,3 +16,4 @@ DPDK Tools User Guides
> cryptoperf
> comp_perf
> testeventdev
> + flow-perf
>
I think above should be alphabetically sorted as well.
