[RFC,v2,1/1] add flow shared action API

Message ID 20200620133257.12441-1-andrey.vesnovaty@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series add flow action context API |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Andrey Vesnovaty June 20, 2020, 1:32 p.m. UTC
  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(-)
  

Comments

Stephen Hemminger July 2, 2020, 12:24 a.m. UTC | #1
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.
  
Ori Kam July 2, 2020, 7:20 a.m. UTC | #2
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
  
Andrey Vesnovaty July 2, 2020, 8:06 a.m. UTC | #3
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
>
>
  

Patch

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);
 };
 
 /**