[dpdk-dev] [PATCH v6 2/5] app/flow-perf: add insertion rate calculation

Wisam Monther wisamm at mellanox.com
Tue May 12 12:34:23 CEST 2020


>-----Original Message-----
>From: Andrew Rybchenko <arybchenko at solarflare.com>
>Sent: Monday, May 11, 2020 3:05 PM
>To: Wisam Monther <wisamm at mellanox.com>; dev at dpdk.org; Jack Min
><jackmin at mellanox.com>; Thomas Monjalon <thomas at monjalon.net>;
>jerinjacobk at gmail.com; ajit.khaparde at broadcom.com
>Subject: Re: [PATCH v6 2/5] app/flow-perf: add insertion rate calculation
>
>On 5/11/20 2:09 PM, Wisam Jaddo wrote:
>> Add insertion rate calculation feature into flow
>> performance application.
>>
>> The application now provide the ability to test
>> insertion rate of specific rte_flow rule, by
>> stressing it to the NIC, and calculate the
>> insertion rate.
>>
>> The application offers some options in the command
>> line, to configure which rule to apply.
>>
>> After that the application will start producing
>> rules with same pattern but increasing the outer IP
>> source address by 1 each time, thus it will give
>> different flow each time, and all other items will
>> have open masks.
>>
>> The current design have single core insertion rate.
>> In the future we may have a multi core insertion
>> rate measurement support in the app.
>>
>> Signed-off-by: Wisam Jaddo <wisamm at mellanox.com>
>> ---
>>  app/test-flow-perf/Makefile            |   3 +
>>  app/test-flow-perf/actions_gen.c       | 164 +++++++++
>>  app/test-flow-perf/actions_gen.h       |  29 ++
>>  app/test-flow-perf/config.h            |  16 +
>>  app/test-flow-perf/flow_gen.c          | 145 ++++++++
>>  app/test-flow-perf/flow_gen.h          |  37 ++
>>  app/test-flow-perf/items_gen.c         | 277 +++++++++++++++
>>  app/test-flow-perf/items_gen.h         |  31 ++
>>  app/test-flow-perf/main.c              | 472 ++++++++++++++++++++++++-
>>  app/test-flow-perf/meson.build         |   3 +
>>  doc/guides/rel_notes/release_20_05.rst |   3 +
>>  doc/guides/tools/flow-perf.rst         | 195 +++++++++-
>>  12 files changed, 1368 insertions(+), 7 deletions(-)
>>  create mode 100644 app/test-flow-perf/actions_gen.c
>>  create mode 100644 app/test-flow-perf/actions_gen.h
>>  create mode 100644 app/test-flow-perf/flow_gen.c
>>  create mode 100644 app/test-flow-perf/flow_gen.h
>>  create mode 100644 app/test-flow-perf/items_gen.c
>>  create mode 100644 app/test-flow-perf/items_gen.h
>>
>> diff --git a/app/test-flow-perf/Makefile b/app/test-flow-perf/Makefile
>> index db043c17a..4f2db7591 100644
>> --- a/app/test-flow-perf/Makefile
>> +++ b/app/test-flow-perf/Makefile
>> @@ -16,6 +16,9 @@ CFLAGS += $(WERROR_FLAGS)
>>  #
>>  # all source are stored in SRCS-y
>>  #
>> +SRCS-y += actions_gen.c
>> +SRCS-y += flow_gen.c
>> +SRCS-y += items_gen.c
>>  SRCS-y += main.c
>>
>>  include $(RTE_SDK)/mk/rte.app.mk
>> diff --git a/app/test-flow-perf/actions_gen.c b/app/test-flow-
>perf/actions_gen.c
>> new file mode 100644
>> index 000000000..16bb3cf20
>> --- /dev/null
>> +++ b/app/test-flow-perf/actions_gen.c
>> @@ -0,0 +1,164 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright 2020 Mellanox Technologies, Ltd
>> + *
>> + * The file contains the implementations of actions generators.
>> + * Each generator is responsible for preparing it's action instance
>> + * and initializing it with needed data.
>> + */
>> +
>> +#include <sys/types.h>
>> +#include <rte_malloc.h>
>> +#include <rte_flow.h>
>> +#include <rte_ethdev.h>
>> +
>> +#include "actions_gen.h"
>> +#include "config.h"
>> +
>> +/* 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];
>> +};
>> +
>> +void
>> +add_mark(struct rte_flow_action *actions,
>> +	uint8_t actions_counter)
>> +{
>> +	static struct rte_flow_action_mark mark_action;
>
>Function-local static variables a bit better than file-local
>or global variable, but just a bit. See below.
>At bare minimum it requires a check that the action is not
>in use already. Same in many cases below.

Yes it's better,
What you mean by " At bare minimum it requires a check that the action is not in use already"
Can you please elaborate?

>
>> +
>> +	do {
>> +		mark_action.id = MARK_ID;
>> +	} while (0);
>
>Why do you use dummy do-while loop here? Many similar cases
>below.

Sometimes, it create the flow before setting the correct id, I think it's compiler stuff
So the dummy loop to make sure the compiler finish it's execution and make sure
When the flow is created the action have correct value.

>
>> +
>> +	actions[actions_counter].type = RTE_FLOW_ACTION_TYPE_MARK;
>> +	actions[actions_counter].conf = &mark_action;
>> +}
>> +
>> +void
>> +add_queue(struct rte_flow_action *actions,
>> +	uint8_t actions_counter, uint16_t queue)
>> +{
>> +	static struct rte_flow_action_queue queue_action;
>
>It does not allow to use the action twice to deliver to
>to queues.

Yes, it's needed only once

>
>> +
>> +	do {
>> +		queue_action.index = queue;
>> +	} while (0);
>> +
>> +	actions[actions_counter].type = RTE_FLOW_ACTION_TYPE_QUEUE;
>> +	actions[actions_counter].conf = &queue_action;
>> +}
>> +
>> +void
>> +add_jump(struct rte_flow_action *actions,
>> +	uint8_t actions_counter, uint16_t next_table)
>> +{
>> +	static struct rte_flow_action_jump jump_action;
>> +
>> +	do {
>> +		jump_action.group = next_table;
>> +	} while (0);
>> +
>> +	actions[actions_counter].type = RTE_FLOW_ACTION_TYPE_JUMP;
>> +	actions[actions_counter].conf = &jump_action;
>> +}
>> +
>> +void
>> +add_rss(struct rte_flow_action *actions,
>> +	uint8_t actions_counter, uint16_t *queues,
>> +	uint16_t queues_number)
>> +{
>> +	static struct rte_flow_action_rss *rss_action;
>> +	static struct action_rss_data *rss_data;
>
>It is better to add an empty line here to split static and
>non-static variable and make it easy to catch the difference.
>
>> +	uint16_t queue;
>> +
>> +	rss_data = rte_malloc("rss_data",
>> +		sizeof(struct action_rss_data), 0);
>
>Does it mean that the second invocation will make
>a memory leak?

Not exactly, because the second invocation may have different queues and need
To be used with different flow, so I think it's ok to re malloc it.

>
>> +
>> +	if (rss_data == NULL)
>> +		rte_exit(EXIT_FAILURE, "No Memory available!");
>> +
>> +	*rss_data = (struct action_rss_data){
>> +		.conf = (struct rte_flow_action_rss){
>> +			.func = RTE_ETH_HASH_FUNCTION_DEFAULT,
>> +			.level = 0,
>> +			.types = GET_RSS_HF(),
>> +			.key_len = sizeof(rss_data->key),
>> +			.queue_num = queues_number,
>> +			.key = rss_data->key,
>> +			.queue = rss_data->queue,
>> +		},
>> +		.key = { 1 },
>> +		.queue = { 0 },
>> +	};
>> +
>> +	for (queue = 0; queue < queues_number; queue++)
>> +		rss_data->queue[queue] = queues[queue];
>> +
>> +	rss_action = &rss_data->conf;
>> +
>> +	actions[actions_counter].type = RTE_FLOW_ACTION_TYPE_RSS;
>> +	actions[actions_counter++].conf = rss_action;
>> +}
>> +
>> +void
>> +add_count(struct rte_flow_action *actions,
>> +	uint8_t actions_counter)
>> +{
>> +	static struct rte_flow_action_count count_action;
>
>Again it means it is impossible to use the action twice in one
>rule.

Yes,
If I removed the static from the inner scope here,
The action will be freed when we reach the end of the file,
And since the design here to add the action into actions array to be used later in the creation,
It will not have correct action in case of none-static in the inner scope.

If any action needs to have privilege to be called twice in single rule
New support needs to be done.

This is the design for it, I'll add into the known limitation.

>> +static void
>> +fill_items(struct rte_flow_item *items,
>> +	uint32_t flow_items, uint32_t outer_ip_src)
>
>It looks like it is better to have the function inside
>items_gen.c. It would allow to make all add_<item> functions
>local to items_gen.c.
>
>> +{
>> +	uint8_t items_counter = 0;
>> +
>> +	/* Support outer items up to tunnel layer only. */
>> +
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_META))
>> +		add_meta_data(items, items_counter++);
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_TAG))
>> +		add_meta_tag(items, items_counter++);
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_ETH))
>> +		add_ether(items, items_counter++);
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VLAN))
>> +		add_vlan(items, items_counter++);
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_IPV4))
>> +		add_ipv4(items, items_counter++, outer_ip_src);
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_IPV6))
>> +		add_ipv6(items, items_counter++, outer_ip_src);
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_TCP))
>> +		add_tcp(items, items_counter++);
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_UDP))
>> +		add_udp(items, items_counter++);
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VXLAN))
>> +		add_vxlan(items, items_counter++);
>> +	if (flow_items &
>FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VXLAN_GPE))
>> +		add_vxlan_gpe(items, items_counter++);
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GRE))
>> +		add_gre(items, items_counter++);
>> +	if (flow_items &
>FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GENEVE))
>> +		add_geneve(items, items_counter++);
>> +	if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GTP))
>> +		add_gtp(items, items_counter++);
>
>It could be done in a loop: define an array of structures
>FLOW_ITEM_MASK(proto) values and add function which should be
>called. The only exception is IPv4/IPv6 which requires extra argument -
>so all add callbacks should have add_data argument
>which is a structure with possible tunings.

Ok, let me re-phrase to make sure I got it:
1- Move it to items_gen.c
2- Create array of structures with all items.
The structure should be something like this:
 static const struct items_dict {
        const uint64_t mask;
        void (*funct)(struct rte_flow_item *items,
                        uint8_t items_counter, rte_be32_t src_ip);
        bool add_args;
    }
3- I need to change the signature "parameters" to all other than ipv4,ipv6 to have another parameter to
Have matched singture.
4- in none-ipv4-ipv6 item I need to call the RTE_SET_USED(src_ip); 
5- loop over the array to add items

Is this right?

>
>> +
>> +	items[items_counter].type = RTE_FLOW_ITEM_TYPE_END;
>> +}
>> +
>> +static void
>> +fill_actions(struct rte_flow_action *actions,
>> +	uint32_t flow_actions, uint32_t counter, uint16_t next_table,
>> +	uint16_t hairpinq)
>
>
>It looks like it is better to have the function inside
>actions_gen.c. It would allow to make all add_<action>
>functions local to actions_gen.c.

Sure, but  this one will remain as is, since the actions have different signature and other actions when be added
Will have more different parameters.

>
>> +{
>> +	uint8_t actions_counter = 0;
>> +	uint16_t hairpin_queues[hairpinq];
>> +	uint16_t queues[RXQ_NUM];
>> +	uint16_t i;
>> +
>> +	/* None-fate actions */
>> +	if (flow_actions &
>FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_MARK))
>> +		add_mark(actions, actions_counter++);
>> +	if (flow_actions &
>FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_COUNT))
>> +		add_count(actions, actions_counter++);
>> +	if (flow_actions &
>FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_SET_META))
>> +		add_set_meta(actions, actions_counter++);
>> +	if (flow_actions &
>FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_SET_TAG))
>> +		add_set_tag(actions, actions_counter++);
>> +
>> +	/* Fate actions */
>> +	if (flow_actions &
>FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_QUEUE))
>> +		add_queue(actions, actions_counter++, counter %
>RXQ_NUM);
>> +	if (flow_actions &
>FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_RSS)) {
>> +		for (i = 0; i < RXQ_NUM; i++)
>> +			queues[i] = i;
>> +		add_rss(actions, actions_counter++, queues, RXQ_NUM);
>> +	}
>> +	if (flow_actions &
>FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_JUMP))
>> +		add_jump(actions, actions_counter++, next_table);
>> +	if (flow_actions &
>FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_PORT_ID))
>> +		add_port_id(actions, actions_counter++);
>> +	if (flow_actions &
>FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_DROP))
>> +		add_drop(actions, actions_counter++);
>> +	if (flow_actions & HAIRPIN_QUEUE_ACTION)
>> +		add_queue(actions, actions_counter++,
>> +			(counter % hairpinq) + RXQ_NUM);
>> +	if (flow_actions & HAIRPIN_RSS_ACTION) {
>> +		for (i = 0; i < hairpinq; i++)
>> +			hairpin_queues[i] = i + RXQ_NUM;
>> +		add_rss(actions, actions_counter++, hairpin_queues,
>hairpinq);
>> +	}
>> +
>> +	actions[actions_counter].type = RTE_FLOW_ACTION_TYPE_END;
>> +}
>> +
>> +
>> +void
>> +add_ether(struct rte_flow_item *items, uint8_t items_counter)
>> +{
>> +	static struct rte_flow_item_eth eth_spec;
>> +	static struct rte_flow_item_eth eth_mask;
>
>Same as actions, it does not allow to have two Eth items
>in one rule. However, it looks like current design does not
>cover it already on mask level.

Yes, indeed.
Already add this in flow_perf.rst as known limitation.

>
>>  		/* Control */
>>  		{ "help",                       0, 0, 0 },
>> +		{ "flows-count",                1, 0, 0 },
>> +		{ "dump-iterations",            0, 0, 0 },
>
>It looks like above it the path which should be defined
>here.

I'm not following, what do you mean?

>
>> +		/* Attributes */
>> +		{ "ingress",                    0, 0, 0 },
>> +		{ "egress",                     0, 0, 0 },
>> +		{ "transfer",                   0, 0, 0 },
>> +		{ "group",                      1, 0, 0 },
>> +		/* Items */
>> +		{ "ether",                      0, 0, 0 },
>> +		{ "vlan",                       0, 0, 0 },
>> +		{ "ipv4",                       0, 0, 0 },
>> +		{ "ipv6",                       0, 0, 0 },
>> +		{ "tcp",                        0, 0, 0 },
>> +		{ "udp",                        0, 0, 0 },
>> +		{ "vxlan",                      0, 0, 0 },
>> +		{ "vxlan-gpe",                  0, 0, 0 },
>> +		{ "gre",                        0, 0, 0 },
>> +		{ "geneve",                     0, 0, 0 },
>> +		{ "gtp",                        0, 0, 0 },
>> +		{ "meta",                       0, 0, 0 },
>> +		{ "tag",                        0, 0, 0 },
>> +		/* Actions */
>> +		{ "port-id",                    0, 0, 0 },
>> +		{ "rss",                        0, 0, 0 },
>> +		{ "queue",                      0, 0, 0 },
>> +		{ "jump",                       0, 0, 0 },
>> +		{ "mark",                       0, 0, 0 },
>> +		{ "count",                      0, 0, 0 },
>> +		{ "set-meta",                   0, 0, 0 },
>> +		{ "set-tag",                    0, 0, 0 },
>> +		{ "drop",                       0, 0, 0 },
>> +		{ "hairpin-queue",              1, 0, 0 },
>> +		{ "hairpin-rss",                1, 0, 0 },
>
>This part should be added by code which iterates by
>flow_options. I.e. allocate lgopts dynamically, copy
>static options there by memcpy() and add dynamic as
>described above. May be flow_options require extra
>field  'has_arg'.

Regard this one,
Some option have special code, like hairpin, group and control, so even with has_arg
It cannot be looped in pretty way, I tried it and it became ugly, I prefer to leave this as it.
I don't think it's critical to get rid of it? What do you think?



More information about the dev mailing list