Re: [dpdk-dev] [RFC] add flow action context API

[Thomas Monjalon <thomas@monjalon.net>]

20/05/2020 11:18, Andrey Vesnovaty:
> This commit introduces extension of DPDK flow action API enabling
> modification of single rte_flow_action.
> 
> Motivation and example
> ===
> Adding or removing one or more queues to RSS actions cloned in multiple
> flow rules imposes per rule toll for current DPDK flow API; the scenario
> requires for each flow sharing cloned RSS action:
> - call `rte_flow_destroy()`
> - call `rte_flow_create()` with modified RSS action
> 
> In order to prevent the overhead of multiple RSS flow rules reconfiguration
> API for in-place flow action modification introduced in this commit.

It seems there is an usability improvement with this new API.
If I understand well, the main motivation is to improve the performance?
The PMD implementation should try to keep a shared object
to benefit of the performance improvement, right?


The existing rte_flow API functions are:
	rte_flow_validate()
	rte_flow_create()
	rte_flow_destroy()
	rte_flow_flush()
	rte_flow_query()
	rte_flow_isolate()
	rte_flow_get_aged_flows()

> +	# added in 20.08
> +	rte_flow_action_ctx_create;
> +	rte_flow_action_ctx_destoy;
> +	rte_flow_action_ctx_modify;
> +	rte_flow_action_ctx_query;

We had "create", "destroy", "query", but no "modify" capability.
The new API is adding 2 things in my opinion:
	- shared action object
	- "modify" capability (is "update" a better wording?)

About the wording, do we need "ctx"?
I feel rte_flow_action is a good enough prefix for this API,
and should be documented as a shared action object.
I think the word "object" is more meaningful than "context".
Am I missing something?


> +	/**
> +	 * Describes action context.
> +	 *
> +	 * Enables multiple rules reference the same action by id/ctx.
> +	 *
> +	 * No action specific struct here (void*) since it can be any
> +	 * action type.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_CTX,

Why do we need a new action type?


> @@ -101,6 +101,28 @@ struct rte_flow_ops {
> +	/** See rte_flow_action_ctx_destoy() */
> +	void *(*action_ctx_create)
> +		(struct rte_eth_dev *dev,
> +		const struct rte_flow_action *action,
> +		struct rte_flow_error *error);
> +	/** See rte_flow_action_ctx_create() */
> +	int (*action_ctx_destroy)
> +		(struct rte_eth_dev *dev,
> +		void *ctx,
> +		struct rte_flow_error *error);
> +	/** See rte_flow_action_ctx_modify() */
> +	int (*action_ctx_modify)
> +		(struct rte_eth_dev *dev,
> +		void *ctx,
> +		const void *action_conf,
> +		struct rte_flow_error *error);
> +	/** See rte_flow_action_ctx_query() */
> +	int (*action_ctx_query)
> +		(struct rte_eth_dev *dev,
> +		const void *ctx,
> +		void *data,
> +		struct rte_flow_error *error);

API functions are directly linked to PMD ops, it looks simple and good.