Re: [PATCH v4] ethdev: add flow API to expand RSS flows

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Wed, Jun 27, 2018 at 04:55:25PM +0200, Nelio Laranjeiro wrote:
> Introduce an helper for PMD to expand easily flows items list with RSS
> action into multiple flow items lists with priority information.
> 
> For instance a user items list being "eth / end" with rss action types
> "ipv4-udp ipv6-udp end" needs to be expanded into three items lists:
> 
>  - eth
>  - eth / ipv4 / udp
>  - eth / ipv6 / udp
> 
> to match the user request.  Some drivers are unable to reach such
> request without this expansion, this API is there to help those.
> Only PMD should use such API for their internal cooking, the application
> will still handle a single flow.
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---
> 
> Changes in v4:
> 
> - Replace the expanded algorithm with a graph to support also tunnel
> pattern matching.

Looks like a useful helper, nice to see that it's much shorter than the
previous versions. A few nits below, mainly suggestions for documentation.

<snip>
> +
> +int
> +rte_flow_expand_rss(struct rte_flow_expand_rss *buf, size_t size,
> +		    const struct rte_flow_item *pat, uint64_t types,
> +		    const struct rte_flow_expand_node nodes[],
> +		    const int node_entry_index)

Please add short documentation reminder on top of this definition (i.e. a
copy of the first sentence from rte_flow_driver.h).

> +{
> +	const int elt_n = 8;
> +	const struct rte_flow_item *item;
> +	const struct rte_flow_expand_node *node = &nodes[node_entry_index];
> +	const int *next_node;
> +	const int *stack[elt_n];
> +	int stack_n = 0;
> +	struct rte_flow_item flow_items[elt_n];
> +	unsigned int i;
> +	size_t lsize;
> +	size_t user_pattern_size = 0;
> +	void *addr = NULL;
> +
> +	lsize = sizeof(*buf) +
> +		/* Size for the list of patterns. */
> +		sizeof(*buf->patterns) +
> +		RTE_ALIGN_CEIL(elt_n * sizeof(*item), sizeof(void *)) +
> +		/* Size for priorities. */
> +		sizeof(*buf->priority) +
> +		RTE_ALIGN_CEIL(elt_n * sizeof(uint32_t), sizeof(void *));
> +	if (lsize <= size) {
> +		buf->priority = (void *)(buf + 1);
> +		buf->priority[0] = 0;
> +		buf->patterns = (void *)&buf->priority[elt_n];
> +		buf->patterns[0] = (void *)&buf->patterns[elt_n];
> +		buf->entries = 0;
> +		addr = buf->patterns[0];
> +	}
> +	for (item = pat; item->type != RTE_FLOW_ITEM_TYPE_END; item++) {
> +		const struct rte_flow_expand_node *next = NULL;
> +
> +		for (i = 0; node->next && node->next[i]; ++i) {
> +			next = &nodes[node->next[i]];
> +			if (next->type == item->type)
> +				break;
> +		}
> +		if (next)
> +			node = next;
> +		user_pattern_size += sizeof(*item);
> +	}
> +	user_pattern_size += sizeof(*item); /**< Handle END item. */

This comment shouldn't be in Doxygen format.

> +	lsize += user_pattern_size;
> +	/* Copy the user pattern in the first entry of the buffer. */
> +	if (lsize <= size) {
> +		rte_memcpy(addr, pat, user_pattern_size);
> +		addr = (void *)(((uintptr_t)addr) + user_pattern_size);
> +		buf->priority[buf->entries] = 0;
> +		buf->entries = 1;
> +	}
> +	/* Start expanding. */
> +	memset(flow_items, 0, sizeof(flow_items));
> +	user_pattern_size -= sizeof(*item);
> +	next_node = node->next;
> +	stack[stack_n] = next_node;
> +	node = next_node ? &nodes[*next_node] : NULL;
> +	while (node) {
> +		flow_items[stack_n].type = node->type;
> +		if ((node->rss_types & types) == node->rss_types) {
> +			/*
> +			 * compute the number of items to copy from the
> +			 * expansion and copy it.
> +			 * When the stack_n is 0, there are 1 element in it,
> +			 * plus the addition END item.
> +			 */
> +			int elt = stack_n + 2;
> +
> +			flow_items[stack_n + 1].type = RTE_FLOW_ITEM_TYPE_END;
> +			lsize += elt * sizeof(*item) + user_pattern_size;
> +			if (lsize <= size) {
> +				size_t n = elt * sizeof(*item);
> +
> +				buf->priority[buf->entries] = stack_n + 1;
> +				buf->patterns[buf->entries++] = addr;
> +				rte_memcpy(addr, buf->patterns[0],
> +					   user_pattern_size);
> +				addr = (void *)(((uintptr_t)addr) +
> +						user_pattern_size);
> +				rte_memcpy(addr, flow_items, n);
> +				addr = (void *) (((uintptr_t)addr) + n);

Extra space after (void *).

> +			}
> +		}
> +		/* Go deeper. */
> +		if (node->next) {
> +			next_node = node->next;
> +			stack[++stack_n] = next_node;

Since stack[] contains at most elt_n (8) elements, even assuming it's always
plenty enough, I think it would be wise to check for any overflow before
incrementing stack_n and return an error if so.

> +		} else if (*(next_node + 1)) {
> +			/* Follow up with the next possibility. */
> +			++next_node;
> +		} else {
> +			/* Move to the next path. */
> +			if (stack_n)
> +				next_node = stack[--stack_n];
> +			next_node++;
> +			stack[stack_n] = next_node;
> +		}
> +		node = *next_node ? &nodes[*next_node] : NULL;
> +	};
> +	return lsize;
> +}
> diff --git a/lib/librte_ethdev/rte_flow_driver.h b/lib/librte_ethdev/rte_flow_driver.h
> index 1c90c600d..538b46e54 100644
> --- a/lib/librte_ethdev/rte_flow_driver.h
> +++ b/lib/librte_ethdev/rte_flow_driver.h
> @@ -114,6 +114,54 @@ struct rte_flow_ops {
>  const struct rte_flow_ops *
>  rte_flow_ops_get(uint16_t port_id, struct rte_flow_error *error);
>  
> +/** Macro to create the expansion graph. */

=> Helper macro to build input graph for rte_flow_expand_rss().

Mostly rewording suggestions from here on.

> +#define RTE_FLOW_EXPAND_ITEMS(...) \

You should keep the same prefix for all objects associated with this
function. Everything should start with "rte_flow_expand_rss" for
consistency.

=> RTE_FLOW_EXPAND_RSS_NEXT(...)

> +	(const int []){ \
> +		__VA_ARGS__, 0, \
> +	}
> +
> +/** structure to generate the expansion graph. */

=> Node object of input graph for rte_flow_expand_rss().

> +struct rte_flow_expand_node {

=> struct rte_flow_expand_rss_node

> +	const int *const next; /**< list of items finalised by 0. */

=> List of next node indexes. Index 0 is interpreted as a terminator.

> +	const enum rte_flow_item_type type; /**< Item type to add. */

=> Pattern item type of current node.

> +	uint64_t rss_types; /**< RSS bit-field value. */

=> RSS types bit-field associated with this node (see ETH_RSS_* definitions).

> +};
> +
> +/**
> + * Expansion structure for RSS flows.
> + */

=> Object returned by rte_flow_expand_rss().

This block could fit a single line by the way.

> +struct rte_flow_expand_rss {
> +	uint32_t entries; /**< Number of entries in the following arrays. */

=> Number of entries in @p patterns and @p priorities.

> +	struct rte_flow_item **patterns; /**< Expanded pattern array. */
> +	uint32_t *priority; /**< Priority offset for each expansion. */

How about priority => priorities since there are as many of them as
patterns?

> +};

Another suggestion regarding this structure definition, since it's entirely
written by rte_flow_expand_rss() based on an arbitrarily-sized user-provided
buffer, and given it's not a public API, a flexible array member could make
sense.

This would highlight the link between patterns and priorities and make the
result more convenient to use thanks to fewer pointers:

 struct rte_flow_expand_rss {
     uint32_t entries;
     struct {
          struct rte_flow_item *pattern;
          uint32_t priority;
     }  entry[];
 };

> +
> +/**
> + * Expand RSS flows into several possible flows according to the RSS hash
> + * fields requested and the driver capabilities.
> + *
> + * @param[in,out] buf

Since this buffer is only written to: @param[in,out] => @param[out]

> + *   Buffer to store the result expansion.

=> Buffer for expansion result. May be truncated if @p size is not large
   enough.

> + * @param[in] size
> + *   Size in octets of the buffer.

=> Buffer size in bytes. If 0, @p buf can be NULL.

> + * @param[in] pat

pat => pattern

> + *   User flow pattern.
> + * @param[in] types
> + *   RSS types expected (see ETH_RSS_*).

=> RSS types to expand (see ETH_RSS_* definitions).

> + * @param[in] nodes.

nodes => graph

> + *   Expansion graph of possibilities for the RSS.

=> Input graph to expand @p pattern according to @p types.

> + * @param[in] node_entry_index

"[in]" is unnecessary since it's not a pointer.

node_entry_index => graph_root_index

> + *   The index in the \p nodes array as start point.

Let's keep "@" instead of "\" for directives.

=> Index of root node in @p graph, typically 0.

> + *
> + * @return
> + *   The size in octets used to expand.

=> A positive value representing the size of @p buf in bytes regardless of
   @p size on success, a negative errno value otherwise and rte_errno is
   set.

> + */
> +int
> +rte_flow_expand_rss(struct rte_flow_expand_rss *buf, size_t size,
> +		    const struct rte_flow_item *pat, uint64_t types,

pat => pattern

> +		    const struct rte_flow_expand_node nodes[],
> +		    const int node_entry_index);

No need for node_entry_index to be const.

> +
>  #ifdef __cplusplus
>  }
>  #endif
> -- 
> 2.18.0
> 

-- 
Adrien Mazarguil
6WIND