>-----Original Message-----
>From: Andrew Rybchenko <arybche...@solarflare.com>
>Sent: Wednesday, May 6, 2020 5:26 PM
>To: Wisam Monther <wis...@mellanox.com>; dev@dpdk.org; Jack Min
><jack...@mellanox.com>; Thomas Monjalon <tho...@monjalon.net>;
>jerinjac...@gmail.com; gerlitz...@gmail.com; l....@epfl.ch;
>ajit.khapa...@broadcom.com
>Subject: Re: [dpdk-dev] [PATCH v5 1/5] app/flow-perf: add flow performance
>skeleton
>
>On 5/6/20 3:36 PM, Wisam Jaddo wrote:
>> Add flow performance application skeleton.
>>
>> Signed-off-by: Wisam Jaddo <wis...@mellanox.com>
>> ---
>
>[snip]
>
>> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c new
>> file mode 100644 index 000000000..7a924cdb7
>> --- /dev/null
>> +++ b/app/test-flow-perf/main.c
>> @@ -0,0 +1,200 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright 2020 Mellanox Technologies, Ltd
>> + *
>> + * 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.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <stdint.h>
>> +#include <inttypes.h>
>> +#include <stdarg.h>
>> +#include <errno.h>
>> +#include <getopt.h>
>> +#include <signal.h>
>> +#include <stdbool.h>
>> +#include <sys/time.h>
>> +
>> +#include <rte_malloc.h>
>> +#include <rte_mempool.h>
>> +#include <rte_mbuf.h>
>> +#include <rte_ethdev.h>
>> +#include <rte_flow.h>
>> +
>> +#include "config.h"
>> +
>> +static uint32_t nb_lcores;
>> +static struct rte_mempool *mbuf_mp;
>> +
>> +static void
>> +usage(char *progname)
>> +{
>> + printf("\nusage: %s\n", progname);
>> +}
>> +
>> +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")) {
>
>DPDK coding style recommends to compare vs 0 instead of logical not.
Ok, will move it
>
>> + usage(argv[0]);
>> + rte_exit(EXIT_SUCCESS, "Displayed help\n");
>> + }
>> + break;
>> + default:
>> + printf("Invalid option: %s\n", argv[optind]);
>
>Again, sorry if I missed reply: Why error is not logged to stderr?
No, I missed it, will move it to stderr
>
>> + usage(argv[0]);
>> + rte_exit(EXIT_SUCCESS, "Invalid option\n");
>> + break;
>> + }
>> + }
>> +}
>> +
>> +static void
>> +init_port(void)
>> +{
>> + int ret;
>> + uint16_t i;
>> + uint16_t port_id;
>> + uint16_t nr_ports;
>> + struct rte_eth_conf port_conf = {
>> + .rx_adv_conf = {
>> + .rss_conf.rss_hf =
>> + ETH_RSS_IP |
>> + ETH_RSS_TCP,
>> + }
>> + };
>> + struct rte_eth_txconf txq_conf;
>> + struct rte_eth_rxconf rxq_conf;
>> + struct rte_eth_dev_info dev_info;
>> +
>> + nr_ports = rte_eth_dev_count_avail();
>> + if (nr_ports == 0)
>> + rte_exit(EXIT_FAILURE, "Error: no port detected\n");
>> +
>> + 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",
>> + port_id, strerror(-ret));
>> +
>> + port_conf.txmode.offloads &= dev_info.tx_offload_capa;
>> + port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
>> +
>> + printf(":: initializing port: %d\n", port_id);
>> +
>> + ret = rte_eth_dev_configure(port_id, RXQ_NUM,
>> + TXQ_NUM, &port_conf);
>> + if (ret < 0)
>> + rte_exit(EXIT_FAILURE,
>> + ":: cannot configure device: err=%d,
>port=%u\n",
>> + ret, port_id);
>> +
>> + rxq_conf = dev_info.default_rxconf;
>> + rxq_conf.offloads = port_conf.rxmode.offloads;
>
>
>As far as I know there is no necessity to repeat port offlaod on queue level.
>So, the line is not necesary.
Yes you are right, just checked the code, it takes the offloads from the port
it self.
Will remove it.
>
>> +
>> + for (i = 0; i < RXQ_NUM; 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",
>> + ret, port_id);
>> + }
>> +
>> + txq_conf = dev_info.default_txconf;
>> + txq_conf.offloads = port_conf.txmode.offloads;
>
>As far as I know there is no necessity to repeat port offlaod on queue level.
>So, the line is not necesary.
Will remove it
>
>> +
>> + for (i = 0; i < TXQ_NUM; 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",
>> + ret, port_id);
>> + }
>> +
>> + /* Catch all packets from traffic generator. */
>> + ret = rte_eth_promiscuous_enable(port_id);
>> + if (ret != 0)
>> + rte_exit(EXIT_FAILURE,
>> + ":: promiscuous mode enable failed: err=%s,
>port=%u\n",
>> + rte_strerror(-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)
>> +{
>> + int ret;
>> + uint16_t port;
>> + struct rte_flow_error error;
>> +
>> + 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_ETH_FOREACH_DEV(port) {
>> + rte_flow_flush(port, &error);
>> + rte_eth_dev_stop(port);
>> + rte_eth_dev_close(port);
>> + }
>> + return 0;
>> +}
>
>[snip]
>
>> diff --git a/config/common_base b/config/common_base index
>> 14000ba07..b2edd5267 100644 diff --git
>> a/doc/guides/rel_notes/release_20_05.rst
>> b/doc/guides/rel_notes/release_20_05.rst
>> index b124c3f28..258b1e03e 100644
>> --- a/doc/guides/rel_notes/release_20_05.rst
>> +++ b/doc/guides/rel_notes/release_20_05.rst
>> @@ -212,6 +212,16 @@ New Features
>> * Added IPsec inbound load-distribution support for ipsec-secgw
>application
>> using NIC load distribution feature(Flow Director).
>>
>> +* **Added flow performance application.**
>> +
>> + Add new application to test rte_flow performance.
>> +
>> + Application features:
>> + * Measure rte_flow insertion rate.
>> + * Measure rte_flow deletion rate.
>> + * Dump rte_flow memory consumption.
>> + * Measure packet per second forwarding.
>
>I think above lines should be added in appropriate patches which really do it.
What do you mean?
each feature should add it's own line in the same commit?
>
>> +
>>
>> Removed Items
>> -------------
>
>[snip]
