Hi,
> -----Original Message-----
> From: Haifei Luo <haif...@nvidia.com>
> Sent: Tuesday, April 6, 2021 9:51 AM
> To: dek...@nvidia.com; Ori Kam <or...@nvidia.com>; Slava Ovsiienko
> <viachesl...@nvidia.com>; Matan Azrad <ma...@nvidia.com>; Shahaf
> Shuler <shah...@nvidia.com>; Wisam Monther <wis...@nvidia.com>
> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon
> <tho...@monjalon.net>; Raslan Darawsheh <rasl...@nvidia.com>; Roni
> Bar Yanai <ron...@nvidia.com>
> Subject: [PATCH] app/flow-perf: support meter policy API
>
> Add option "policy-mtr" to indicate if meter creation will include policy or
> not.
> Meter creation will keep same without it.
>
> With "policy-mtr", policy is introduced. API create_meter_policy is to create
> a
> policy. API create_meter_rule will use it to create meter.
>
> Depends-on: series=16037 ("Support meter policy API ")
> https://patchwork.dpdk.org/project/dpdk/list/?series=16037
>
> Signed-off-by: Haifei Luo <haif...@nvidia.com>
> ---
> app/test-flow-perf/actions_gen.c | 6 ++- app/test-flow-perf/actions_gen.h
> | 3 +-
> app/test-flow-perf/config.h | 6 +--
> app/test-flow-perf/flow_gen.c | 2 +-
> app/test-flow-perf/main.c | 93
> ++++++++++++++++++++++++++++++++++++++--
> doc/guides/tools/flow-perf.rst | 3 ++
> 6 files changed, 102 insertions(+), 11 deletions(-)
>
> diff --git a/app/test-flow-perf/actions_gen.c b/app/test-flow-
> perf/actions_gen.c
> index 1f5c64f..248e19a 100644
> --- a/app/test-flow-perf/actions_gen.c
> +++ b/app/test-flow-perf/actions_gen.c
> @@ -19,7 +19,6 @@
> #include "flow_gen.h"
> #include "config.h"
>
> -
> /* Storage for additional parameters for actions */ struct additional_para {
> uint16_t queue;
> @@ -30,6 +29,7 @@ struct additional_para {
> uint64_t encap_data;
> uint64_t decap_data;
> uint8_t core_idx;
> + uint16_t port_id;
> };
>
> /* Storage for struct rte_flow_action_raw_encap including external data. */
> @@ -907,7 +907,8 @@ struct action_rss_data { void fill_actions(struct
> rte_flow_action *actions, uint64_t *flow_actions,
> uint32_t counter, uint16_t next_table, uint16_t hairpinq,
> - uint64_t encap_data, uint64_t decap_data, uint8_t core_idx)
> + uint64_t encap_data, uint64_t decap_data, uint8_t core_idx,
> + uint16_t port_id)
> {
> struct additional_para additional_para_data;
> uint8_t actions_counter = 0;
> @@ -930,6 +931,7 @@ struct action_rss_data {
> .encap_data = encap_data,
> .decap_data = decap_data,
> .core_idx = core_idx,
> + .port_id = port_id,
> };
>
> if (hairpinq != 0) {
> diff --git a/app/test-flow-perf/actions_gen.h b/app/test-flow-
> perf/actions_gen.h
> index 77353cf..f846734 100644
> --- a/app/test-flow-perf/actions_gen.h
> +++ b/app/test-flow-perf/actions_gen.h
> @@ -19,6 +19,7 @@
>
> void fill_actions(struct rte_flow_action *actions, uint64_t *flow_actions,
> uint32_t counter, uint16_t next_table, uint16_t hairpinq,
> - uint64_t encap_data, uint64_t decap_data, uint8_t core_idx);
> + uint64_t encap_data, uint64_t decap_data, uint8_t core_idx,
> + uint16_t port_id);
>
> #endif /* FLOW_PERF_ACTION_GEN */
> diff --git a/app/test-flow-perf/config.h b/app/test-flow-perf/config.h index
> 3d4696d..50dbf90 100644
> --- a/app/test-flow-perf/config.h
> +++ b/app/test-flow-perf/config.h
> @@ -8,15 +8,15 @@
> #define GET_RSS_HF() (ETH_RSS_IP | ETH_RSS_TCP)
>
> /* Configuration */
> -#define RXQ_NUM 4
> -#define TXQ_NUM 4
> +#define RXQ_NUM 10
> +#define TXQ_NUM 10
This is irrelevant change
> #define TOTAL_MBUF_NUM 32000
> #define MBUF_SIZE 2048
> #define MBUF_CACHE_SIZE 512
> #define NR_RXD 256
> #define NR_TXD 256
> #define MAX_PORTS 64
> -#define METER_CIR 1250000
> +#define METER_CIR 1250000000
Why to change the default?
> #define DEFAULT_METER_PROF_ID 100
>
> /* This is used for encap/decap & header modify actions.
> diff --git a/app/test-flow-perf/flow_gen.c b/app/test-flow-perf/flow_gen.c
> index df4af16..62e4cf5 100644
> --- a/app/test-flow-perf/flow_gen.c
> +++ b/app/test-flow-perf/flow_gen.c
> @@ -61,7 +61,7 @@ struct rte_flow *
>
> fill_actions(actions, flow_actions,
> outer_ip_src, next_table, hairpinq,
> - encap_data, decap_data, core_idx);
> + encap_data, decap_data, core_idx, port_id);
>
> fill_items(items, flow_items, outer_ip_src, core_idx);
>
> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c index
> 66ec776..9a0d758 100644
> --- a/app/test-flow-perf/main.c
> +++ b/app/test-flow-perf/main.c
> @@ -53,6 +53,8 @@
> static uint64_t flow_items[MAX_ITEMS_NUM]; static uint64_t
> flow_actions[MAX_ACTIONS_NUM]; static uint64_t
> flow_attrs[MAX_ATTRS_NUM];
> +uint32_t g_policy_id[MAX_PORTS];
> +
> static uint8_t items_idx, actions_idx, attrs_idx;
>
> static uint64_t ports_mask;
> @@ -61,6 +63,7 @@
> static bool delete_flag;
> static bool dump_socket_mem_flag;
> static bool enable_fwd;
> +static bool policy_mtr;
>
> static struct rte_mempool *mbuf_mp;
> static uint32_t nb_lcores;
> @@ -114,6 +117,13 @@ struct multi_cores_pool {
> .cores_count = 1,
> };
>
> +/* Storage for struct rte_flow_action_rss including external data. */
> +struct action_rss_data {
> + struct rte_flow_action_rss conf;
> + uint8_t key[40];
> + uint16_t queue[128];
> +};
> +
Why you create this struct?
> static void
> usage(char *progname)
> {
> @@ -131,6 +141,7 @@ struct multi_cores_pool {
> printf(" --enable-fwd: To enable packets forwarding"
> " after insertion\n");
> printf(" --portmask=N: hexadecimal bitmask of ports used\n");
> + printf(" --policy-mtr: To create meter with policy\n");
It's worthy to mention the current supported policies here, and also what is
the default.
>
> printf("To set flow attributes:\n");
> printf(" --ingress: set ingress attribute in flows\n"); @@ -569,6
> +580,7 @@ struct multi_cores_pool {
> { "enable-fwd", 0, 0, 0 },
> { "portmask", 1, 0, 0 },
> { "cores", 1, 0, 0 },
> + { "policy-mtr", 0, 0, 0 },
> /* Attributes */
> { "ingress", 0, 0, 0 },
> { "egress", 0, 0, 0 },
> @@ -798,6 +810,10 @@ struct multi_cores_pool {
> rte_exit(EXIT_FAILURE, " ");
> }
> }
> + if (strcmp(lgopts[opt_idx].name,
> + "policy-mtr") == 0) {
> + policy_mtr = true;
> + }
> break;
> default:
> fprintf(stderr, "Invalid option: %s\n", argv[optind]);
> @@ -909,6 +925,60 @@ struct multi_cores_pool { }
>
> static void
> +create_meter_policy(void)
> +{
> + struct rte_mtr_error error;
> + uint32_t policy_id;
> + int ret, i, port_id;
> + const struct rte_flow_action *acts[RTE_COLORS];
> + struct action_rss_data rss_data;
> + struct rte_flow_action g_actions[2], r_actions[2];
> + uint16_t nr_ports;
> +
> + memset(&rss_data, 0, sizeof(rss_data));
> + rss_data.conf.func = RTE_ETH_HASH_FUNCTION_DEFAULT;
> + rss_data.conf.level = 0;
> + rss_data.conf.types = GET_RSS_HF();
> + rss_data.conf.key_len = 0;
> + rss_data.conf.key = NULL;
> + rss_data.conf.queue_num = RXQ_NUM;
> + uint16_t q_data[RXQ_NUM];
> + rss_data.conf.queue = q_data;
> +
> + for (i = 0; i < RXQ_NUM; i++)
> + q_data[i] = i;
> +
> + for (i = 0; i < RXQ_NUM; i++)
> + rss_data.queue[i] = i;
> +
> + g_actions[0].type = RTE_FLOW_ACTION_TYPE_RSS;
> + g_actions[0].conf = &(rss_data.conf);
> + g_actions[1].type = RTE_FLOW_ACTION_TYPE_END;
> + g_actions[1].conf = NULL;
> +
This is some fixed behavior, it's not that right to use something like this,
Example of other solution to have option like:
--meter-w-policy=G:<GREEN_ACTIONS>, Y:<YELLOW_ACTIONS>, R:<RED_ACTIONS>
This way it will be easier to use, and more dynamic, it's never right to set
fixed actions
For such application.
> + r_actions[0].type = RTE_FLOW_ACTION_TYPE_DROP;
> + r_actions[0].conf = NULL;
> + r_actions[1].type = RTE_FLOW_ACTION_TYPE_END;
> + r_actions[1].conf = NULL;
> +
> + acts[0] = &g_actions[0];
> + acts[1] = NULL;
> + acts[2] = &r_actions[0];
> +
> + nr_ports = rte_eth_dev_count_avail();
> + for (port_id = 0; port_id < nr_ports; port_id++) {
> + policy_id = port_id + 10;
> + ret = rte_mtr_meter_policy_create(port_id,
> + policy_id,
> + acts, &error);
> + if (ret)
> + printf("meter add failed port_id %d\n",
> + port_id);
> + g_policy_id[port_id] = policy_id;
> + }
> +}
> +
> +static void
> create_meter_rule(int port_id, uint32_t counter) {
> int ret;
> @@ -924,7 +994,14 @@ struct multi_cores_pool {
>
> /*create meter*/
> params.meter_profile_id = default_prof_id;
> - ret = rte_mtr_create(port_id, counter, ¶ms, 1, &error);
> +
> + if (!policy_mtr)
> + ret = rte_mtr_create(port_id, counter, ¶ms, 1, &error);
> + else {
> + params.meter_policy_id = g_policy_id[port_id];
> + ret = rte_mtr_create(port_id, counter, ¶ms, 0, &error);
> + }
> +
> if (ret != 0) {
> printf("Port %u create meter idx(%d) error(%d) message:
> %s\n",
> port_id, counter, error.type,
> @@ -938,11 +1015,16 @@ struct multi_cores_pool { {
> struct rte_mtr_error error;
>
> - if (rte_mtr_destroy(port_id, counter, &error)) {
> - printf("Port %u destroy meter(%d) error(%d) message:
> %s\n",
> + if (policy_mtr) {
> + if (rte_mtr_meter_policy_delete(port_id, counter+1,
> &error))
> + printf("erro delete policy %d\n", counter+1);
> + } else {
> + if (rte_mtr_destroy(port_id, counter, &error)) {
> + printf("Port %u destroy meter(%d) error(%d)
> message: %s\n",
> port_id, counter, error.type,
> error.message ? error.message : "(no stated
> reason)");
> - rte_exit(EXIT_FAILURE, "Error in deleting meter rule");
> + rte_exit(EXIT_FAILURE, "Error in deleting meter
> rule");
> + }
> }
> }
>
> @@ -1876,6 +1958,9 @@ struct multi_cores_pool {
>
> if (has_meter())
> create_meter_profile();
> + if (policy_mtr)
> + create_meter_policy();
> +
> rte_eal_mp_remote_launch(run_rte_flow_handler_cores, NULL,
> CALL_MAIN);
>
> if (enable_fwd) {
> diff --git a/doc/guides/tools/flow-perf.rst b/doc/guides/tools/flow-perf.rst
> index 017e200..50fe683 100644
> --- a/doc/guides/tools/flow-perf.rst
> +++ b/doc/guides/tools/flow-perf.rst
> @@ -349,3 +349,6 @@ Actions:
> * ``--meter``
> Add meter action to all flows actions.
> Currently, 1 meter profile -> N meter rules -> N rte flows.
> +
> +* ``--policy-mtr``
> + Add policy-mtr to create meter with policy.
> --
> 1.8.3.1
