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

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

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

Change description
===
Provide an API to create single rte_flow_action context to point/reference
rte_flow_action object contents from multiple rte_flow_rule objects.
Actually the introduced API makes action object shared and modification
of such an action effects all the rules referencing the action via context
(see struct rte_flow_action_ctx).

Action context lifetime
---
Once action context created (see rte_flow_action_ctx_create()) it can be
safely reused for:
- new flow rule creation
- action configuration/state modification
  (see rte_flow_action_ctx_modify())
- action state query (see rte_flow_action_ctx_query())
Once rte_flow_action_ctx_destroy() called the destroyed action context
should not be used i.e. result of the usage undefined.

Action query
---
Provide separate API to query action shared by multiple flows via action
context detached from any specific flow. Taking a counter as an example:
query returns value virtually aggregated across all flow rules referencing
the counter object via action context.

PMD support
---
The support of introduced API is pure PMD specific design and
responsibility for each action type (see struct rte_flow_ops).

testpmd
===
In order to utilize introduced API testpmd cli may implement following
extension create/modify/destroy/query action context iaccordingly

flow action_ctx create {port_id} [index] {action}
flow action_ctx modify {port_id} {index} {action}
flow action_ctx destroy {port_id} {index}
flow action_ctx query {port_id} {index}

example
---
configure rss to queues 1 & 2

testpmd> flow action_ctx create 0 100 rss 1 2

create flow rule utilizing action context

testpmd> flow create 0 ingress \
    pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
  actions ctx 100 end / end

add 2 more queues

testpmd> flow action_ctx modify 0 100 rss 1 2 3 4

Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
---
 lib/librte_ethdev/rte_ethdev_version.map |   6 +
 lib/librte_ethdev/rte_flow.c             |  85 ++++++++++++++
 lib/librte_ethdev/rte_flow.h             | 135 ++++++++++++++++++++++-
 lib/librte_ethdev/rte_flow_driver.h      |  22 ++++
 4 files changed, 247 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 3f32fdecf..d005abc33 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -230,4 +230,10 @@ EXPERIMENTAL {
 
 	# added in 20.02
 	rte_flow_dev_dump;
+
+	# added in 20.08
+	rte_flow_action_ctx_create;
+	rte_flow_action_ctx_destoy;
+	rte_flow_action_ctx_modify;
+	rte_flow_action_ctx_query;
 };
diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index 885a7ff9a..b03de1aef 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -1231,3 +1231,88 @@ rte_flow_dev_dump(uint16_t port_id, FILE *file, struct rte_flow_error *error)
 				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 				  NULL, rte_strerror(ENOSYS));
 }
+
+void *
+rte_flow_action_ctx_create(uint16_t port_id,
+		const struct rte_flow_action *action,
+		struct rte_flow_error *error)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	void *ctx;
+	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+	if (unlikely(!ops))
+		return NULL;
+	if (likely(!!ops->action_ctx_create)) {
+		ctx = ops->action_ctx_create(dev, action, error);
+		if (ctx == NULL)
+			flow_err(port_id, -rte_errno, error);
+		return ctx;
+	}
+	rte_flow_error_set(error, ENOSYS, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+			   NULL, rte_strerror(ENOSYS));
+	return NULL;
+}
+
+int
+rte_flow_action_ctx_destoy(uint16_t port_id,
+		void *ctx,
+		struct rte_flow_error *error)
+{
+	(void)(port_id);
+	(void)(ctx);
+	(void)(error);
+
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+	if (unlikely(!ops))
+		return -rte_errno;
+	if (likely(!!ops->action_ctx_destroy))
+		return flow_err(port_id,
+				ops->action_ctx_destroy(dev, ctx, error),
+				error);
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
+}
+
+int
+rte_flow_action_ctx_modify(uint16_t port_id,
+		void *ctx,
+		const void *action_conf,
+		struct rte_flow_error *error)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+	if (unlikely(!ops))
+		return -rte_errno;
+	if (likely(!!ops->action_ctx_modify))
+		return flow_err(port_id, ops->action_ctx_modify(dev, ctx,
+				action_conf, error),
+			error);
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
+}
+
+int
+rte_flow_action_ctx_query(uint16_t port_id,
+	       const void *ctx,
+	       void *data,
+	       struct rte_flow_error *error)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+	if (unlikely(!ops))
+		return -rte_errno;
+	if (likely(!!ops->action_ctx_query))
+		return flow_err(port_id, ops->action_ctx_query(dev, ctx,
+				data, error),
+			error);
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
+}
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 5625dc491..e109a07c5 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -1643,7 +1643,8 @@ enum rte_flow_action_type {
 	/**
 	 * Enables counters for this flow rule.
 	 *
-	 * These counters can be retrieved and reset through rte_flow_query(),
+	 * These counters can be retrieved and reset through rte_flow_query() or
+	 * rte_flow_action_ctx_query() if the action referenced via context/id,
 	 * see struct rte_flow_query_count.
 	 *
 	 * See struct rte_flow_action_count.
@@ -2051,6 +2052,16 @@ enum rte_flow_action_type {
 	 * See struct rte_flow_action_set_dscp.
 	 */
 	RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP,
+
+	/**
+	 * 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,
 };
 
 /**
@@ -3224,6 +3235,128 @@ rte_flow_conv(enum rte_flow_conv_op op,
 	      const void *src,
 	      struct rte_flow_error *error);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Create the action context pointing to the action via id/ctx.
+ *
+ * @param[in] port_id
+ *    The port identifier of the Ethernet device.
+ * @param[in] action
+ *   Action to be pointed via id/ctx.
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. PMDs initialize this
+ *   structure in case of error only.
+ * @return
+ *   A valid handle in case of success, NULL otherwise and rte_errno is set
+ *   to one of the error codes defined:
+ *   - (ENOSYS) if underlying device does not support this functionality.
+ *   - (EIO) if underlying device is removed.
+ *   - (EINVAL) if *action* invalid.
+ *   - (ENOTSUP) if *action* valid but unsupported.
+ */
+__rte_experimental
+void *
+rte_flow_action_ctx_create(uint16_t port_id,
+		const struct rte_flow_action *action,
+		struct rte_flow_error *error);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Destroys the action pointed by action context.
+ *
+ * @param[in] port_id
+ *    The port identifier of the Ethernet device.
+ * @param[in] ctx
+ *   Describes id/ctx pinting to the action to be destroyed.
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. PMDs initialize this
+ *   structure in case of error only.
+ * @return
+ *   - (0) if success.
+ *   - (-ENOSYS) if underlying device does not support this functionality.
+ *   - (-EIO) if underlying device is removed.
+ *   - (-ENOENT) if action pointed by *ctx* was not found.
+ *   - (-ETOOMANYREFS) if action pointed by *ctx* still referenced by one or
+ *     more rules
+ *   rte_errno is also set.
+ */
+__rte_experimental
+int
+rte_flow_action_ctx_destoy(uint16_t port_id,
+		void *ctx,
+		struct rte_flow_error *error);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Modifies inplace the action configuration pointed by action context
+ * created via rte_flow_action_ctx_create().
+ *
+ * @param[in] port_id
+ *    The port identifier of the Ethernet device.
+ * @param[in] ctx
+ *   Action ctx pointing to the action to be modified.
+ * @param[in] action_conf
+ *   Action specification used to modify the action pointed by ctx.
+ *   action_conf should be of same type with the action pointed by ctx,
+ *   otherwise function behavior undefined.
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. PMDs initialize this
+ *   structure in case of error only.
+ * @return
+ *   - (0) if success.
+ *   - (-ENOSYS) if underlying device does not support this functionality.
+ *   - (-EIO) if underlying device is removed.
+ *   - (-EINVAL) if *action_conf* invalid.
+ *   - (-ENOTSUP) if *action_conf* valid but unsupported.
+ *   - (-ENOENT) if action pointed by *ctx* was not found.
+ *   rte_errno is also set.
+ */
+__rte_experimental
+int
+rte_flow_action_ctx_modify(uint16_t port_id,
+		void *ctx,
+		const void *action_conf,
+		struct rte_flow_error *error);
+
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Query an existing action referenced via id/context.
+ *
+ * This function allows retrieving action-specific data such as counters.
+ * Data is gathered by special action which may be present/referenced in
+ * more than one flow rule definition.
+ *
+ * \see RTE_FLOW_ACTION_TYPE_COUNT
+ *
+ * @param port_id
+ *   Port identifier of Ethernet device.
+ * @param[in] ctx
+ *   Action ctx pointing to the action to query.
+ * @param[in, out] data
+ *   Pointer to storage for the associated query data type.
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. PMDs initialize this
+ *   structure in case of error only.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+__rte_experimental
+int
+rte_flow_action_ctx_query(uint16_t port_id,
+	       const void *ctx,
+	       void *data,
+	       struct rte_flow_error *error);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_ethdev/rte_flow_driver.h b/lib/librte_ethdev/rte_flow_driver.h
index 51a9a57a0..3e9a08857 100644
--- a/lib/librte_ethdev/rte_flow_driver.h
+++ b/lib/librte_ethdev/rte_flow_driver.h
@@ -101,6 +101,28 @@ struct rte_flow_ops {
 		(struct rte_eth_dev *dev,
 		 FILE *file,
 		 struct rte_flow_error *error);
+	/** 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);
 };
 
 /**
-- 
2.26.2

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

[Thomas Monjalon]

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.

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

[Jerin Jacob]

On Wed, May 20, 2020 at 2:48 PM Andrey Vesnovaty
<andrey.vesnovaty@gmail.com> wrote:
>
> 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.
>
> Change description
> ===
> Provide an API to create single rte_flow_action context to point/reference
> rte_flow_action object contents from multiple rte_flow_rule objects.
> Actually the introduced API makes action object shared and modification
> of such an action effects all the rules referencing the action via context
> (see struct rte_flow_action_ctx).
>
> Action context lifetime
> ---
> Once action context created (see rte_flow_action_ctx_create()) it can be
> safely reused for:
> - new flow rule creation
> - action configuration/state modification
>   (see rte_flow_action_ctx_modify())
> - action state query (see rte_flow_action_ctx_query())
> Once rte_flow_action_ctx_destroy() called the destroyed action context
> should not be used i.e. result of the usage undefined.

Motivation makes sense to me. It will be an improvement.

But creating shared objects adds additional complexity to "PMD and
application" as it
needs to manage the state. Since rte_flow_action already expressed in
vendor natural format,  What will be the benefit of a shared context?

Would the following additional API suffice the motivation?

rte_flow_modify_action(struct rte_flow * flow,  const struct
rte_flow_action actions[])

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

[Andrey Vesnovaty]

On Wed, Jun 3, 2020 at 1:02 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 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?
>
> Right, the goal is performance improvement.
Single API call modifies behaviour of multiple flows.

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

Naming is one of the most challenging parts of this RFC.
Some similarity I have found in existing code is
rte_mtr_policer_actions_update()
Is there any existing code having update/modify semantics?

>
> 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?
>
> CTX comes for the fact that each flow_rule doesn't have an ownership for
the given action but operates inside some context (shared action context
actually).
As mentioned above, naming is one of the most challenging parts of this
RFC.

>
> > +     /**
> > +      * 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?
>
> Because it's not an action itself but a reference/handle to it.

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

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

[Andrey Vesnovaty]

On Wed, Jun 3, 2020 at 1:53 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:

> On Wed, May 20, 2020 at 2:48 PM Andrey Vesnovaty
> <andrey.vesnovaty@gmail.com> wrote:
> >
> > 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.
> >
> > Change description
> > ===
> > Provide an API to create single rte_flow_action context to
> point/reference
> > rte_flow_action object contents from multiple rte_flow_rule objects.
> > Actually the introduced API makes action object shared and modification
> > of such an action effects all the rules referencing the action via
> context
> > (see struct rte_flow_action_ctx).
> >
> > Action context lifetime
> > ---
> > Once action context created (see rte_flow_action_ctx_create()) it can be
> > safely reused for:
> > - new flow rule creation
> > - action configuration/state modification
> >   (see rte_flow_action_ctx_modify())
> > - action state query (see rte_flow_action_ctx_query())
> > Once rte_flow_action_ctx_destroy() called the destroyed action context
> > should not be used i.e. result of the usage undefined.
>
> Motivation makes sense to me. It will be an improvement.
>
> But creating shared objects adds additional complexity to "PMD and
> application" as it
> needs to manage the state. Since rte_flow_action already expressed in
> vendor natural format,  What will be the benefit of a shared context?
>
> The benefit (or goal of the proposed API extension) is to modify the
behaviour of multiple flows by single API call.


> Would the following additional API suffice the motivation?
>
> rte_flow_modify_action(struct rte_flow * flow,  const struct
> rte_flow_action actions[])
>

This API limits the scope to single flow which isn't the goal for the
proposed change.

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

[Jerin Jacob]

I would suggest adding rte_flow driver implementers if there is a
change in rte_flow_ops in RFC so that
your patch will get enough. I added the maintainers of rte_flow PMD[1]
implementers in Cc.


>>
>> Would the following additional API suffice the motivation?
>>
>> rte_flow_modify_action(struct rte_flow * flow,  const struct
>> rte_flow_action actions[])
>
>
> This API limits the scope to single flow which isn't the goal for the proposed change.

Yes. But we need to find the balance between HW features(driver
interface) and public API?
Is Mellanox HW has the support for native shared HW ACTION context?
if so, it makes sense to have such a fat public API as you suggested.
If not, it is a matter of application to iterate over needed flows to
modify action through rte_flow_modify_action().

Assume Mellanox HW has native HW ACTION context and the majority of
the other HW[1] does not
have, then IMO, We should have common code, to handle in this complex
state machine
implementation of action_ctx_create, action_ctx_destroy,
rte_flow_action_ctx_modify, action_ctx_query
in case PMD does not support it. (If PMD/HW supports it then it can
use native implementation)

Reason:
1) I think, All the HW will have the the option to update the ACTION
for the given flow.
octeontx2 has it. If not, let's discuss what is typical HW abstraction
ACTION only update.
2) This case can be implemented if PMD just has flow_modify_action() support.
Multiple flows will be matter will be iterating over all registered flow.
3) Avoid code duplication on all of the below PMDs[1]



[1]
drivers/net/hinic/hinic_pmd_flow.c:const struct rte_flow_ops hinic_flow_ops = {
drivers/net/ipn3ke/ipn3ke_flow.h:extern const struct rte_flow_ops
ipn3ke_flow_ops;
drivers/net/i40e/i40e_flow.c:const struct rte_flow_ops i40e_flow_ops = {
drivers/net/qede/qede_filter.c:const struct rte_flow_ops qede_flow_ops = {
drivers/net/octeontx2/otx2_flow.h:extern const struct rte_flow_ops
otx2_flow_ops;
drivers/net/ice/ice_generic_flow.h:extern const struct rte_flow_ops
ice_flow_ops;
drivers/net/tap/tap_flow.c:static const struct rte_flow_ops tap_flow_ops = {
drivers/net/dpaa2/dpaa2_ethdev.h:extern const struct rte_flow_ops
dpaa2_flow_ops;
drivers/net/e1000/e1000_ethdev.h:extern const struct rte_flow_ops igb_flow_ops;
drivers/net/enic/enic_flow.c:const struct rte_flow_ops enic_flow_ops = {
drivers/net/bonding/rte_eth_bond_flow.c:const struct rte_flow_ops
bond_flow_ops = {
drivers/net/mlx5/mlx5_flow.c:static const struct rte_flow_ops mlx5_flow_ops = {
drivers/net/igc/igc_flow.h:extern const struct rte_flow_ops igc_flow_ops;
drivers/net/cxgbe/cxgbe_flow.c:static const struct rte_flow_ops
cxgbe_flow_ops = {
drivers/net/failsafe/failsafe_private.h:extern const struct
rte_flow_ops fs_flow_ops;
drivers/net/mvpp2/mrvl_flow.c:const struct rte_flow_ops mrvl_flow_ops = {
drivers/net/iavf/iavf_generic_flow.c:const struct rte_flow_ops iavf_flow_ops = {
drivers/net/hns3/hns3_flow.c:static const struct rte_flow_ops hns3_flow_ops = {
drivers/net/bnxt/bnxt_flow.c:const struct rte_flow_ops bnxt_flow_ops = {
drivers/net/mlx4/mlx4_flow.c:static const struct rte_flow_ops mlx4_flow_ops = {
drivers/net/sfc/sfc_flow.c:const struct rte_flow_ops sfc_flow_ops = {
drivers/net/softnic/rte_eth_softnic_flow.c:const struct rte_flow_ops
pmd_flow_ops = {
drivers/net/ixgbe/ixgbe_ethdev.h:extern const struct rte_flow_ops
ixgbe_flow_ops;

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

[Andrey Vesnovaty]

On Thu, Jun 4, 2020 at 3:37 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:

> I would suggest adding rte_flow driver implementers if there is a
> change in rte_flow_ops in RFC so that
> your patch will get enough. I added the maintainers of rte_flow PMD[1]
> implementers in Cc.
>
>
> >>
> >> Would the following additional API suffice the motivation?
> >>
> >> rte_flow_modify_action(struct rte_flow * flow,  const struct
> >> rte_flow_action actions[])
> >
> >
> > This API limits the scope to single flow which isn't the goal for the
> proposed change.
>
> Yes. But we need to find the balance between HW features(driver
> interface) and public API?
> Is Mellanox HW has the support for native shared HW ACTION context?
>

Yes, I'm working on a shared context for RSS action, patched will be
available in a couple of weeks.
Other candidates for this kind of API extension are counters/meters.


> if so, it makes sense to have such a fat public API as you suggested.
> If not, it is a matter of application to iterate over needed flows to
> modify action through rte_flow_modify_action().
>
> Assume Mellanox HW has native HW ACTION context and the majority of
> the other HW[1] does not
> have, then IMO, We should have common code, to handle in this complex
> state machine
> implementation of action_ctx_create, action_ctx_destroy,
> rte_flow_action_ctx_modify, action_ctx_query
> in case PMD does not support it. (If PMD/HW supports it then it can
> use native implementation)
>

Does it mean that all PMDs will support action-ctx create/destroy/update
for all types of actions?

Reason:
> 1) I think, All the HW will have the the option to update the ACTION
> for the given flow.
> octeontx2 has it. If not, let's discuss what is typical HW abstraction
> ACTION only update.
>


> 2) This case can be implemented if PMD just has flow_modify_action()
> support.
> Multiple flows will be matter will be iterating over all registered flow.
>

General case won't be just iteration over all flows but:
1. destroy flow
2. create "modified" action
3. create flow with the action from (2)
Is this what you mean by "common code" to handle action-ctx
create/destroy/update implementation?

>
> 3) Avoid code duplication on all of the below PMDs[1]
>
>
>
> [1]
> drivers/net/hinic/hinic_pmd_flow.c:const struct rte_flow_ops
> hinic_flow_ops = {
> drivers/net/ipn3ke/ipn3ke_flow.h:extern const struct rte_flow_ops
> ipn3ke_flow_ops;
> drivers/net/i40e/i40e_flow.c:const struct rte_flow_ops i40e_flow_ops = {
> drivers/net/qede/qede_filter.c:const struct rte_flow_ops qede_flow_ops = {
> drivers/net/octeontx2/otx2_flow.h:extern const struct rte_flow_ops
> otx2_flow_ops;
> drivers/net/ice/ice_generic_flow.h:extern const struct rte_flow_ops
> ice_flow_ops;
> drivers/net/tap/tap_flow.c:static const struct rte_flow_ops tap_flow_ops =
> {
> drivers/net/dpaa2/dpaa2_ethdev.h:extern const struct rte_flow_ops
> dpaa2_flow_ops;
> drivers/net/e1000/e1000_ethdev.h:extern const struct rte_flow_ops
> igb_flow_ops;
> drivers/net/enic/enic_flow.c:const struct rte_flow_ops enic_flow_ops = {
> drivers/net/bonding/rte_eth_bond_flow.c:const struct rte_flow_ops
> bond_flow_ops = {
> drivers/net/mlx5/mlx5_flow.c:static const struct rte_flow_ops
> mlx5_flow_ops = {
> drivers/net/igc/igc_flow.h:extern const struct rte_flow_ops igc_flow_ops;
> drivers/net/cxgbe/cxgbe_flow.c:static const struct rte_flow_ops
> cxgbe_flow_ops = {
> drivers/net/failsafe/failsafe_private.h:extern const struct
> rte_flow_ops fs_flow_ops;
> drivers/net/mvpp2/mrvl_flow.c:const struct rte_flow_ops mrvl_flow_ops = {
> drivers/net/iavf/iavf_generic_flow.c:const struct rte_flow_ops
> iavf_flow_ops = {
> drivers/net/hns3/hns3_flow.c:static const struct rte_flow_ops
> hns3_flow_ops = {
> drivers/net/bnxt/bnxt_flow.c:const struct rte_flow_ops bnxt_flow_ops = {
> drivers/net/mlx4/mlx4_flow.c:static const struct rte_flow_ops
> mlx4_flow_ops = {
> drivers/net/sfc/sfc_flow.c:const struct rte_flow_ops sfc_flow_ops = {
> drivers/net/softnic/rte_eth_softnic_flow.c:const struct rte_flow_ops
> pmd_flow_ops = {
> drivers/net/ixgbe/ixgbe_ethdev.h:extern const struct rte_flow_ops
> ixgbe_flow_ops;
>

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

[Thomas Monjalon]

04/06/2020 13:12, Andrey Vesnovaty:
> Thomas Monjalon <thomas@monjalon.net> wrote:
> > 20/05/2020 11:18, Andrey Vesnovaty:
> > 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?)
> 
> Naming is one of the most challenging parts of this RFC.
> Some similarity I have found in existing code is
> rte_mtr_policer_actions_update()
> Is there any existing code having update/modify semantics?

Except one callback in librte_fib, no DPDK API has "modify" in its name.
You can find the word "update" in the API of multiple DPDK libs.
I would like having the opinion of a native english speaker here.


> > 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?
> 
> CTX comes for the fact that each flow_rule doesn't have an ownership for
> the given action but operates inside some context (shared action context
> actually).
> As mentioned above, naming is one of the most challenging parts of this
> RFC.

I think we can replace "context" with "object" in the explanations.
The functions could be named without "ctx":
	- rte_flow_action_create
	- rte_flow_action_destroy
	- rte_flow_action_update
	- rte_flow_action_query


> > > +     /**
> > > +      * 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?
> >
> Because it's not an action itself but a reference/handle to it.

Sorry I don't understand when RTE_FLOW_ACTION_TYPE_CTX has to be used.
There is no mention of RTE_FLOW_ACTION_TYPE_CTX in the explanations.

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

[Bruce Richardson]

On Thu, Jun 04, 2020 at 07:23:04PM +0200, Thomas Monjalon wrote:
> 04/06/2020 13:12, Andrey Vesnovaty:
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> > > 20/05/2020 11:18, Andrey Vesnovaty:
> > > 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?)
> > 
> > Naming is one of the most challenging parts of this RFC.
> > Some similarity I have found in existing code is
> > rte_mtr_policer_actions_update()
> > Is there any existing code having update/modify semantics?
> 
> Except one callback in librte_fib, no DPDK API has "modify" in its name.
> You can find the word "update" in the API of multiple DPDK libs.
> I would like having the opinion of a native english speaker here.
> 
From a language viewpoint either is fine for conveying what the API does.
In this case "update" would seem to be better for consistency reasons.

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

[Thomas Monjalon]

05/06/2020 10:30, Bruce Richardson:
> On Thu, Jun 04, 2020 at 07:23:04PM +0200, Thomas Monjalon wrote:
> > 04/06/2020 13:12, Andrey Vesnovaty:
> > > Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > 20/05/2020 11:18, Andrey Vesnovaty:
> > > > 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?)
> > > 
> > > Naming is one of the most challenging parts of this RFC.
> > > Some similarity I have found in existing code is
> > > rte_mtr_policer_actions_update()
> > > Is there any existing code having update/modify semantics?
> > 
> > Except one callback in librte_fib, no DPDK API has "modify" in its name.
> > You can find the word "update" in the API of multiple DPDK libs.
> > I would like having the opinion of a native english speaker here.
> > 
> From a language viewpoint either is fine for conveying what the API does.
> In this case "update" would seem to be better for consistency reasons.

OK, thank you Bruce

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

[Jerin Jacob]

On Thu, Jun 4, 2020 at 9:27 PM Andrey Vesnovaty
<andrey.vesnovaty@gmail.com> wrote:
>
> On Thu, Jun 4, 2020 at 3:37 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>
>> I would suggest adding rte_flow driver implementers if there is a
>> change in rte_flow_ops in RFC so that
>> your patch will get enough. I added the maintainers of rte_flow PMD[1]
>> implementers in Cc.
>>
>>
>> >>
>> >> Would the following additional API suffice the motivation?
>> >>
>> >> rte_flow_modify_action(struct rte_flow * flow,  const struct
>> >> rte_flow_action actions[])
>> >
>> >
>> > This API limits the scope to single flow which isn't the goal for the proposed change.
>>
>> Yes. But we need to find the balance between HW features(driver
>> interface) and public API?
>> Is Mellanox HW has the support for native shared HW ACTION context?
>
>
> Yes, I'm working on a shared context for RSS action, patched will be available in a couple of weeks.
> Other candidates for this kind of API extension are counters/meters.

OK.

>
>>
>> if so, it makes sense to have such a fat public API as you suggested.
>> If not, it is a matter of application to iterate over needed flows to
>> modify action through rte_flow_modify_action().
>>
>> Assume Mellanox HW has native HW ACTION context and the majority of
>> the other HW[1] does not
>> have, then IMO, We should have common code, to handle in this complex
>> state machine
>> implementation of action_ctx_create, action_ctx_destroy,
>> rte_flow_action_ctx_modify, action_ctx_query
>> in case PMD does not support it. (If PMD/HW supports it then it can
>> use native implementation)
>
>
> Does it mean that all PMDs will support action-ctx create/destroy/update for all types of actions?

That can be based on following _driver_ op interface return:
flow_modify_action(struct rte_flow * flow,  const struct
rte_flow_action actions[])

>
>> Reason:
>> 1) I think, All the HW will have the the option to update the ACTION
>> for the given flow.
>> octeontx2 has it. If not, let's discuss what is typical HW abstraction
>> ACTION only update.
>
>
>>
>> 2) This case can be implemented if PMD just has flow_modify_action() support.
>> Multiple flows will be matter will be iterating over all registered flow.
>
>
> General case won't be just iteration over all flows but:
> 1. destroy flow
> 2. create "modified" action
> 3. create flow with the action from (2)
> Is this what you mean by "common code" to handle action-ctx create/destroy/update implementation?

Yes. Not sure why to destroy the flow if the only action is getting updated.
(2) and (3) driver op can be just implemented over,
flow_modify_action(struct rte_flow * flow,  const struct
rte_flow_action actions[])


>
>>
>> 3) Avoid code duplication on all of the below PMDs[1]
>>
>>
>>
>> [1]
>> drivers/net/hinic/hinic_pmd_flow.c:const struct rte_flow_ops hinic_flow_ops = {
>> drivers/net/ipn3ke/ipn3ke_flow.h:extern const struct rte_flow_ops
>> ipn3ke_flow_ops;
>> drivers/net/i40e/i40e_flow.c:const struct rte_flow_ops i40e_flow_ops = {
>> drivers/net/qede/qede_filter.c:const struct rte_flow_ops qede_flow_ops = {
>> drivers/net/octeontx2/otx2_flow.h:extern const struct rte_flow_ops
>> otx2_flow_ops;
>> drivers/net/ice/ice_generic_flow.h:extern const struct rte_flow_ops
>> ice_flow_ops;
>> drivers/net/tap/tap_flow.c:static const struct rte_flow_ops tap_flow_ops = {
>> drivers/net/dpaa2/dpaa2_ethdev.h:extern const struct rte_flow_ops
>> dpaa2_flow_ops;
>> drivers/net/e1000/e1000_ethdev.h:extern const struct rte_flow_ops igb_flow_ops;
>> drivers/net/enic/enic_flow.c:const struct rte_flow_ops enic_flow_ops = {
>> drivers/net/bonding/rte_eth_bond_flow.c:const struct rte_flow_ops
>> bond_flow_ops = {
>> drivers/net/mlx5/mlx5_flow.c:static const struct rte_flow_ops mlx5_flow_ops = {
>> drivers/net/igc/igc_flow.h:extern const struct rte_flow_ops igc_flow_ops;
>> drivers/net/cxgbe/cxgbe_flow.c:static const struct rte_flow_ops
>> cxgbe_flow_ops = {
>> drivers/net/failsafe/failsafe_private.h:extern const struct
>> rte_flow_ops fs_flow_ops;
>> drivers/net/mvpp2/mrvl_flow.c:const struct rte_flow_ops mrvl_flow_ops = {
>> drivers/net/iavf/iavf_generic_flow.c:const struct rte_flow_ops iavf_flow_ops = {
>> drivers/net/hns3/hns3_flow.c:static const struct rte_flow_ops hns3_flow_ops = {
>> drivers/net/bnxt/bnxt_flow.c:const struct rte_flow_ops bnxt_flow_ops = {
>> drivers/net/mlx4/mlx4_flow.c:static const struct rte_flow_ops mlx4_flow_ops = {
>> drivers/net/sfc/sfc_flow.c:const struct rte_flow_ops sfc_flow_ops = {
>> drivers/net/softnic/rte_eth_softnic_flow.c:const struct rte_flow_ops
>> pmd_flow_ops = {
>> drivers/net/ixgbe/ixgbe_ethdev.h:extern const struct rte_flow_ops
>> ixgbe_flow_ops;

[dpdk-dev] [RFC v2 0/1] add flow action context API

[Andrey Vesnovaty]

Hi, and thanks a lot for your RFC v1 comments. 

RFC v2 emphasize the intent for sharing the flow action:
* The term 'action context' was unclear and replaced with
   'shared action'.
* RFC v2 subject became 'add flow shared action API'.
* all proposed APIs renamed according the above.

The new shared action is an independent entity decoupled from any flow
while any flow can reuse such an action. Please go over the RFC
description, it was almost entirely rewritten.

@Jerin Jacob:
Thanks again for your comments, it made me admit that v1 description was
incomplete & unclear.  I hope v2 will be better at least in terms of
clarity.
@Thomas Monjalon:
rte_flow_action_ctx_modify() -> rte_flow_action_ctx_modify()

Looking forward to your responses on v2,
thanks in advance.

Andrey Vesnovaty (1):
  add flow shared action API

 lib/librte_ethdev/rte_ethdev_version.map |   6 +
 lib/librte_ethdev/rte_flow.c             |  81 +++++++++++++
 lib/librte_ethdev/rte_flow.h             | 143 ++++++++++++++++++++++-
 lib/librte_ethdev/rte_flow_driver.h      |  22 ++++
 4 files changed, 251 insertions(+), 1 deletion(-)

-- 
2.26.2

[dpdk-dev] [RFC v2 1/1] add flow shared action API

[Andrey Vesnovaty]

This commit introduces extension of DPDK flow action API enabling
sharing of single rte_flow_action in multiple flows. The API intended for
PMDs where multiple HW offloaded flows can reuse the same HW
essence/object representing flow action and modification of such an
essence/object effects all the rules using it.

Motivation and example
===
Adding or removing one or more queues to RSS used by 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

API for sharing action and its in-place update benefits:
- reduce the overhead of multiple RSS flow rules reconfiguration .
- optimize resource utilization by sharing action across of of multiple
  flows

Change description
===

Shared action
---
In order to represent flow action shared by multiple flows new action
type RTE_FLOW_ACTION_TYPE_SHARED introduced (see `enum
rte_flow_action_type`).
Actually the introduced API decouples action from any specific flow and
enables sharing of single action by its handle for multiple flows.

Shared action create/use/destroy
---
Shared action may be reused by some or none flow rules at any given
moment, IOW shared action reside outside of the context of any flow.
Shared action represent HW resources/objects used for action offloading
implementation. For allocation/release of all HW resources and all
related initializations/cleanups in PMD space required for shared action
implementation added new API
rte_flow_shared_action_create()/rte_flow_shared_action_destroy().
In addition to the above all preparations needed to maintain shared
access to the action resources, configuration and state should be done in
rte_flow_shared_action_create().

In order to share some flow action reuse the handle of type
`rte_flow_shared_action_t` returned by rte_flow_shared_action_create()
as a `conf` field of `struct rte_flow_action` (see "example" section).

If some shared action not used by any flow rule all resources allocated
by the shared action can be released by rte_flow_shared_action_destroy()
(see "example" section). The shared action handle passed as argument to
destroy API should not be used i.e. result of the usage is undefined.

Shared action re-configuration
---
Shared action behavior defined by its configuration & and can be updated
via rte_flow_shared_action_update() (see "example" section). The shared
action update operation modifies HW related resources/objects allocated
by the action. The number of operations performed by the update operation
should not be dependent on number of flows sharing the related action.
On return of shared action updated API action behavior should be
according to updated configuration for all flows sharing the action.

Shared action query
---
Provide separate API to query shared action sate (see
rte_flow_shared_action_update()). Taking a counter as an example: query
returns value aggregating all counter increments across all flow rules
sharing the counter.

PMD support
---
The support of introduced API is pure PMD specific design and
responsibility for each action type (see struct rte_flow_ops).

testpmd
===
In order to utilize introduced API testpmd cli may implement following
extension
create/update/destroy/query shared action accordingly

flow shared_action create {port_id} [index] {action}
flow shared_action update {port_id} {index} {action}
flow shared_action destroy {port_id} {index}
flow shared_action query {port_id} {index}

testpmd example
---
configure rss to queues 1 & 2

testpmd> flow shared_action create 0 100 rss 1 2

create flow rule utilizing shared action

testpmd> flow create 0 ingress \
    pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
  actions shared 100 end / end

add 2 more queues

testpmd> flow shared_action modify 0 100 rss 1 2 3 4

example
===

struct rte_flow_action actions[2];
struct rte_flow_action action;
/* skipped: initialize action */
rte_flow_shared_action_t handle = rte_flow_shared_action_create(port_id,
                                    &action, &error);
actions[0].type = RTE_FLOW_ACTION_TYPE_SHARED;
actions[0].conf = handle;
actions[1].type = RTE_FLOW_ACTION_TYPE_END;
/* skipped: init attr0 & pattern0 args */
struct rte_flow *flow0 = rte_flow_create(port_id, &attr0, pattern0,
					actions, error);
/* create more rules reusing shared action */
struct rte_flow *flow1 = rte_flow_create(port_id, &attr1, pattern1,
					actions, error);
/* skipped: for flows 2 till N */
struct rte_flow *flowN = rte_flow_create(port_id, &attrN, patternN,
					actions, error);
/* update shared action */
struct rte_flow_action updated_action;
/*
 * skipped: initialize updated_action according to desired action
 * configuration change
 */
rte_flow_shared_action_update(port_id, handle, updated_action.conf,
				error);
/*
 * from now on all flows 1 till N will act according to configuration of
 * updated_action
 */
/* skipped: destroy all flows 1 till N */
rte_flow_shared_action_destroy(port_id, handle, error);

Signed-off-by: Andrey Vesnovaty <andreyv@mellanox.com>
---
 lib/librte_ethdev/rte_ethdev_version.map |   6 +
 lib/librte_ethdev/rte_flow.c             |  81 +++++++++++++
 lib/librte_ethdev/rte_flow.h             | 143 ++++++++++++++++++++++-
 lib/librte_ethdev/rte_flow_driver.h      |  22 ++++
 4 files changed, 251 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 3f32fdecf..e291c2bd9 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -230,4 +230,10 @@ EXPERIMENTAL {
 
 	# added in 20.02
 	rte_flow_dev_dump;
+
+	# added in 20.08
+	rte_flow_shared_action_create;
+	rte_flow_shared_action_destoy;
+	rte_flow_shared_action_update;
+	rte_flow_shared_action_query;
 };
diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index 885a7ff9a..9ea55c0b4 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -1231,3 +1231,84 @@ rte_flow_dev_dump(uint16_t port_id, FILE *file, struct rte_flow_error *error)
 				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
 				  NULL, rte_strerror(ENOSYS));
 }
+
+void *
+rte_flow_shared_action_create(uint16_t port_id,
+		const struct rte_flow_action *action,
+		struct rte_flow_error *error)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	void *ctx;
+	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+	if (unlikely(!ops))
+		return NULL;
+	if (likely(!!ops->shared_action_create)) {
+		ctx = ops->shared_action_create(dev, action, error);
+		if (ctx == NULL)
+			flow_err(port_id, -rte_errno, error);
+		return ctx;
+	}
+	rte_flow_error_set(error, ENOSYS, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+			   NULL, rte_strerror(ENOSYS));
+	return NULL;
+}
+
+int
+rte_flow_shared_action_destoy(uint16_t port_id,
+		void *ctx,
+		struct rte_flow_error *error)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+	if (unlikely(!ops))
+		return -rte_errno;
+	if (likely(!!ops->shared_action_destroy))
+		return flow_err(port_id,
+				ops->shared_action_destroy(dev, ctx, error),
+				error);
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
+}
+
+int
+rte_flow_shared_action_update(uint16_t port_id,
+		void *ctx,
+		const void *action_conf,
+		struct rte_flow_error *error)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+	if (unlikely(!ops))
+		return -rte_errno;
+	if (likely(!!ops->shared_action_update))
+		return flow_err(port_id, ops->shared_action_update(dev, ctx,
+				action_conf, error),
+			error);
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
+}
+
+int
+rte_flow_shared_action_query(uint16_t port_id,
+	       const rte_flow_shared_action_t action,
+	       void *data,
+	       struct rte_flow_error *error)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+	if (unlikely(!ops))
+		return -rte_errno;
+	if (likely(!!ops->shared_action_query))
+		return flow_err(port_id, ops->shared_action_query(dev, action,
+				data, error),
+			error);
+	return rte_flow_error_set(error, ENOSYS,
+				  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
+				  NULL, rte_strerror(ENOSYS));
+}
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 5625dc491..742e0d8f0 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -1643,7 +1643,8 @@ enum rte_flow_action_type {
 	/**
 	 * Enables counters for this flow rule.
 	 *
-	 * These counters can be retrieved and reset through rte_flow_query(),
+	 * These counters can be retrieved and reset through rte_flow_query() or
+	 * rte_flow_shared_action_query() if the action provided via handle,
 	 * see struct rte_flow_query_count.
 	 *
 	 * See struct rte_flow_action_count.
@@ -2051,6 +2052,14 @@ enum rte_flow_action_type {
 	 * See struct rte_flow_action_set_dscp.
 	 */
 	RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP,
+
+	/**
+	 * Describes action shared a cross multiple flow rules.
+	 *
+	 * Enables multiple rules reference the same action by handle (see
+	 * rte_flow_shared_action_t).
+	 */
+	RTE_FLOW_ACTION_TYPE_SHARED,
 };
 
 /**
@@ -2593,6 +2602,14 @@ struct rte_flow_action_set_dscp {
 	uint8_t dscp;
 };
 
+
+/**
+ * RTE_FLOW_ACTION_TYPE_SHARED
+ *
+ * Describes handle for action shared a cross multiple flow rules.
+ */
+typedef void *rte_flow_shared_action_t;
+
 /* Mbuf dynamic field offset for metadata. */
 extern int rte_flow_dynf_metadata_offs;
 
@@ -3224,6 +3241,130 @@ rte_flow_conv(enum rte_flow_conv_op op,
 	      const void *src,
 	      struct rte_flow_error *error);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Create shared action for reuse in multiple flow rules.
+ *
+ * @param[in] port_id
+ *    The port identifier of the Ethernet device.
+ * @param[in] action
+ *   Action configuration for shared action creation.
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. PMDs initialize this
+ *   structure in case of error only.
+ * @return
+ *   A valid handle in case of success, NULL otherwise and rte_errno is set
+ *   to one of the error codes defined:
+ *   - (ENOSYS) if underlying device does not support this functionality.
+ *   - (EIO) if underlying device is removed.
+ *   - (EINVAL) if *action* invalid.
+ *   - (ENOTSUP) if *action* valid but unsupported.
+ */
+__rte_experimental
+rte_flow_shared_action_t
+rte_flow_shared_action_create(uint16_t port_id,
+		const struct rte_flow_action *action,
+		struct rte_flow_error *error);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Destroys the shared action by handle.
+ *
+ * @param[in] port_id
+ *    The port identifier of the Ethernet device.
+ * @param[in] action
+ *   Handle for the shared action to be destroyed.
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. PMDs initialize this
+ *   structure in case of error only.
+ * @return
+ *   - (0) if success.
+ *   - (-ENOSYS) if underlying device does not support this functionality.
+ *   - (-EIO) if underlying device is removed.
+ *   - (-ENOENT) if action pointed by *action* handle was not found.
+ *   - (-ETOOMANYREFS) if action pointed by *action* handle still used by one or
+ *     more rules
+ *   rte_errno is also set.
+ */
+__rte_experimental
+int
+rte_flow_shared_action_destoy(uint16_t port_id,
+		rte_flow_shared_action_t action,
+		struct rte_flow_error *error);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Updates inplace the shared action configuration pointed by *action* handle
+ * with the configuration provided as *action_conf* argument.
+ * The update of the shared action configuration effects all flow rules reusing
+ * the action via handle.
+ *
+ * @param[in] port_id
+ *    The port identifier of the Ethernet device.
+ * @param[in] action
+ *   Handle for the shared action to be updated.
+ * @param[in] action_conf
+ *   Action specification used to modify the action pointed by handle.
+ *   action_conf should be of same type with the action pointed by the *action*
+ *   handle argument, otherwise function behavior undefined.
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. PMDs initialize this
+ *   structure in case of error only.
+ * @return
+ *   - (0) if success.
+ *   - (-ENOSYS) if underlying device does not support this functionality.
+ *   - (-EIO) if underlying device is removed.
+ *   - (-EINVAL) if *action_conf* invalid.
+ *   - (-ENOTSUP) if *action_conf* valid but unsupported.
+ *   - (-ENOENT) if action pointed by *ctx* was not found.
+ *   rte_errno is also set.
+ */
+__rte_experimental
+int
+rte_flow_shared_action_update(uint16_t port_id,
+		rte_flow_shared_action_t action,
+		const void *action_conf,
+		struct rte_flow_error *error);
+
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Query the shared action by handle.
+ *
+ * This function allows retrieving action-specific data such as counters.
+ * Data is gathered by special action which may be present/referenced in
+ * more than one flow rule definition.
+ *
+ * \see RTE_FLOW_ACTION_TYPE_COUNT
+ *
+ * @param port_id
+ *   Port identifier of Ethernet device.
+ * @param[in] action
+ *   Handle for the shared action to query.
+ * @param[in, out] data
+ *   Pointer to storage for the associated query data type.
+ * @param[out] error
+ *   Perform verbose error reporting if not NULL. PMDs initialize this
+ *   structure in case of error only.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+__rte_experimental
+int
+rte_flow_shared_action_query(uint16_t port_id,
+	       const rte_flow_shared_action_t action,
+	       void *data,
+	       struct rte_flow_error *error);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_ethdev/rte_flow_driver.h b/lib/librte_ethdev/rte_flow_driver.h
index 51a9a57a0..e25c6c072 100644
--- a/lib/librte_ethdev/rte_flow_driver.h
+++ b/lib/librte_ethdev/rte_flow_driver.h
@@ -101,6 +101,28 @@ struct rte_flow_ops {
 		(struct rte_eth_dev *dev,
 		 FILE *file,
 		 struct rte_flow_error *error);
+	/** See rte_flow_shared_action_create() */
+	rte_flow_shared_action_t (*shared_action_create)
+		(struct rte_eth_dev *dev,
+		const struct rte_flow_action *action,
+		struct rte_flow_error *error);
+	/** See rte_flow_shared_action_destroy() */
+	int (*shared_action_destroy)
+		(struct rte_eth_dev *dev,
+		void *ctx,
+		struct rte_flow_error *error);
+	/** See rte_flow_shared_action_update() */
+	int (*shared_action_update)
+		(struct rte_eth_dev *dev,
+		void *ctx,
+		const void *action_conf,
+		struct rte_flow_error *error);
+	/** See rte_flow_shared_action_query() */
+	int (*shared_action_query)
+		(struct rte_eth_dev *dev,
+		const void *ctx,
+		void *data,
+		struct rte_flow_error *error);
 };
 
 /**
-- 
2.26.2

Re: [dpdk-dev] [RFC v2 0/1] add flow action context API

[Thomas Monjalon]

20/06/2020 15:32, Andrey Vesnovaty:
> Hi, and thanks a lot for your RFC v1 comments. 
> 
> RFC v2 emphasize the intent for sharing the flow action:
> * The term 'action context' was unclear and replaced with
>    'shared action'.
> * RFC v2 subject became 'add flow shared action API'.
> * all proposed APIs renamed according the above.
> 
> The new shared action is an independent entity decoupled from any flow
> while any flow can reuse such an action. Please go over the RFC
> description, it was almost entirely rewritten.
> 
> @Jerin Jacob:
> Thanks again for your comments, it made me admit that v1 description was
> incomplete & unclear.  I hope v2 will be better at least in terms of
> clarity.
> @Thomas Monjalon:
> rte_flow_action_ctx_modify() -> rte_flow_action_ctx_modify()

I guess it is a typo.
I see the name "rte_flow_shared_action_update" in the patch

Re: [dpdk-dev] [RFC v2 0/1] add flow action context API

[Andrey Vesnovaty]

On Mon, Jun 22, 2020 at 6:22 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 20/06/2020 15:32, Andrey Vesnovaty:
> > Hi, and thanks a lot for your RFC v1 comments.
> >
> > RFC v2 emphasize the intent for sharing the flow action:
> > * The term 'action context' was unclear and replaced with
> >    'shared action'.
> > * RFC v2 subject became 'add flow shared action API'.
> > * all proposed APIs renamed according the above.
> >
> > The new shared action is an independent entity decoupled from any flow
> > while any flow can reuse such an action. Please go over the RFC
> > description, it was almost entirely rewritten.
> >
> > @Jerin Jacob:
> > Thanks again for your comments, it made me admit that v1 description was
> > incomplete & unclear.  I hope v2 will be better at least in terms of
> > clarity.
> > @Thomas Monjalon:
> > rte_flow_action_ctx_modify() -> rte_flow_action_ctx_modify()
>
> I guess it is a typo.
> I see the name "rte_flow_shared_action_update" in the patch
>
> Right, a typo. Should be:
rte_flow_action_ctx_modify() ->  rte_flow_shared_action_update ()

Re: [dpdk-dev] [RFC v2 0/1] add flow action context API

[Jerin Jacob]

On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty
<andrey.vesnovaty@gmail.com> wrote:
>
> Hi, and thanks a lot for your RFC v1 comments.
>
> RFC v2 emphasize the intent for sharing the flow action:
> * The term 'action context' was unclear and replaced with
>    'shared action'.
> * RFC v2 subject became 'add flow shared action API'.
> * all proposed APIs renamed according the above.
>
> The new shared action is an independent entity decoupled from any flow
> while any flow can reuse such an action. Please go over the RFC
> description, it was almost entirely rewritten.
>
> @Jerin Jacob:
> Thanks again for your comments, it made me admit that v1 description was
> incomplete & unclear.  I hope v2 will be better at least in terms of
> clarity.

The public API and its usage is very clear. Thanks for this RFC.

I think, RFC v2 still not addressing the concern raised in the
http://mails.dpdk.org/archives/dev/2020-June/169296.html.

Since MLX hardware has an HW based shared object it is fine to have
public API based on that level of abstraction.
But at the PMD driver level we need to choose the correct abstraction
to support all PMD and support shared object scheme if possible.

I purpose to introduce something below or similar
            int (*action_update)
                (struct rte_eth_dev *,
                  struct rte_flow *flow,
                 const struct rte_flow_action [],
                 struct rte_flow_error *);

in addition to: shared_action_create, shared_action_destroy,
shared_action_update, shared_action_query

Have generic implementation of above, if action_update callback is not
NULL. So that, it can work all PMDs and to
avoid the duplication of "complex" shared session management code.

Re: [dpdk-dev] [RFC v2 0/1] add flow action context API

[Andrey Vesnovaty]

Hi

On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:

> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty
> <andrey.vesnovaty@gmail.com> wrote:
> >
> > Hi, and thanks a lot for your RFC v1 comments.
> >
> > RFC v2 emphasize the intent for sharing the flow action:
> > * The term 'action context' was unclear and replaced with
> >    'shared action'.
> > * RFC v2 subject became 'add flow shared action API'.
> > * all proposed APIs renamed according the above.
> >
> > The new shared action is an independent entity decoupled from any flow
> > while any flow can reuse such an action. Please go over the RFC
> > description, it was almost entirely rewritten.
> >
> > @Jerin Jacob:
> > Thanks again for your comments, it made me admit that v1 description was
> > incomplete & unclear.  I hope v2 will be better at least in terms of
> > clarity.
>
> The public API and its usage is very clear. Thanks for this RFC.


My pleasure.

>
> I think, RFC v2 still not addressing the concern raised in the
> http://mails.dpdk.org/archives/dev/2020-June/169296.html.
>
> Since MLX hardware has an HW based shared object it is fine to have
> public API based on that level of abstraction.
> But at the PMD driver level we need to choose the correct abstraction
> to support all PMD and support shared object scheme if possible.
>
> I purpose to introduce something below or similar
>             int (*action_update)
>                 (struct rte_eth_dev *,
>                   struct rte_flow *flow,
>                  const struct rte_flow_action [],
>                  struct rte_flow_error *);
>
Where this callback suppose to belong (struct rte_flow_ops)?
How should it be implemented by PMD?
Is it about shared action and if "yes" why there is 'flow' argument?

>
> in addition to: shared_action_create, shared_action_destroy,
> shared_action_update, shared_action_query
>
> Have generic implementation of above, if action_update callback is not
> NULL.

"is not NULL" -> "is NULL"?


> So that, it can work all PMDs and to
> avoid the duplication of "complex" shared session management code.
>
Do you mean shared action in use by multiple flows by "shared session"?

Re: [dpdk-dev] [RFC v2 0/1] add flow action context API

[Jerin Jacob]

On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty
<andrey.vesnovaty@gmail.com> wrote:
>
> Hi
>
> On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>
>> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty
>> <andrey.vesnovaty@gmail.com> wrote:
>> >
>> > Hi, and thanks a lot for your RFC v1 comments.
>> >
>> > RFC v2 emphasize the intent for sharing the flow action:
>> > * The term 'action context' was unclear and replaced with
>> >    'shared action'.
>> > * RFC v2 subject became 'add flow shared action API'.
>> > * all proposed APIs renamed according the above.
>> >
>> > The new shared action is an independent entity decoupled from any flow
>> > while any flow can reuse such an action. Please go over the RFC
>> > description, it was almost entirely rewritten.
>> >
>> > @Jerin Jacob:
>> > Thanks again for your comments, it made me admit that v1 description was
>> > incomplete & unclear.  I hope v2 will be better at least in terms of
>> > clarity.
>>
>> The public API and its usage is very clear. Thanks for this RFC.
>
>
> My pleasure.
>>
>>
>> I think, RFC v2 still not addressing the concern raised in the
>> http://mails.dpdk.org/archives/dev/2020-June/169296.html.
>>
>> Since MLX hardware has an HW based shared object it is fine to have
>> public API based on that level of abstraction.
>> But at the PMD driver level we need to choose the correct abstraction
>> to support all PMD and support shared object scheme if possible.
>>
>> I purpose to introduce something below or similar
>>             int (*action_update)
>>                 (struct rte_eth_dev *,
>>                   struct rte_flow *flow,
>>                  const struct rte_flow_action [],
>>                  struct rte_flow_error *);
>
> Where this callback suppose to belong (struct rte_flow_ops)?

Yes.

> How should it be implemented by PMD?

See below,

> Is it about shared action and if "yes" why there is 'flow' argument?

flow holds the "pattern" and "action" data as PMD specific handle.
So PMD, implementation can just change that action if it gets the PMD
specific handle.


>>
>>
>> in addition to: shared_action_create, shared_action_destroy,
>> shared_action_update, shared_action_query
>>
>> Have generic implementation of above, if action_update callback is not
>> NULL.
>
> "is not NULL" -> "is NULL"?

Yes. When it is NULL.

>
>>
>> So that, it can work all PMDs and to
>> avoid the duplication of "complex" shared session management code.
>
> Do you mean shared action in use by multiple flows by "shared session"?

Yes.

Re: [dpdk-dev] [RFC v2 0/1] add flow action context API

[Andrey Vesnovaty]

On Sun, Jun 28, 2020 at 4:42 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:

> On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty
> <andrey.vesnovaty@gmail.com> wrote:
> >
> > Hi
> >
> > On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com>
> wrote:
> >>
> >> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty
> >> <andrey.vesnovaty@gmail.com> wrote:
> >> >
> >> > Hi, and thanks a lot for your RFC v1 comments.
> >> >
> >> > RFC v2 emphasize the intent for sharing the flow action:
> >> > * The term 'action context' was unclear and replaced with
> >> >    'shared action'.
> >> > * RFC v2 subject became 'add flow shared action API'.
> >> > * all proposed APIs renamed according the above.
> >> >
> >> > The new shared action is an independent entity decoupled from any flow
> >> > while any flow can reuse such an action. Please go over the RFC
> >> > description, it was almost entirely rewritten.
> >> >
> >> > @Jerin Jacob:
> >> > Thanks again for your comments, it made me admit that v1 description
> was
> >> > incomplete & unclear.  I hope v2 will be better at least in terms of
> >> > clarity.
> >>
> >> The public API and its usage is very clear. Thanks for this RFC.
> >
> >
> > My pleasure.
> >>
> >>
> >> I think, RFC v2 still not addressing the concern raised in the
> >> http://mails.dpdk.org/archives/dev/2020-June/169296.html.
> >>
> >> Since MLX hardware has an HW based shared object it is fine to have
> >> public API based on that level of abstraction.
> >> But at the PMD driver level we need to choose the correct abstraction
> >> to support all PMD and support shared object scheme if possible.
> >>
> >> I purpose to introduce something below or similar
> >>             int (*action_update)
> >>                 (struct rte_eth_dev *,
> >>                   struct rte_flow *flow,
> >>                  const struct rte_flow_action [],
> >>                  struct rte_flow_error *);
> >
> > Where this callback suppose to belong (struct rte_flow_ops)?
>
> Yes.
>
> > How should it be implemented by PMD?
>
> See below,
>
> > Is it about shared action and if "yes" why there is 'flow' argument?
>
> flow holds the "pattern" and "action" data as PMD specific handle.
> So PMD, implementation can just change that action if it gets the PMD
> specific handle.
>
>
> >>
> >>
> >> in addition to: shared_action_create, shared_action_destroy,
> >> shared_action_update, shared_action_query
> >>
> >> Have generic implementation of above, if action_update callback is not
> >> NULL.
> >
> > "is not NULL" -> "is NULL"?
>
> Yes. When it is NULL.


Jerin, few clarifications regarding generic implementation of shared action:
Based on this conversation I'm assuming that generic implementation
supposed to be something like:
For each flow using some shared action:
call ops-> action_update()
If the assumption above correct:
1. taking into account that shared_action_update() is atomic, how can this
deal with partial success: some flows may fail validation - should it:
  1.1.lock all flows
  1.2.validate all flows
  1.3.update all flows
  1.4. unlock
2. action_update callback is PMD specific & if it's unsupported there is no
support for shared action any way
Please address the issues above

>
> >>
> >> So that, it can work all PMDs and to
> >> avoid the duplication of "complex" shared session management code.
> >
> > Do you mean shared action in use by multiple flows by "shared session"?
>
> Yes.
>
Common 'shared session' management code:
- can be reduced to atomic usage counter
- maintaining list of flow using shared action expected to impact
performance & not necessary for all PMD specific implementations
Access to other shared resources hard to generalize because:
- for some PMDs mutual exclusion is HW feature & no need to protect it in SW
- for others there may be multiple resources & access to each one protected
by different mechanism

An observation related to action_update callback:
If replaced (updated) action was shared then the flow won't be influenced
any more by updates or removed shared action.

Re: [dpdk-dev] [RFC v2 0/1] add flow action context API

[Jerin Jacob]

On Mon, Jun 29, 2020 at 3:52 PM Andrey Vesnovaty
<andrey.vesnovaty@gmail.com> wrote:
>
>
>
> On Sun, Jun 28, 2020 at 4:42 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>
>> On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty
>> <andrey.vesnovaty@gmail.com> wrote:
>> >
>> > Hi
>> >
>> > On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>> >>
>> >> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty
>> >> <andrey.vesnovaty@gmail.com> wrote:
>> >> >
>> >> > Hi, and thanks a lot for your RFC v1 comments.
>> >> >
>> >> > RFC v2 emphasize the intent for sharing the flow action:
>> >> > * The term 'action context' was unclear and replaced with
>> >> >    'shared action'.
>> >> > * RFC v2 subject became 'add flow shared action API'.
>> >> > * all proposed APIs renamed according the above.
>> >> >
>> >> > The new shared action is an independent entity decoupled from any flow
>> >> > while any flow can reuse such an action. Please go over the RFC
>> >> > description, it was almost entirely rewritten.
>> >> >
>> >> > @Jerin Jacob:
>> >> > Thanks again for your comments, it made me admit that v1 description was
>> >> > incomplete & unclear.  I hope v2 will be better at least in terms of
>> >> > clarity.
>> >>
>> >> The public API and its usage is very clear. Thanks for this RFC.
>> >
>> >
>> > My pleasure.
>> >>
>> >>
>> >> I think, RFC v2 still not addressing the concern raised in the
>> >> http://mails.dpdk.org/archives/dev/2020-June/169296.html.
>> >>
>> >> Since MLX hardware has an HW based shared object it is fine to have
>> >> public API based on that level of abstraction.
>> >> But at the PMD driver level we need to choose the correct abstraction
>> >> to support all PMD and support shared object scheme if possible.
>> >>
>> >> I purpose to introduce something below or similar
>> >>             int (*action_update)
>> >>                 (struct rte_eth_dev *,
>> >>                   struct rte_flow *flow,
>> >>                  const struct rte_flow_action [],
>> >>                  struct rte_flow_error *);
>> >
>> > Where this callback suppose to belong (struct rte_flow_ops)?
>>
>> Yes.
>>
>> > How should it be implemented by PMD?
>>
>> See below,
>>
>> > Is it about shared action and if "yes" why there is 'flow' argument?
>>
>> flow holds the "pattern" and "action" data as PMD specific handle.
>> So PMD, implementation can just change that action if it gets the PMD
>> specific handle.
>>
>>
>> >>
>> >>
>> >> in addition to: shared_action_create, shared_action_destroy,
>> >> shared_action_update, shared_action_query
>> >>
>> >> Have generic implementation of above, if action_update callback is not
>> >> NULL.
>> >
>> > "is not NULL" -> "is NULL"?
>>
>> Yes. When it is NULL.
>
>
> Jerin, few clarifications regarding generic implementation of shared action:
> Based on this conversation I'm assuming that generic implementation supposed to be something like:
> For each flow using some shared action:
> call ops-> action_update()
> If the assumption above correct:
> 1. taking into account that shared_action_update() is atomic, how can this deal with partial success: some flows may fail validation - should it:
>   1.1.lock all flows
>   1.2.validate all flows
>   1.3.update all flows
>   1.4. unlock

Yes.

> 2. action_update callback is PMD specific & if it's unsupported there is no support for shared action any way

Yes.

> Please address the issues above

>
>> >
>> >>
>> >> So that, it can work all PMDs and to
>> >> avoid the duplication of "complex" shared session management code.
>> >
>> > Do you mean shared action in use by multiple flows by "shared session"?
>>
>> Yes.
>
> Common 'shared session' management code:
> - can be reduced to atomic usage counter
> - maintaining list of flow using shared action expected to impact performance & not necessary for all PMD specific implementations
> Access to other shared resources hard to generalize because:
> - for some PMDs mutual exclusion is HW feature & no need to protect it in SW
> - for others there may be multiple resources & access to each one protected by different mechanism

The general callback you can assume, it supports only action_update
based callback.
If PMD has mutual exclusion HW feature then it can override the
function pointers.



> An observation related to action_update callback:
> If replaced (updated) action was shared then the flow won't be influenced any more by updates or removed shared action.

Re: [dpdk-dev] [RFC v2 0/1] add flow action context API

[Andrey Vesnovaty]

On Tue, Jun 30, 2020 at 12:52 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:

> On Mon, Jun 29, 2020 at 3:52 PM Andrey Vesnovaty
> <andrey.vesnovaty@gmail.com> wrote:
> >
> >
> >
> > On Sun, Jun 28, 2020 at 4:42 PM Jerin Jacob <jerinjacobk@gmail.com>
> wrote:
> >>
> >> On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty
> >> <andrey.vesnovaty@gmail.com> wrote:
> >> >
> >> > Hi
> >> >
> >> > On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com>
> wrote:
> >> >>
> >> >> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty
> >> >> <andrey.vesnovaty@gmail.com> wrote:
> >> >> >
> >> >> > Hi, and thanks a lot for your RFC v1 comments.
> >> >> >
> >> >> > RFC v2 emphasize the intent for sharing the flow action:
> >> >> > * The term 'action context' was unclear and replaced with
> >> >> >    'shared action'.
> >> >> > * RFC v2 subject became 'add flow shared action API'.
> >> >> > * all proposed APIs renamed according the above.
> >> >> >
> >> >> > The new shared action is an independent entity decoupled from any
> flow
> >> >> > while any flow can reuse such an action. Please go over the RFC
> >> >> > description, it was almost entirely rewritten.
> >> >> >
> >> >> > @Jerin Jacob:
> >> >> > Thanks again for your comments, it made me admit that v1
> description was
> >> >> > incomplete & unclear.  I hope v2 will be better at least in terms
> of
> >> >> > clarity.
> >> >>
> >> >> The public API and its usage is very clear. Thanks for this RFC.
> >> >
> >> >
> >> > My pleasure.
> >> >>
> >> >>
> >> >> I think, RFC v2 still not addressing the concern raised in the
> >> >> http://mails.dpdk.org/archives/dev/2020-June/169296.html.
> >> >>
> >> >> Since MLX hardware has an HW based shared object it is fine to have
> >> >> public API based on that level of abstraction.
> >> >> But at the PMD driver level we need to choose the correct abstraction
> >> >> to support all PMD and support shared object scheme if possible.
> >> >>
> >> >> I purpose to introduce something below or similar
> >> >>             int (*action_update)
> >> >>                 (struct rte_eth_dev *,
> >> >>                   struct rte_flow *flow,
> >> >>                  const struct rte_flow_action [],
> >> >>                  struct rte_flow_error *);
> >> >
> >> > Where this callback suppose to belong (struct rte_flow_ops)?
> >>
> >> Yes.
> >>
> >> > How should it be implemented by PMD?
> >>
> >> See below,
> >>
> >> > Is it about shared action and if "yes" why there is 'flow' argument?
> >>
> >> flow holds the "pattern" and "action" data as PMD specific handle.
> >> So PMD, implementation can just change that action if it gets the PMD
> >> specific handle.
> >>
> >>
> >> >>
> >> >>
> >> >> in addition to: shared_action_create, shared_action_destroy,
> >> >> shared_action_update, shared_action_query
> >> >>
> >> >> Have generic implementation of above, if action_update callback is
> not
> >> >> NULL.
> >> >
> >> > "is not NULL" -> "is NULL"?
> >>
> >> Yes. When it is NULL.
> >
> >
> > Jerin, few clarifications regarding generic implementation of shared
> action:
> > Based on this conversation I'm assuming that generic implementation
> supposed to be something like:
> > For each flow using some shared action:
> > call ops-> action_update()
> > If the assumption above correct:
> > 1. taking into account that shared_action_update() is atomic, how can
> this deal with partial success: some flows may fail validation - should it:
> >   1.1.lock all flows
> >   1.2.validate all flows
> >   1.3.update all flows
> >   1.4. unlock
>
> Yes.
>
This kind of locking in addition to shared session management requires
locking of each flow_create/flow_destroy in addition to action_uodate
callback implementation even if there are no shared actions at all. In
other words it imposes an overhead on all PMDs that don't support shared
action natively.

>
> > 2. action_update callback is PMD specific & if it's unsupported there is
> no support for shared action any way
>
> Yes.
>
> > Please address the issues above
>
> >
> >> >
> >> >>
> >> >> So that, it can work all PMDs and to
> >> >> avoid the duplication of "complex" shared session management code.
> >> >
> >> > Do you mean shared action in use by multiple flows by "shared
> session"?
> >>
> >> Yes.
> >
> > Common 'shared session' management code:
> > - can be reduced to atomic usage counter
> > - maintaining list of flow using shared action expected to impact
> performance & not necessary for all PMD specific implementations
> > Access to other shared resources hard to generalize because:
> > - for some PMDs mutual exclusion is HW feature & no need to protect it
> in SW
> > - for others there may be multiple resources & access to each one
> protected by different mechanism
>
> The general callback you can assume, it supports only action_update
> based callback.
> If PMD has mutual exclusion HW feature then it can override the
> function pointers.
>
>
>
> > An observation related to action_update callback:
> > If replaced (updated) action was shared then the flow won't be
> influenced any more by updates or removed shared action.
>

Re: [dpdk-dev] [RFC v2 0/1] add flow action context API

[Jerin Jacob]

On Wed, Jul 1, 2020 at 2:54 PM Andrey Vesnovaty
<andrey.vesnovaty@gmail.com> wrote:
>
>
> On Tue, Jun 30, 2020 at 12:52 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>
>> On Mon, Jun 29, 2020 at 3:52 PM Andrey Vesnovaty
>> <andrey.vesnovaty@gmail.com> wrote:
>> >
>> >
>> >
>> > On Sun, Jun 28, 2020 at 4:42 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>> >>
>> >> On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty
>> >> <andrey.vesnovaty@gmail.com> wrote:
>> >> >
>> >> > Hi
>> >> >
>> >> > On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>> >> >>
>> >> >> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty
>> >> >> <andrey.vesnovaty@gmail.com> wrote:
>> >> >> >
>> >> >> > Hi, and thanks a lot for your RFC v1 comments.
>> >> >> >
>> >> >> > RFC v2 emphasize the intent for sharing the flow action:
>> >> >> > * The term 'action context' was unclear and replaced with
>> >> >> >    'shared action'.
>> >> >> > * RFC v2 subject became 'add flow shared action API'.
>> >> >> > * all proposed APIs renamed according the above.
>> >> >> >
>> >> >> > The new shared action is an independent entity decoupled from any flow
>> >> >> > while any flow can reuse such an action. Please go over the RFC
>> >> >> > description, it was almost entirely rewritten.
>> >> >> >
>> >> >> > @Jerin Jacob:
>> >> >> > Thanks again for your comments, it made me admit that v1 description was
>> >> >> > incomplete & unclear.  I hope v2 will be better at least in terms of
>> >> >> > clarity.
>> >> >>
>> >> >> The public API and its usage is very clear. Thanks for this RFC.
>> >> >
>> >> >
>> >> > My pleasure.
>> >> >>
>> >> >>
>> >> >> I think, RFC v2 still not addressing the concern raised in the
>> >> >> http://mails.dpdk.org/archives/dev/2020-June/169296.html.
>> >> >>
>> >> >> Since MLX hardware has an HW based shared object it is fine to have
>> >> >> public API based on that level of abstraction.
>> >> >> But at the PMD driver level we need to choose the correct abstraction
>> >> >> to support all PMD and support shared object scheme if possible.
>> >> >>
>> >> >> I purpose to introduce something below or similar
>> >> >>             int (*action_update)
>> >> >>                 (struct rte_eth_dev *,
>> >> >>                   struct rte_flow *flow,
>> >> >>                  const struct rte_flow_action [],
>> >> >>                  struct rte_flow_error *);
>> >> >
>> >> > Where this callback suppose to belong (struct rte_flow_ops)?
>> >>
>> >> Yes.
>> >>
>> >> > How should it be implemented by PMD?
>> >>
>> >> See below,
>> >>
>> >> > Is it about shared action and if "yes" why there is 'flow' argument?
>> >>
>> >> flow holds the "pattern" and "action" data as PMD specific handle.
>> >> So PMD, implementation can just change that action if it gets the PMD
>> >> specific handle.
>> >>
>> >>
>> >> >>
>> >> >>
>> >> >> in addition to: shared_action_create, shared_action_destroy,
>> >> >> shared_action_update, shared_action_query
>> >> >>
>> >> >> Have generic implementation of above, if action_update callback is not
>> >> >> NULL.
>> >> >
>> >> > "is not NULL" -> "is NULL"?
>> >>
>> >> Yes. When it is NULL.
>> >
>> >
>> > Jerin, few clarifications regarding generic implementation of shared action:
>> > Based on this conversation I'm assuming that generic implementation supposed to be something like:
>> > For each flow using some shared action:
>> > call ops-> action_update()
>> > If the assumption above correct:
>> > 1. taking into account that shared_action_update() is atomic, how can this deal with partial success: some flows may fail validation - should it:
>> >   1.1.lock all flows
>> >   1.2.validate all flows
>> >   1.3.update all flows
>> >   1.4. unlock
>>
>> Yes.
>
> This kind of locking in addition to shared session management requires locking of each flow_create/flow_destroy in addition to action_uodate callback implementation even if there are no shared actions at all. In other words it imposes an overhead on all PMDs that don't support shared action natively.

Yes. That's what my concern with implementing shared session if the
PMD only supports only action update for the given rte_flow *.
Another approach would be to introduce rte_flow_action_update() public
API which can either take
"const struct rte_flow_action []" OR shared context ID, to cater to
both cases or something on similar lines.




>>
>>
>> > 2. action_update callback is PMD specific & if it's unsupported there is no support for shared action any way
>>
>> Yes.
>>
>> > Please address the issues above
>>
>> >
>> >> >
>> >> >>
>> >> >> So that, it can work all PMDs and to
>> >> >> avoid the duplication of "complex" shared session management code.
>> >> >
>> >> > Do you mean shared action in use by multiple flows by "shared session"?
>> >>
>> >> Yes.
>> >
>> > Common 'shared session' management code:
>> > - can be reduced to atomic usage counter
>> > - maintaining list of flow using shared action expected to impact performance & not necessary for all PMD specific implementations
>> > Access to other shared resources hard to generalize because:
>> > - for some PMDs mutual exclusion is HW feature & no need to protect it in SW
>> > - for others there may be multiple resources & access to each one protected by different mechanism
>>
>> The general callback you can assume, it supports only action_update
>> based callback.
>> If PMD has mutual exclusion HW feature then it can override the
>> function pointers.
>>
>>
>>
>> > An observation related to action_update callback:
>> > If replaced (updated) action was shared then the flow won't be influenced any more by updates or removed shared action.

Re: [dpdk-dev] [RFC v2 1/1] add flow shared action API

[Stephen Hemminger]

On Sat, 20 Jun 2020 16:32:57 +0300
Andrey Vesnovaty <andrey.vesnovaty@gmail.com> wrote:

> +
> +void *
> +rte_flow_shared_action_create(uint16_t port_id,
> +		const struct rte_flow_action *action,
> +		struct rte_flow_error *error)
> +{

NAK

API's that return void * (opaque pointer) are dangerous and should
not be added to DPDK.

To do data hiding. Define a structure but don't expose the internals
of what that structure are.

Re: [dpdk-dev] [RFC v2 1/1] add flow shared action API

[Ori Kam]

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, July 2, 2020 3:24 AM
> To: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> Ori Kam <orika@mellanox.com>; dev@dpdk.org; Andrey Vesnovaty
> <andreyv@mellanox.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/1] add flow shared action API
> 
> On Sat, 20 Jun 2020 16:32:57 +0300
> Andrey Vesnovaty <andrey.vesnovaty@gmail.com> wrote:
> 
> > +
> > +void *
> > +rte_flow_shared_action_create(uint16_t port_id,
> > +		const struct rte_flow_action *action,
> > +		struct rte_flow_error *error)
> > +{
> 
> NAK
> 
> API's that return void * (opaque pointer) are dangerous and should
> not be added to DPDK.
> 
> To do data hiding. Define a structure but don't expose the internals
> of what that structure are.

I agree with you it is better not to use void *
So I suggest to use new struct rte_action_ctx or something like this. That 
will be implemented differently for each driver just like rte_flow.
What do you think?

Best,
Ori

Re: [dpdk-dev] [RFC v2 1/1] add flow shared action API

[Andrey Vesnovaty]

On Thu, Jul 2, 2020 at 10:20 AM Ori Kam <orika@mellanox.com> wrote:

> Hi Stephen,
>
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Thursday, July 2, 2020 3:24 AM
> > To: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
> > Cc: Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> > Ori Kam <orika@mellanox.com>; dev@dpdk.org; Andrey Vesnovaty
> > <andreyv@mellanox.com>
> > Subject: Re: [dpdk-dev] [RFC v2 1/1] add flow shared action API
> >
> > On Sat, 20 Jun 2020 16:32:57 +0300
> > Andrey Vesnovaty <andrey.vesnovaty@gmail.com> wrote:
> >
> > > +
> > > +void *
> > > +rte_flow_shared_action_create(uint16_t port_id,
> > > +           const struct rte_flow_action *action,
> > > +           struct rte_flow_error *error)
> > > +{
> >
> > NAK
> >
> > API's that return void * (opaque pointer) are dangerous and should
> > not be added to DPDK.
> >
> > To do data hiding. Define a structure but don't expose the internals
> > of what that structure are.
>
> I'll add `struct rte_flow_shared_action` to upcoming patches. Thanks.


> I agree with you it is better not to use void *
> So I suggest to use new struct rte_action_ctx or something like this. That
> will be implemented differently for each driver just like rte_flow.
> What do you think?
>
> Best,
> Ori
>
>