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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 5/12/20 1:34 PM, Wisam Monther wrote:
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> Sent: Monday, May 11, 2020 3:05 PM
>> To: Wisam Monther <wisamm@mellanox.com>; dev@dpdk.org; Jack Min
>> <jackmin@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
>> jerinjacobk@gmail.com; ajit.khaparde@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@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?

In theory, nothing prevents to call the function twice in
attempt to add MARK action twice. Right now design
design guaranees that it will be used only once since
bitmask is used to mark required actions.

However the design using bitmask is bad since it does
not allow to control order of actions in flow rule.
Same concern is applicable to items bitmask. Application
user should control order, not internal logic.

So, if the design drawback is fixed, something
should ensure that the action is not added twice.
It is not that important for MASK, since it is constant now,
but important for QUEUE.

>>
>>> +
>>> +	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.

As far as I know, C does not work like this. It will not help
to cope with race conditions if your design has race
conditions. Right now it is hard to judge for me, but do/while
loop is definitely unnecessary/useless here.

>>
>>> +
>>> +	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

What if I would like to test a rule which delivers to two
QUEUEs using DUP? I'm just highlighting hard limitation in
the the design. Not a blocker itself, but should be taken
into account.

> 
>>
>>> +
>>> +	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.

Sorry, but it does not answer my question. Is it a memory leak?
If yes, why it is not a problem here?

>>
>>> +
>>> +	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,

Not sure that I understand.

> 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;

const is not required here

>         void (*funct)(struct rte_flow_item *items,
>                         uint8_t items_counter, rte_be32_t src_ip);

The last argument should be a pointer to a structure
with src_ip field to make it generic and be able to
add more argument values there.

>         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?

Yes, with above corrections.

>>
>>> +
>>> +	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.

Sorry, still don't understand why.

>>
>>> +{
>>> +	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?

See below. I was just trying to separately static and
dynamically added parts.

>>
>>> +		/* 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?

Yes, it is not critical, but looking at above array, I don't
understand how it could become ugly.