add flow shared action API
Checks
Commit Message
From: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
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
`struct rte_flow_shared_action` 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 */
struct rte_flow_shared_action *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>
Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
Signed-off-by: Andrey Vesnovaty <andreyv@mellanox.com>
---
This patch based on RFC: https://patches.dpdk.org/patch/71820/
---
lib/librte_ethdev/rte_ethdev_version.map | 6 +
lib/librte_ethdev/rte_flow.c | 81 +++++++++++++
lib/librte_ethdev/rte_flow.h | 148 ++++++++++++++++++++++-
lib/librte_ethdev/rte_flow_driver.h | 22 ++++
4 files changed, 256 insertions(+), 1 deletion(-)
Comments
On Fri, Jul 3, 2020 at 8:07 PM Andrey Vesnovaty <andreyv@mellanox.com> wrote:
>
> From: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
>
> 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
> `struct rte_flow_shared_action` 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 */
> struct rte_flow_shared_action *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>
> Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
> Signed-off-by: Andrey Vesnovaty <andreyv@mellanox.com>
Duplicate Signoffs.
# Request to CC all the people who are already giving comments to this patch.
# The Following comment is not addressed in this patch.
http://mails.dpdk.org/archives/dev/2020-July/172408.html
> ---
> This patch based on RFC: https://patches.dpdk.org/patch/71820/
>
> ---
> lib/librte_ethdev/rte_ethdev_version.map | 6 +
> lib/librte_ethdev/rte_flow.c | 81 +++++++++++++
> lib/librte_ethdev/rte_flow.h | 148 ++++++++++++++++++++++-
> lib/librte_ethdev/rte_flow_driver.h | 22 ++++
> 4 files changed, 256 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..7728057c3 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));
> }
> +
> +struct rte_flow_shared_action *
> +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];
> + struct rte_flow_shared_action *shared_action;
> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> +
> + if (unlikely(!ops))
> + return NULL;
> + if (likely(!!ops->shared_action_create)) {
> + shared_action = ops->shared_action_create(dev, action, error);
> + if (shared_action == NULL)
> + flow_err(port_id, -rte_errno, error);
> + return shared_action;
> + }
> + 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,
> + struct rte_flow_shared_action *action,
> + 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, action, 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,
> + struct rte_flow_shared_action *action,
> + 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, action,
> + 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 struct rte_flow_shared_action *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..98140ebb1 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
> + * struct rte_flow_shared_action).
> + */
> + RTE_FLOW_ACTION_TYPE_SHARED,
> };
>
> /**
> @@ -2593,6 +2602,20 @@ struct rte_flow_action_set_dscp {
> uint8_t dscp;
> };
>
> +
> +/**
> + * RTE_FLOW_ACTION_TYPE_SHARED
> + *
> + * Opaque type returned after successfully creating a shared action.
> + *
> + * This handle can be used to manage and query the related action:
> + * - share it a cross multiple flow rules
> + * - update action configuration
> + * - query action data
> + * - destroy action
> + */
> +struct rte_flow_shared_action;
> +
> /* Mbuf dynamic field offset for metadata. */
> extern int rte_flow_dynf_metadata_offs;
>
> @@ -3224,6 +3247,129 @@ 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
> +struct rte_flow_shared_action *
> +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,
> + struct rte_flow_shared_action *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,
> + struct rte_flow_shared_action *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 struct rte_flow_shared_action *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..c103d159e 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() */
> + struct rte_flow_shared_action *(*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,
> + struct rte_flow_shared_action *shared_action,
> + struct rte_flow_error *error);
> + /** See rte_flow_shared_action_update() */
> + int (*shared_action_update)
> + (struct rte_eth_dev *dev,
> + struct rte_flow_shared_action *shared_action,
> + 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 struct rte_flow_shared_action *shared_action,
> + void *data,
> + struct rte_flow_error *error);
> };
>
> /**
> --
> 2.26.2
>
03/07/2020 17:02, Jerin Jacob:
> On Fri, Jul 3, 2020 at 8:07 PM Andrey Vesnovaty <andreyv@mellanox.com> wrote:
> > Signed-off-by: Andrey Vesnovaty <andreyv@mellanox.com>
> > Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
> > Signed-off-by: Andrey Vesnovaty <andreyv@mellanox.com>
>
> Duplicate Signoffs.
>
> # Request to CC all the people who are already giving comments to this patch.
Yes you are absolutely right Jerin.
It is said in the contribution docs, but we must repeat it again
and again, especially for newcomers like here.
The CC list is very important!
Please pay attention to Cc the relevant maintainers
AND the persons who already showed some interest.
It may be a blocker for merge, thanks for sharing with co-workers.
Hi, Jerin & Thomas.
On Fri, Jul 3, 2020 at 6:21 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 03/07/2020 17:02, Jerin Jacob:
> > On Fri, Jul 3, 2020 at 8:07 PM Andrey Vesnovaty <andreyv@mellanox.com>
> wrote:
> > > Signed-off-by: Andrey Vesnovaty <andreyv@mellanox.com>
> > > Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
> > > Signed-off-by: Andrey Vesnovaty <andreyv@mellanox.com>
> >
> > Duplicate Signoffs.
> >
> > # Request to CC all the people who are already giving comments to this
> patch.
>
> Yes you are absolutely right Jerin.
> It is said in the contribution docs, but we must repeat it again
> and again, especially for newcomers like here.
>
> The CC list is very important!
> Please pay attention to Cc the relevant maintainers
> AND the persons who already showed some interest.
>
Just to clarify my next steps:
- remove irrelevant signoffs.
- all those who already showed interest should be added to CC field
- send PATCH v2 in reply to this email.
Thanks & sorry for the mess.
>
> It may be a blocker for merge, thanks for sharing with co-workers.
>
>
>
Thanks,
Andrey Vesnovaty
(+972)*526775512* | *Skype:* andrey775512
On Fri, Jul 3, 2020 at 6:02 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> On Fri, Jul 3, 2020 at 8:07 PM Andrey Vesnovaty <andreyv@mellanox.com>
> wrote:
> >
> > From: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
> >
> > 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
> > `struct rte_flow_shared_action` 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 */
> > struct rte_flow_shared_action *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>
> > Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
> > Signed-off-by: Andrey Vesnovaty <andreyv@mellanox.com>
>
> Duplicate Signoffs.
>
> # Request to CC all the people who are already giving comments to this
> patch.
> # The Following comment is not addressed in this patch.
> http://mails.dpdk.org/archives/dev/2020-July/172408.html
I need to mention the locking issue once again.
If there is a need to maintain "shared session" in the generic rte_flow
layer all
calls to flow_create() with shared action & all delete need to take
sharedsession
management locks at least for verification. Lock partitioning is also bit
problematic
since one flow may have more than one shared action.
>
>
>
> > ---
> > This patch based on RFC: https://patches.dpdk.org/patch/71820/
> >
> > ---
> > lib/librte_ethdev/rte_ethdev_version.map | 6 +
> > lib/librte_ethdev/rte_flow.c | 81 +++++++++++++
> > lib/librte_ethdev/rte_flow.h | 148 ++++++++++++++++++++++-
> > lib/librte_ethdev/rte_flow_driver.h | 22 ++++
> > 4 files changed, 256 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..7728057c3 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));
> > }
> > +
> > +struct rte_flow_shared_action *
> > +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];
> > + struct rte_flow_shared_action *shared_action;
> > + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id,
> error);
> > +
> > + if (unlikely(!ops))
> > + return NULL;
> > + if (likely(!!ops->shared_action_create)) {
> > + shared_action = ops->shared_action_create(dev, action,
> error);
> > + if (shared_action == NULL)
> > + flow_err(port_id, -rte_errno, error);
> > + return shared_action;
> > + }
> > + 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,
> > + struct rte_flow_shared_action *action,
> > + 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, action,
> 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,
> > + struct rte_flow_shared_action *action,
> > + 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,
> action,
> > + 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 struct rte_flow_shared_action *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..98140ebb1 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
> > + * struct rte_flow_shared_action).
> > + */
> > + RTE_FLOW_ACTION_TYPE_SHARED,
> > };
> >
> > /**
> > @@ -2593,6 +2602,20 @@ struct rte_flow_action_set_dscp {
> > uint8_t dscp;
> > };
> >
> > +
> > +/**
> > + * RTE_FLOW_ACTION_TYPE_SHARED
> > + *
> > + * Opaque type returned after successfully creating a shared action.
> > + *
> > + * This handle can be used to manage and query the related action:
> > + * - share it a cross multiple flow rules
> > + * - update action configuration
> > + * - query action data
> > + * - destroy action
> > + */
> > +struct rte_flow_shared_action;
> > +
> > /* Mbuf dynamic field offset for metadata. */
> > extern int rte_flow_dynf_metadata_offs;
> >
> > @@ -3224,6 +3247,129 @@ 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
> > +struct rte_flow_shared_action *
> > +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,
> > + struct rte_flow_shared_action *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,
> > + struct rte_flow_shared_action *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 struct rte_flow_shared_action *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..c103d159e 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() */
> > + struct rte_flow_shared_action *(*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,
> > + struct rte_flow_shared_action *shared_action,
> > + struct rte_flow_error *error);
> > + /** See rte_flow_shared_action_update() */
> > + int (*shared_action_update)
> > + (struct rte_eth_dev *dev,
> > + struct rte_flow_shared_action *shared_action,
> > + 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 struct rte_flow_shared_action *shared_action,
> > + void *data,
> > + struct rte_flow_error *error);
> > };
> >
> > /**
> > --
> > 2.26.2
> >
>
On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
<andrey.vesnovaty@gmail.com> wrote:
>
>
> Thanks,
>
> Andrey Vesnovaty
> (+972)526775512 | Skype: andrey775512
>
>
> On Fri, Jul 3, 2020 at 6:02 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>
>> On Fri, Jul 3, 2020 at 8:07 PM Andrey Vesnovaty <andreyv@mellanox.com> wrote:
>> >
>> > From: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
>> >
>> > 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
>> > `struct rte_flow_shared_action` 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 */
>> > struct rte_flow_shared_action *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>
>> > Signed-off-by: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
>> > Signed-off-by: Andrey Vesnovaty <andreyv@mellanox.com>
>>
>> Duplicate Signoffs.
>>
>> # Request to CC all the people who are already giving comments to this patch.
>> # The Following comment is not addressed in this patch.
>> http://mails.dpdk.org/archives/dev/2020-July/172408.html
>
>
> I need to mention the locking issue once again.
> If there is a need to maintain "shared session" in the generic rte_flow layer all
> calls to flow_create() with shared action & all delete need to take sharedsession
> management locks at least for verification. Lock partitioning is also bit problematic
> since one flow may have more than one shared action.
Then, I think better 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. This would allow HW's
without have the shared context ID
to use the action update.
Hi Jerin,
PSB,
Thanks,
Ori
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Saturday, July 4, 2020 3:33 PM
> dpdk-dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>
> On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
> <andrey.vesnovaty@gmail.com> wrote:
> >
> >
> > Thanks,
> >
> > Andrey Vesnovaty
> > (+972)526775512 | Skype: andrey775512
> >
[..Nip ..]
> > I need to mention the locking issue once again.
> > If there is a need to maintain "shared session" in the generic rte_flow layer
> all
> > calls to flow_create() with shared action & all delete need to take
> sharedsession
> > management locks at least for verification. Lock partitioning is also bit
> problematic
> > since one flow may have more than one shared action.
>
> Then, I think better 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. This would allow HW's
> without have the shared context ID
> to use the action update.
Can you please explain your idea?
As I can see if we use the flow_action array it may result in bugs.
For example, the application created two flows with the same RSS (not using the context)
Then he wants to change one flow to use different RSS, but the result will that both flows
will be changed.
Also this will enforce the PMD to keep track on all flows which will have memory penalty for
some PMDs.
On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
>
> Hi Jerin,
> PSB,
>
> Thanks,
> Ori
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Saturday, July 4, 2020 3:33 PM
> > dpdk-dev <dev@dpdk.org>
> > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> >
> > On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
> > <andrey.vesnovaty@gmail.com> wrote:
> > >
> > >
> > > Thanks,
> > >
> > > Andrey Vesnovaty
> > > (+972)526775512 | Skype: andrey775512
> > >
>
>
> [..Nip ..]
>
> > > I need to mention the locking issue once again.
> > > If there is a need to maintain "shared session" in the generic rte_flow layer
> > all
> > > calls to flow_create() with shared action & all delete need to take
> > sharedsession
> > > management locks at least for verification. Lock partitioning is also bit
> > problematic
> > > since one flow may have more than one shared action.
> >
> > Then, I think better 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. This would allow HW's
> > without have the shared context ID
> > to use the action update.
>
> Can you please explain your idea?
I see two types of HW schemes supporting action updates without going
through call `rte_flow_destroy()` and call `rte_flow_create()`
- The shared HW action context feature
- The HW has "pattern" and "action" mapped to different HW objects and
action can be updated any time.
Other than above-mentioned RSS use case, another use case would be to
a) create rte_flow and set the action as DROP (Kind of reserving the HW object)
b) Update the action only when the rest of the requirements ready.
Any API schematic that supports both notions of HW is fine with me.
> As I can see if we use the flow_action array it may result in bugs.
> For example, the application created two flows with the same RSS (not using the context)
> Then he wants to change one flow to use different RSS, but the result will that both flows
> will be changed.
Sorry. I don't quite follow this.
> Also this will enforce the PMD to keep track on all flows which will have memory penalty for
> some PMDs.
Hi Jerin,
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Monday, July 6, 2020 12:00 PM
> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>
> On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
> >
> > Hi Jerin,
> > PSB,
> >
> > Thanks,
> > Ori
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Saturday, July 4, 2020 3:33 PM
> > > dpdk-dev <dev@dpdk.org>
> > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> > >
> > > On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
> > > <andrey.vesnovaty@gmail.com> wrote:
> > > >
> > > >
> > > > Thanks,
> > > >
> > > > Andrey Vesnovaty
> > > > (+972)526775512 | Skype: andrey775512
> > > >
> >
> >
> > [..Nip ..]
> >
> > > > I need to mention the locking issue once again.
> > > > If there is a need to maintain "shared session" in the generic rte_flow
> layer
> > > all
> > > > calls to flow_create() with shared action & all delete need to take
> > > sharedsession
> > > > management locks at least for verification. Lock partitioning is also bit
> > > problematic
> > > > since one flow may have more than one shared action.
> > >
> > > Then, I think better 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. This would allow HW's
> > > without have the shared context ID
> > > to use the action update.
> >
> > Can you please explain your idea?
>
> I see two types of HW schemes supporting action updates without going
> through call `rte_flow_destroy()` and call `rte_flow_create()`
> - The shared HW action context feature
> - The HW has "pattern" and "action" mapped to different HW objects and
> action can be updated any time.
> Other than above-mentioned RSS use case, another use case would be to
> a) create rte_flow and set the action as DROP (Kind of reserving the HW object)
> b) Update the action only when the rest of the requirements ready.
>
> Any API schematic that supports both notions of HW is fine with me.
>
I have an idea if the API will be changed to something like this,
Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx, rte_flow_action *action, error)
This will enable the application to send a different action than the original one to be switched.
Assuming the PMD supports this.
Does it answer your concerns?
>
> > As I can see if we use the flow_action array it may result in bugs.
> > For example, the application created two flows with the same RSS (not using
> the context)
> > Then he wants to change one flow to use different RSS, but the result will that
> both flows
> > will be changed.
>
> Sorry. I don't quite follow this.
>
I was trying to show that there must be some context. But I don’t think this is relevant to
your current ideas.
> > Also this will enforce the PMD to keep track on all flows which will have
> memory penalty for
> > some PMDs.
Best,
Ori
Hi, Jerin.
Please see below Ori's suggestion below to implement your
rte_flow_action_update() idea
with some API changes of rte_flow_shared_action_xxx API changes.
On Mon, Jul 6, 2020 at 3:28 PM Ori Kam <orika@mellanox.com> wrote:
> Hi Jerin,
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Monday, July 6, 2020 12:00 PM
> > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> >
> > On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
> > >
> > > Hi Jerin,
> > > PSB,
> > >
> > > Thanks,
> > > Ori
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > Sent: Saturday, July 4, 2020 3:33 PM
> > > > dpdk-dev <dev@dpdk.org>
> > > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> > > >
> > > > On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
> > > > <andrey.vesnovaty@gmail.com> wrote:
> > > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Andrey Vesnovaty
> > > > > (+972)526775512 | Skype: andrey775512
> > > > >
> > >
> > >
> > > [..Nip ..]
> > >
> > > > > I need to mention the locking issue once again.
> > > > > If there is a need to maintain "shared session" in the generic
> rte_flow
> > layer
> > > > all
> > > > > calls to flow_create() with shared action & all delete need to take
> > > > sharedsession
> > > > > management locks at least for verification. Lock partitioning is
> also bit
> > > > problematic
> > > > > since one flow may have more than one shared action.
> > > >
> > > > Then, I think better 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. This would allow HW's
> > > > without have the shared context ID
> > > > to use the action update.
> > >
> > > Can you please explain your idea?
> >
> > I see two types of HW schemes supporting action updates without going
> > through call `rte_flow_destroy()` and call `rte_flow_create()`
> > - The shared HW action context feature
> > - The HW has "pattern" and "action" mapped to different HW objects and
> > action can be updated any time.
> > Other than above-mentioned RSS use case, another use case would be to
> > a) create rte_flow and set the action as DROP (Kind of reserving the HW
> object)
> > b) Update the action only when the rest of the requirements ready.
> >
> > Any API schematic that supports both notions of HW is fine with me.
> >
> I have an idea if the API will be changed to something like this,
> Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> rte_flow_action *action, error)
> This will enable the application to send a different action than the
> original one to be switched.
> Assuming the PMD supports this.
> Does it answer your concerns?
>
This allows both:
1. Update action configuration
2. Replace action by some other action
For 2 pure software implementation may carate shred action (that can be
shared
with one flow only, depends on PMD) and later on
rte_flow_shared_action_update may replace this
action with some other action by handle returned from
rte_flow_shared_action_create
Doesign between 1 and 2 is per PMD.
> >
> > > As I can see if we use the flow_action array it may result in bugs.
> > > For example, the application created two flows with the same RSS (not
> using
> > the context)
> > > Then he wants to change one flow to use different RSS, but the result
> will that
> > both flows
> > > will be changed.
> >
> > Sorry. I don't quite follow this.
> >
> I was trying to show that there must be some context. But I don’t think
> this is relevant to
> your current ideas.
>
> > > Also this will enforce the PMD to keep track on all flows which will
> have
> > memory penalty for
> > > some PMDs.
>
> Best,
> Ori
>
Thanks,
Andrey
On Mon, Jul 6, 2020 at 7:02 PM Andrey Vesnovaty
<andrey.vesnovaty@gmail.com> wrote:
>
> Hi, Jerin.
Hi Ori and Andrey,
>
> Please see below Ori's suggestion below to implement your rte_flow_action_update() idea
> with some API changes of rte_flow_shared_action_xxx API changes.
>
> On Mon, Jul 6, 2020 at 3:28 PM Ori Kam <orika@mellanox.com> wrote:
>>
>> Hi Jerin,
>>
>> > -----Original Message-----
>> > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > Sent: Monday, July 6, 2020 12:00 PM
>> > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>> >
>> > On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
>> > >
>> > > Hi Jerin,
>> > > PSB,
>> > >
>> > > Thanks,
>> > > Ori
>> > >
>> > > > -----Original Message-----
>> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
>> > > > Sent: Saturday, July 4, 2020 3:33 PM
>> > > > dpdk-dev <dev@dpdk.org>
>> > > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>> > > >
>> > > > On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
>> > > > <andrey.vesnovaty@gmail.com> wrote:
>> > > > >
>> > > > >
>> > > > > Thanks,
>> > > > >
>> > > > > Andrey Vesnovaty
>> > > > > (+972)526775512 | Skype: andrey775512
>> > > > >
>> > >
>> > >
>> > > [..Nip ..]
>> > >
>> > > > > I need to mention the locking issue once again.
>> > > > > If there is a need to maintain "shared session" in the generic rte_flow
>> > layer
>> > > > all
>> > > > > calls to flow_create() with shared action & all delete need to take
>> > > > sharedsession
>> > > > > management locks at least for verification. Lock partitioning is also bit
>> > > > problematic
>> > > > > since one flow may have more than one shared action.
>> > > >
>> > > > Then, I think better 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. This would allow HW's
>> > > > without have the shared context ID
>> > > > to use the action update.
>> > >
>> > > Can you please explain your idea?
>> >
>> > I see two types of HW schemes supporting action updates without going
>> > through call `rte_flow_destroy()` and call `rte_flow_create()`
>> > - The shared HW action context feature
>> > - The HW has "pattern" and "action" mapped to different HW objects and
>> > action can be updated any time.
>> > Other than above-mentioned RSS use case, another use case would be to
>> > a) create rte_flow and set the action as DROP (Kind of reserving the HW object)
>> > b) Update the action only when the rest of the requirements ready.
>> >
>> > Any API schematic that supports both notions of HW is fine with me.
>> >
>> I have an idea if the API will be changed to something like this,
>> Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx, rte_flow_action *action, error)
>> This will enable the application to send a different action than the original one to be switched.
>> Assuming the PMD supports this.
>> Does it answer your concerns?
>
>
> This allows both:
> 1. Update action configuration
> 2. Replace action by some other action
> For 2 pure software implementation may carate shred action (that can be shared
> with one flow only, depends on PMD) and later on rte_flow_shared_action_update may replace this
> action with some other action by handle returned from rte_flow_shared_action_create
> Doesign between 1 and 2 is per PMD.
struct rte_flow * object holds the driver representation of the
pattern + action.
So in order to update the action, we would need struct rte_flow * in API.
I think, simple API change would be to accommodate "rte_shared_ctx
*ctx, rte_flow_action *action" modes
without introducing the emulation for one or other mode, will be.
enum rte_flow_action_update_type {
RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
};
struct rte_flow_action_update_type_param {
enum rte_flow_action_update_type type;
union {
struct rte_flow_action_update_type_shared_action_param {
rte_shared_ctx *ctx;
} shared_action;
struct rte_flow_action_update_type_shared_action_param {
rte_flow *flow,
rte_flow_action *action;
} action;
}
}
rte_flow_action_update(uint16_port port, struct
rte_flow_action_update_type_param *param, error)
>
>>
>> >
>> > > As I can see if we use the flow_action array it may result in bugs.
>> > > For example, the application created two flows with the same RSS (not using
>> > the context)
>> > > Then he wants to change one flow to use different RSS, but the result will that
>> > both flows
>> > > will be changed.
>> >
>> > Sorry. I don't quite follow this.
>> >
>> I was trying to show that there must be some context. But I don’t think this is relevant to
>> your current ideas.
>>
>> > > Also this will enforce the PMD to keep track on all flows which will have
>> > memory penalty for
>> > > some PMDs.
>>
>> Best,
>> Ori
>
>
> Thanks,
> Andrey
Hi Jerin,
Thanks you for your quick reply.
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>
> On Mon, Jul 6, 2020 at 7:02 PM Andrey Vesnovaty
> <andrey.vesnovaty@gmail.com> wrote:
> >
> > Hi, Jerin.
>
> Hi Ori and Andrey,
>
>
> >
> > Please see below Ori's suggestion below to implement your
> rte_flow_action_update() idea
> > with some API changes of rte_flow_shared_action_xxx API changes.
> >
> > On Mon, Jul 6, 2020 at 3:28 PM Ori Kam <orika@mellanox.com> wrote:
> >>
> >> Hi Jerin,
> >>
> >> > -----Original Message-----
> >> > From: Jerin Jacob <jerinjacobk@gmail.com>
> >> > Sent: Monday, July 6, 2020 12:00 PM
> >> > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> >> >
> >> > On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
> >> > >
> >> > > Hi Jerin,
> >> > > PSB,
> >> > >
> >> > > Thanks,
> >> > > Ori
> >> > >
> >> > > > -----Original Message-----
> >> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> >> > > > Sent: Saturday, July 4, 2020 3:33 PM
> >> > > > dpdk-dev <dev@dpdk.org>
> >> > > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> >> > > >
> >> > > > On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
> >> > > > <andrey.vesnovaty@gmail.com> wrote:
> >> > > > >
> >> > > > >
> >> > > > > Thanks,
> >> > > > >
> >> > > > > Andrey Vesnovaty
> >> > > > > (+972)526775512 | Skype: andrey775512
> >> > > > >
> >> > >
> >> > >
> >> > > [..Nip ..]
> >> > >
> >> > > > > I need to mention the locking issue once again.
> >> > > > > If there is a need to maintain "shared session" in the generic
> rte_flow
> >> > layer
> >> > > > all
> >> > > > > calls to flow_create() with shared action & all delete need to take
> >> > > > sharedsession
> >> > > > > management locks at least for verification. Lock partitioning is also
> bit
> >> > > > problematic
> >> > > > > since one flow may have more than one shared action.
> >> > > >
> >> > > > Then, I think better 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. This would allow HW's
> >> > > > without have the shared context ID
> >> > > > to use the action update.
> >> > >
> >> > > Can you please explain your idea?
> >> >
> >> > I see two types of HW schemes supporting action updates without going
> >> > through call `rte_flow_destroy()` and call `rte_flow_create()`
> >> > - The shared HW action context feature
> >> > - The HW has "pattern" and "action" mapped to different HW objects and
> >> > action can be updated any time.
> >> > Other than above-mentioned RSS use case, another use case would be to
> >> > a) create rte_flow and set the action as DROP (Kind of reserving the HW
> object)
> >> > b) Update the action only when the rest of the requirements ready.
> >> >
> >> > Any API schematic that supports both notions of HW is fine with me.
> >> >
> >> I have an idea if the API will be changed to something like this,
> >> Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> rte_flow_action *action, error)
> >> This will enable the application to send a different action than the original
> one to be switched.
> >> Assuming the PMD supports this.
> >> Does it answer your concerns?
> >
> >
> > This allows both:
> > 1. Update action configuration
> > 2. Replace action by some other action
> > For 2 pure software implementation may carate shred action (that can be
> shared
> > with one flow only, depends on PMD) and later on
> rte_flow_shared_action_update may replace this
> > action with some other action by handle returned from
> rte_flow_shared_action_create
> > Doesign between 1 and 2 is per PMD.
>
> struct rte_flow * object holds the driver representation of the
> pattern + action.
> So in order to update the action, we would need struct rte_flow * in API.
>
Why is that? The idea is to change the action, the action itself is connected to flows.
The PMD can save in the shared_ctx all flows that are connected to this action.
> I think, simple API change would be to accommodate "rte_shared_ctx
> *ctx, rte_flow_action *action" modes
> without introducing the emulation for one or other mode, will be.
>
> enum rte_flow_action_update_type {
> RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> };
>
> struct rte_flow_action_update_type_param {
> enum rte_flow_action_update_type type;
> union {
> struct rte_flow_action_update_type_shared_action_param {
> rte_shared_ctx *ctx;
> } shared_action;
> struct rte_flow_action_update_type_shared_action_param {
> rte_flow *flow,
> rte_flow_action *action;
> } action;
> }
> }
>
Thank you for the idea but I fall to see how your suggested API is simpler than the one suggested by me?
In my suggestion the PMD simply needs to check if the new action and change the
context and to that action, or just change parameters in the action, if it is the same action.
Let's go with the original patch API modified to support like you requested also changing the action,
based on my comments.
> rte_flow_action_update(uint16_port port, struct
> rte_flow_action_update_type_param *param, error)
>
> >
> >>
> >> >
> >> > > As I can see if we use the flow_action array it may result in bugs.
> >> > > For example, the application created two flows with the same RSS (not
> using
> >> > the context)
> >> > > Then he wants to change one flow to use different RSS, but the result will
> that
> >> > both flows
> >> > > will be changed.
> >> >
> >> > Sorry. I don't quite follow this.
> >> >
> >> I was trying to show that there must be some context. But I don’t think this is
> relevant to
> >> your current ideas.
> >>
> >> > > Also this will enforce the PMD to keep track on all flows which will have
> >> > memory penalty for
> >> > > some PMDs.
> >>
> >> Best,
> >> Ori
> >
> >
> > Thanks,
> > Andrey
Best,
Ori
On 7/7/2020 7:21 AM, Ori Kam wrote:
> Hi Jerin,
> Thanks you for your quick reply.
>
>> -----Original Message-----
>> From: Jerin Jacob <jerinjacobk@gmail.com>
>> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>>
>> On Mon, Jul 6, 2020 at 7:02 PM Andrey Vesnovaty
>> <andrey.vesnovaty@gmail.com> wrote:
>>>
>>> Hi, Jerin.
>>
>> Hi Ori and Andrey,
>>
>>
>>>
>>> Please see below Ori's suggestion below to implement your
>> rte_flow_action_update() idea
>>> with some API changes of rte_flow_shared_action_xxx API changes.
>>>
>>> On Mon, Jul 6, 2020 at 3:28 PM Ori Kam <orika@mellanox.com> wrote:
>>>>
>>>> Hi Jerin,
>>>>
>>>>> -----Original Message-----
>>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>>>> Sent: Monday, July 6, 2020 12:00 PM
>>>>> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>>>>>
>>>>> On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
>>>>>>
>>>>>> Hi Jerin,
>>>>>> PSB,
>>>>>>
>>>>>> Thanks,
>>>>>> Ori
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>>>>>> Sent: Saturday, July 4, 2020 3:33 PM
>>>>>>> dpdk-dev <dev@dpdk.org>
>>>>>>> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>>>>>>>
>>>>>>> On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
>>>>>>> <andrey.vesnovaty@gmail.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Andrey Vesnovaty
>>>>>>>> (+972)526775512 | Skype: andrey775512
>>>>>>>>
>>>>>>
>>>>>>
>>>>>> [..Nip ..]
>>>>>>
>>>>>>>> I need to mention the locking issue once again.
>>>>>>>> If there is a need to maintain "shared session" in the generic
>> rte_flow
>>>>> layer
>>>>>>> all
>>>>>>>> calls to flow_create() with shared action & all delete need to take
>>>>>>> sharedsession
>>>>>>>> management locks at least for verification. Lock partitioning is also
>> bit
>>>>>>> problematic
>>>>>>>> since one flow may have more than one shared action.
>>>>>>>
>>>>>>> Then, I think better 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. This would allow HW's
>>>>>>> without have the shared context ID
>>>>>>> to use the action update.
>>>>>>
>>>>>> Can you please explain your idea?
>>>>>
>>>>> I see two types of HW schemes supporting action updates without going
>>>>> through call `rte_flow_destroy()` and call `rte_flow_create()`
>>>>> - The shared HW action context feature
>>>>> - The HW has "pattern" and "action" mapped to different HW objects and
>>>>> action can be updated any time.
>>>>> Other than above-mentioned RSS use case, another use case would be to
>>>>> a) create rte_flow and set the action as DROP (Kind of reserving the HW
>> object)
>>>>> b) Update the action only when the rest of the requirements ready.
>>>>>
>>>>> Any API schematic that supports both notions of HW is fine with me.
>>>>>
>>>> I have an idea if the API will be changed to something like this,
>>>> Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
>> rte_flow_action *action, error)
>>>> This will enable the application to send a different action than the original
>> one to be switched.
>>>> Assuming the PMD supports this.
>>>> Does it answer your concerns?
>>>
>>>
>>> This allows both:
>>> 1. Update action configuration
>>> 2. Replace action by some other action
>>> For 2 pure software implementation may carate shred action (that can be
>> shared
>>> with one flow only, depends on PMD) and later on
>> rte_flow_shared_action_update may replace this
>>> action with some other action by handle returned from
>> rte_flow_shared_action_create
>>> Doesign between 1 and 2 is per PMD.
>>
>> struct rte_flow * object holds the driver representation of the
>> pattern + action.
>> So in order to update the action, we would need struct rte_flow * in API.
>>
> Why is that? The idea is to change the action, the action itself is connected to flows.
> The PMD can save in the shared_ctx all flows that are connected to this action.
>
>> I think, simple API change would be to accommodate "rte_shared_ctx
>> *ctx, rte_flow_action *action" modes
>> without introducing the emulation for one or other mode, will be.
>>
>> enum rte_flow_action_update_type {
>> RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
>> RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
>> };
>>
>> struct rte_flow_action_update_type_param {
>> enum rte_flow_action_update_type type;
>> union {
>> struct rte_flow_action_update_type_shared_action_param {
>> rte_shared_ctx *ctx;
>> } shared_action;
>> struct rte_flow_action_update_type_shared_action_param {
>> rte_flow *flow,
>> rte_flow_action *action;
>> } action;
>> }
>> }
>>
> Thank you for the idea but I fall to see how your suggested API is simpler than the one suggested by me?
> In my suggestion the PMD simply needs to check if the new action and change the
> context and to that action, or just change parameters in the action, if it is the same action.
>
> Let's go with the original patch API modified to support like you requested also changing the action,
> based on my comments.
>
>> rte_flow_action_update(uint16_port port, struct
>> rte_flow_action_update_type_param *param, error)
>>
>>>
>>>>
>>>>>
>>>>>> As I can see if we use the flow_action array it may result in bugs.
>>>>>> For example, the application created two flows with the same RSS (not
>> using
>>>>> the context)
>>>>>> Then he wants to change one flow to use different RSS, but the result will
>> that
>>>>> both flows
>>>>>> will be changed.
>>>>>
>>>>> Sorry. I don't quite follow this.
>>>>>
>>>> I was trying to show that there must be some context. But I don’t think this is
>> relevant to
>>>> your current ideas.
>>>>
>>>>>> Also this will enforce the PMD to keep track on all flows which will have
>>>>> memory penalty for
>>>>>> some PMDs.
Hi Ori, Andrey,
This is a set of new APIs and we are very close to the -rc1, so we have only a
few days to close the feature to merge them for this release.
Also accompanying PMD and testpmd implementation with the proposed API changes
looks missing.
We can either postpone the patchset to next release to give time for more PMD
owners to participate, which can give better API for long term.
Or try to to squeeze into this release taking into account that the APIs will be
experimental.
What do you think, what is you schedule for the feature, do you have room to
postpone it?
If not, first existing discussions needs to resolved, and it is good to have the
PMD and testpmd implementations, do you think can this be done for next few days?
Thanks,
ferruh
Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
>
> On 7/7/2020 7:21 AM, Ori Kam wrote:
> > Hi Jerin,
> > Thanks you for your quick reply.
> >
> >> -----Original Message-----
> >> From: Jerin Jacob <jerinjacobk@gmail.com>
> >> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> >>
> >> On Mon, Jul 6, 2020 at 7:02 PM Andrey Vesnovaty
> >> <andrey.vesnovaty@gmail.com> wrote:
> >>>
> >>> Hi, Jerin.
> >>
> >> Hi Ori and Andrey,
> >>
> >>
> >>>
> >>> Please see below Ori's suggestion below to implement your
> >> rte_flow_action_update() idea
> >>> with some API changes of rte_flow_shared_action_xxx API changes.
> >>>
> >>> On Mon, Jul 6, 2020 at 3:28 PM Ori Kam <orika@mellanox.com> wrote:
> >>>>
> >>>> Hi Jerin,
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
> >>>>> Sent: Monday, July 6, 2020 12:00 PM
> >>>>> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> >>>>>
> >>>>> On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
> >>>>>>
> >>>>>> Hi Jerin,
> >>>>>> PSB,
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Ori
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
> >>>>>>> Sent: Saturday, July 4, 2020 3:33 PM
> >>>>>>> dpdk-dev <dev@dpdk.org>
> >>>>>>> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> >>>>>>>
> >>>>>>> On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
> >>>>>>> <andrey.vesnovaty@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>>
> >>>>>>>> Andrey Vesnovaty
> >>>>>>>> (+972)526775512 | Skype: andrey775512
> >>>>>>>>
> >>>>>>
> >>>>>>
> >>>>>> [..Nip ..]
> >>>>>>
> >>>>>>>> I need to mention the locking issue once again.
> >>>>>>>> If there is a need to maintain "shared session" in the generic
> >> rte_flow
> >>>>> layer
> >>>>>>> all
> >>>>>>>> calls to flow_create() with shared action & all delete need to take
> >>>>>>> sharedsession
> >>>>>>>> management locks at least for verification. Lock partitioning is also
> >> bit
> >>>>>>> problematic
> >>>>>>>> since one flow may have more than one shared action.
> >>>>>>>
> >>>>>>> Then, I think better 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. This would allow HW's
> >>>>>>> without have the shared context ID
> >>>>>>> to use the action update.
> >>>>>>
> >>>>>> Can you please explain your idea?
> >>>>>
> >>>>> I see two types of HW schemes supporting action updates without going
> >>>>> through call `rte_flow_destroy()` and call `rte_flow_create()`
> >>>>> - The shared HW action context feature
> >>>>> - The HW has "pattern" and "action" mapped to different HW objects
> and
> >>>>> action can be updated any time.
> >>>>> Other than above-mentioned RSS use case, another use case would be to
> >>>>> a) create rte_flow and set the action as DROP (Kind of reserving the HW
> >> object)
> >>>>> b) Update the action only when the rest of the requirements ready.
> >>>>>
> >>>>> Any API schematic that supports both notions of HW is fine with me.
> >>>>>
> >>>> I have an idea if the API will be changed to something like this,
> >>>> Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> >> rte_flow_action *action, error)
> >>>> This will enable the application to send a different action than the original
> >> one to be switched.
> >>>> Assuming the PMD supports this.
> >>>> Does it answer your concerns?
> >>>
> >>>
> >>> This allows both:
> >>> 1. Update action configuration
> >>> 2. Replace action by some other action
> >>> For 2 pure software implementation may carate shred action (that can be
> >> shared
> >>> with one flow only, depends on PMD) and later on
> >> rte_flow_shared_action_update may replace this
> >>> action with some other action by handle returned from
> >> rte_flow_shared_action_create
> >>> Doesign between 1 and 2 is per PMD.
> >>
> >> struct rte_flow * object holds the driver representation of the
> >> pattern + action.
> >> So in order to update the action, we would need struct rte_flow * in API.
> >>
> > Why is that? The idea is to change the action, the action itself is connected to
> flows.
> > The PMD can save in the shared_ctx all flows that are connected to this
> action.
> >
> >> I think, simple API change would be to accommodate "rte_shared_ctx
> >> *ctx, rte_flow_action *action" modes
> >> without introducing the emulation for one or other mode, will be.
> >>
> >> enum rte_flow_action_update_type {
> >> RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> >> RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> >> };
> >>
> >> struct rte_flow_action_update_type_param {
> >> enum rte_flow_action_update_type type;
> >> union {
> >> struct rte_flow_action_update_type_shared_action_param {
> >> rte_shared_ctx *ctx;
> >> } shared_action;
> >> struct rte_flow_action_update_type_shared_action_param {
> >> rte_flow *flow,
> >> rte_flow_action *action;
> >> } action;
> >> }
> >> }
> >>
> > Thank you for the idea but I fall to see how your suggested API is simpler than
> the one suggested by me?
> > In my suggestion the PMD simply needs to check if the new action and
> change the
> > context and to that action, or just change parameters in the action, if it is the
> same action.
> >
> > Let's go with the original patch API modified to support like you requested
> also changing the action,
> > based on my comments.
> >
> >> rte_flow_action_update(uint16_port port, struct
> >> rte_flow_action_update_type_param *param, error)
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> As I can see if we use the flow_action array it may result in bugs.
> >>>>>> For example, the application created two flows with the same RSS (not
> >> using
> >>>>> the context)
> >>>>>> Then he wants to change one flow to use different RSS, but the result
> will
> >> that
> >>>>> both flows
> >>>>>> will be changed.
> >>>>>
> >>>>> Sorry. I don't quite follow this.
> >>>>>
> >>>> I was trying to show that there must be some context. But I don’t think this
> is
> >> relevant to
> >>>> your current ideas.
> >>>>
> >>>>>> Also this will enforce the PMD to keep track on all flows which will have
> >>>>> memory penalty for
> >>>>>> some PMDs.
>
> Hi Ori, Andrey,
>
> This is a set of new APIs and we are very close to the -rc1, so we have only a
> few days to close the feature to merge them for this release.
>
> Also accompanying PMD and testpmd implementation with the proposed API
> changes
> looks missing.
>
> We can either postpone the patchset to next release to give time for more
> PMD
> owners to participate, which can give better API for long term.
> Or try to to squeeze into this release taking into account that the APIs will be
> experimental.
>
> What do you think, what is you schedule for the feature, do you have room to
> postpone it?
Not so much it is an important API for Mellanox.
> If not, first existing discussions needs to resolved, and it is good to have the
> PMD and testpmd implementations, do you think can this be done for next few
> days?
>
I think that this is the correct API to implement, I fully agree that this API is experimental
just like any other new API, and might change based on comments and use cases.
I know that Mellanox is committed to this feature and that Andrey is working around the clock
to complete the missing parts, and should have a version by tomorrow (July 8th ) evening.
(with update to flow filtering sample app, testpmd will not be ready by RC1, but it will be for RC2)
We would like very much to push it in this version.
Thanks,
Ori
>
> Thanks,
> ferruh
On 7/7/2020 6:24 PM, Ori Kam wrote:
> Hi Ferruh,
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>
>> On 7/7/2020 7:21 AM, Ori Kam wrote:
>>> Hi Jerin,
>>> Thanks you for your quick reply.
>>>
>>>> -----Original Message-----
>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>>> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>>>>
>>>> On Mon, Jul 6, 2020 at 7:02 PM Andrey Vesnovaty
>>>> <andrey.vesnovaty@gmail.com> wrote:
>>>>>
>>>>> Hi, Jerin.
>>>>
>>>> Hi Ori and Andrey,
>>>>
>>>>
>>>>>
>>>>> Please see below Ori's suggestion below to implement your
>>>> rte_flow_action_update() idea
>>>>> with some API changes of rte_flow_shared_action_xxx API changes.
>>>>>
>>>>> On Mon, Jul 6, 2020 at 3:28 PM Ori Kam <orika@mellanox.com> wrote:
>>>>>>
>>>>>> Hi Jerin,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>>>>>> Sent: Monday, July 6, 2020 12:00 PM
>>>>>>> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>>>>>>>
>>>>>>> On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
>>>>>>>>
>>>>>>>> Hi Jerin,
>>>>>>>> PSB,
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Ori
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>>>>>>>> Sent: Saturday, July 4, 2020 3:33 PM
>>>>>>>>> dpdk-dev <dev@dpdk.org>
>>>>>>>>> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>>>>>>>>>
>>>>>>>>> On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
>>>>>>>>> <andrey.vesnovaty@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Andrey Vesnovaty
>>>>>>>>>> (+972)526775512 | Skype: andrey775512
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> [..Nip ..]
>>>>>>>>
>>>>>>>>>> I need to mention the locking issue once again.
>>>>>>>>>> If there is a need to maintain "shared session" in the generic
>>>> rte_flow
>>>>>>> layer
>>>>>>>>> all
>>>>>>>>>> calls to flow_create() with shared action & all delete need to take
>>>>>>>>> sharedsession
>>>>>>>>>> management locks at least for verification. Lock partitioning is also
>>>> bit
>>>>>>>>> problematic
>>>>>>>>>> since one flow may have more than one shared action.
>>>>>>>>>
>>>>>>>>> Then, I think better 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. This would allow HW's
>>>>>>>>> without have the shared context ID
>>>>>>>>> to use the action update.
>>>>>>>>
>>>>>>>> Can you please explain your idea?
>>>>>>>
>>>>>>> I see two types of HW schemes supporting action updates without going
>>>>>>> through call `rte_flow_destroy()` and call `rte_flow_create()`
>>>>>>> - The shared HW action context feature
>>>>>>> - The HW has "pattern" and "action" mapped to different HW objects
>> and
>>>>>>> action can be updated any time.
>>>>>>> Other than above-mentioned RSS use case, another use case would be to
>>>>>>> a) create rte_flow and set the action as DROP (Kind of reserving the HW
>>>> object)
>>>>>>> b) Update the action only when the rest of the requirements ready.
>>>>>>>
>>>>>>> Any API schematic that supports both notions of HW is fine with me.
>>>>>>>
>>>>>> I have an idea if the API will be changed to something like this,
>>>>>> Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
>>>> rte_flow_action *action, error)
>>>>>> This will enable the application to send a different action than the original
>>>> one to be switched.
>>>>>> Assuming the PMD supports this.
>>>>>> Does it answer your concerns?
>>>>>
>>>>>
>>>>> This allows both:
>>>>> 1. Update action configuration
>>>>> 2. Replace action by some other action
>>>>> For 2 pure software implementation may carate shred action (that can be
>>>> shared
>>>>> with one flow only, depends on PMD) and later on
>>>> rte_flow_shared_action_update may replace this
>>>>> action with some other action by handle returned from
>>>> rte_flow_shared_action_create
>>>>> Doesign between 1 and 2 is per PMD.
>>>>
>>>> struct rte_flow * object holds the driver representation of the
>>>> pattern + action.
>>>> So in order to update the action, we would need struct rte_flow * in API.
>>>>
>>> Why is that? The idea is to change the action, the action itself is connected to
>> flows.
>>> The PMD can save in the shared_ctx all flows that are connected to this
>> action.
>>>
>>>> I think, simple API change would be to accommodate "rte_shared_ctx
>>>> *ctx, rte_flow_action *action" modes
>>>> without introducing the emulation for one or other mode, will be.
>>>>
>>>> enum rte_flow_action_update_type {
>>>> RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
>>>> RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
>>>> };
>>>>
>>>> struct rte_flow_action_update_type_param {
>>>> enum rte_flow_action_update_type type;
>>>> union {
>>>> struct rte_flow_action_update_type_shared_action_param {
>>>> rte_shared_ctx *ctx;
>>>> } shared_action;
>>>> struct rte_flow_action_update_type_shared_action_param {
>>>> rte_flow *flow,
>>>> rte_flow_action *action;
>>>> } action;
>>>> }
>>>> }
>>>>
>>> Thank you for the idea but I fall to see how your suggested API is simpler than
>> the one suggested by me?
>>> In my suggestion the PMD simply needs to check if the new action and
>> change the
>>> context and to that action, or just change parameters in the action, if it is the
>> same action.
>>>
>>> Let's go with the original patch API modified to support like you requested
>> also changing the action,
>>> based on my comments.
>>>
>>>> rte_flow_action_update(uint16_port port, struct
>>>> rte_flow_action_update_type_param *param, error)
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> As I can see if we use the flow_action array it may result in bugs.
>>>>>>>> For example, the application created two flows with the same RSS (not
>>>> using
>>>>>>> the context)
>>>>>>>> Then he wants to change one flow to use different RSS, but the result
>> will
>>>> that
>>>>>>> both flows
>>>>>>>> will be changed.
>>>>>>>
>>>>>>> Sorry. I don't quite follow this.
>>>>>>>
>>>>>> I was trying to show that there must be some context. But I don’t think this
>> is
>>>> relevant to
>>>>>> your current ideas.
>>>>>>
>>>>>>>> Also this will enforce the PMD to keep track on all flows which will have
>>>>>>> memory penalty for
>>>>>>>> some PMDs.
>>
>> Hi Ori, Andrey,
>>
>> This is a set of new APIs and we are very close to the -rc1, so we have only a
>> few days to close the feature to merge them for this release.
>>
>> Also accompanying PMD and testpmd implementation with the proposed API
>> changes
>> looks missing.
>>
>> We can either postpone the patchset to next release to give time for more
>> PMD
>> owners to participate, which can give better API for long term.
>> Or try to to squeeze into this release taking into account that the APIs will be
>> experimental.
>>
>> What do you think, what is you schedule for the feature, do you have room to
>> postpone it?
> Not so much it is an important API for Mellanox.
Got it.
>
>> If not, first existing discussions needs to resolved, and it is good to have the
>> PMD and testpmd implementations, do you think can this be done for next few
>> days?
>>
> I think that this is the correct API to implement, I fully agree that this API is experimental
> just like any other new API, and might change based on comments and use cases.
> I know that Mellanox is committed to this feature and that Andrey is working around the clock
> to complete the missing parts, and should have a version by tomorrow (July 8th ) evening.
OK
> (with update to flow filtering sample app, testpmd will not be ready by RC1, but it will be for RC2)
> We would like very much to push it in this version.
OK, please conclude the existing discussion before finalizing.
On Tue, Jul 7, 2020 at 11:51 AM Ori Kam <orika@mellanox.com> wrote:
>
> Hi Jerin,
> Thanks you for your quick reply.
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> >
> > On Mon, Jul 6, 2020 at 7:02 PM Andrey Vesnovaty
> > <andrey.vesnovaty@gmail.com> wrote:
> > >
> > > Hi, Jerin.
> >
> > Hi Ori and Andrey,
> >
> >
> > >
> > > Please see below Ori's suggestion below to implement your
> > rte_flow_action_update() idea
> > > with some API changes of rte_flow_shared_action_xxx API changes.
> > >
> > > On Mon, Jul 6, 2020 at 3:28 PM Ori Kam <orika@mellanox.com> wrote:
> > >>
> > >> Hi Jerin,
> > >>
> > >> > -----Original Message-----
> > >> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > >> > Sent: Monday, July 6, 2020 12:00 PM
> > >> > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> > >> >
> > >> > On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
> > >> > >
> > >> > > Hi Jerin,
> > >> > > PSB,
> > >> > >
> > >> > > Thanks,
> > >> > > Ori
> > >> > >
> > >> > > > -----Original Message-----
> > >> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > >> > > > Sent: Saturday, July 4, 2020 3:33 PM
> > >> > > > dpdk-dev <dev@dpdk.org>
> > >> > > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> > >> > > >
> > >> > > > On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
> > >> > > > <andrey.vesnovaty@gmail.com> wrote:
> > >> > > > >
> > >> > > > >
> > >> > > > > Thanks,
> > >> > > > >
> > >> > > > > Andrey Vesnovaty
> > >> > > > > (+972)526775512 | Skype: andrey775512
> > >> > > > >
> > >> > >
> > >> > >
> > >> > > [..Nip ..]
> > >> > >
> > >> > > > > I need to mention the locking issue once again.
> > >> > > > > If there is a need to maintain "shared session" in the generic
> > rte_flow
> > >> > layer
> > >> > > > all
> > >> > > > > calls to flow_create() with shared action & all delete need to take
> > >> > > > sharedsession
> > >> > > > > management locks at least for verification. Lock partitioning is also
> > bit
> > >> > > > problematic
> > >> > > > > since one flow may have more than one shared action.
> > >> > > >
> > >> > > > Then, I think better 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. This would allow HW's
> > >> > > > without have the shared context ID
> > >> > > > to use the action update.
> > >> > >
> > >> > > Can you please explain your idea?
> > >> >
> > >> > I see two types of HW schemes supporting action updates without going
> > >> > through call `rte_flow_destroy()` and call `rte_flow_create()`
> > >> > - The shared HW action context feature
> > >> > - The HW has "pattern" and "action" mapped to different HW objects and
> > >> > action can be updated any time.
> > >> > Other than above-mentioned RSS use case, another use case would be to
> > >> > a) create rte_flow and set the action as DROP (Kind of reserving the HW
> > object)
> > >> > b) Update the action only when the rest of the requirements ready.
> > >> >
> > >> > Any API schematic that supports both notions of HW is fine with me.
> > >> >
> > >> I have an idea if the API will be changed to something like this,
> > >> Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> > rte_flow_action *action, error)
> > >> This will enable the application to send a different action than the original
> > one to be switched.
> > >> Assuming the PMD supports this.
> > >> Does it answer your concerns?
> > >
> > >
> > > This allows both:
> > > 1. Update action configuration
> > > 2. Replace action by some other action
> > > For 2 pure software implementation may carate shred action (that can be
> > shared
> > > with one flow only, depends on PMD) and later on
> > rte_flow_shared_action_update may replace this
> > > action with some other action by handle returned from
> > rte_flow_shared_action_create
> > > Doesign between 1 and 2 is per PMD.
> >
> > struct rte_flow * object holds the driver representation of the
> > pattern + action.
> > So in order to update the action, we would need struct rte_flow * in API.
> >
> Why is that? The idea is to change the action, the action itself is connected to flows.
> The PMD can save in the shared_ctx all flows that are connected to this action.
>
> > I think, simple API change would be to accommodate "rte_shared_ctx
> > *ctx, rte_flow_action *action" modes
> > without introducing the emulation for one or other mode, will be.
> >
> > enum rte_flow_action_update_type {
> > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> > RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> > };
> >
> > struct rte_flow_action_update_type_param {
> > enum rte_flow_action_update_type type;
> > union {
> > struct rte_flow_action_update_type_shared_action_param {
> > rte_shared_ctx *ctx;
> > } shared_action;
> > struct rte_flow_action_update_type_shared_action_param {
> > rte_flow *flow,
> > rte_flow_action *action;
> > } action;
> > }
> > }
> >
> Thank you for the idea but I fall to see how your suggested API is simpler than the one suggested by me?
My thought process with the below-proposed API[1] is that It is
dictates "_shared_action_" in API name as
well as arguments. I just thought of expressing it as either-or case
hence I thought [2] is better. i.e The PMD does not support
shared_action, not even need to create one to use
rte_flow_action_update() to avoid the confusion. Thoughts?
[1]
rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
rte_flow_action *action, error)
[2]
rte_flow_action_update(uint16_port port, struct
rte_flow_action_update_type_param *param, error)
> In my suggestion the PMD simply needs to check if the new action and change the
> context and to that action, or just change parameters in the action, if it is the same action.
>
> Let's go with the original patch API modified to support like you requested also changing the action,
> based on my comments.
>
> > rte_flow_action_update(uint16_port port, struct
> > rte_flow_action_update_type_param *param, error)
> >
> > >
> > >>
> > >> >
> > >> > > As I can see if we use the flow_action array it may result in bugs.
> > >> > > For example, the application created two flows with the same RSS (not
> > using
> > >> > the context)
> > >> > > Then he wants to change one flow to use different RSS, but the result will
> > that
> > >> > both flows
> > >> > > will be changed.
> > >> >
> > >> > Sorry. I don't quite follow this.
> > >> >
> > >> I was trying to show that there must be some context. But I don’t think this is
> > relevant to
> > >> your current ideas.
> > >>
> > >> > > Also this will enforce the PMD to keep track on all flows which will have
> > >> > memory penalty for
> > >> > > some PMDs.
> > >>
> > >> Best,
> > >> Ori
> > >
> > >
> > > Thanks,
> > > Andrey
> Best,
> Ori
Hi Jerin,
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
>
> On Tue, Jul 7, 2020 at 11:51 AM Ori Kam <orika@mellanox.com> wrote:
> >
> > Hi Jerin,
> > Thanks you for your quick reply.
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> > >
> > > On Mon, Jul 6, 2020 at 7:02 PM Andrey Vesnovaty
> > > <andrey.vesnovaty@gmail.com> wrote:
> > > >
> > > > Hi, Jerin.
> > >
> > > Hi Ori and Andrey,
> > >
> > >
> > > >
> > > > Please see below Ori's suggestion below to implement your
> > > rte_flow_action_update() idea
> > > > with some API changes of rte_flow_shared_action_xxx API changes.
> > > >
> > > > On Mon, Jul 6, 2020 at 3:28 PM Ori Kam <orika@mellanox.com> wrote:
> > > >>
> > > >> Hi Jerin,
> > > >>
> > > >> > -----Original Message-----
> > > >> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > >> > Sent: Monday, July 6, 2020 12:00 PM
> > > >> > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> > > >> >
> > > >> > On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
> > > >> > >
> > > >> > > Hi Jerin,
> > > >> > > PSB,
> > > >> > >
> > > >> > > Thanks,
> > > >> > > Ori
> > > >> > >
> > > >> > > > -----Original Message-----
> > > >> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > >> > > > Sent: Saturday, July 4, 2020 3:33 PM
> > > >> > > > dpdk-dev <dev@dpdk.org>
> > > >> > > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> > > >> > > >
> > > >> > > > On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
> > > >> > > > <andrey.vesnovaty@gmail.com> wrote:
> > > >> > > > >
> > > >> > > > >
> > > >> > > > > Thanks,
> > > >> > > > >
> > > >> > > > > Andrey Vesnovaty
> > > >> > > > > (+972)526775512 | Skype: andrey775512
> > > >> > > > >
> > > >> > >
> > > >> > >
> > > >> > > [..Nip ..]
> > > >> > >
> > > >> > > > > I need to mention the locking issue once again.
> > > >> > > > > If there is a need to maintain "shared session" in the generic
> > > rte_flow
> > > >> > layer
> > > >> > > > all
> > > >> > > > > calls to flow_create() with shared action & all delete need to take
> > > >> > > > sharedsession
> > > >> > > > > management locks at least for verification. Lock partitioning is
> also
> > > bit
> > > >> > > > problematic
> > > >> > > > > since one flow may have more than one shared action.
> > > >> > > >
> > > >> > > > Then, I think better 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. This would allow HW's
> > > >> > > > without have the shared context ID
> > > >> > > > to use the action update.
> > > >> > >
> > > >> > > Can you please explain your idea?
> > > >> >
> > > >> > I see two types of HW schemes supporting action updates without
> going
> > > >> > through call `rte_flow_destroy()` and call `rte_flow_create()`
> > > >> > - The shared HW action context feature
> > > >> > - The HW has "pattern" and "action" mapped to different HW objects
> and
> > > >> > action can be updated any time.
> > > >> > Other than above-mentioned RSS use case, another use case would be
> to
> > > >> > a) create rte_flow and set the action as DROP (Kind of reserving the
> HW
> > > object)
> > > >> > b) Update the action only when the rest of the requirements ready.
> > > >> >
> > > >> > Any API schematic that supports both notions of HW is fine with me.
> > > >> >
> > > >> I have an idea if the API will be changed to something like this,
> > > >> Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> > > rte_flow_action *action, error)
> > > >> This will enable the application to send a different action than the
> original
> > > one to be switched.
> > > >> Assuming the PMD supports this.
> > > >> Does it answer your concerns?
> > > >
> > > >
> > > > This allows both:
> > > > 1. Update action configuration
> > > > 2. Replace action by some other action
> > > > For 2 pure software implementation may carate shred action (that can be
> > > shared
> > > > with one flow only, depends on PMD) and later on
> > > rte_flow_shared_action_update may replace this
> > > > action with some other action by handle returned from
> > > rte_flow_shared_action_create
> > > > Doesign between 1 and 2 is per PMD.
> > >
> > > struct rte_flow * object holds the driver representation of the
> > > pattern + action.
> > > So in order to update the action, we would need struct rte_flow * in API.
> > >
> > Why is that? The idea is to change the action, the action itself is connected to
> flows.
> > The PMD can save in the shared_ctx all flows that are connected to this
> action.
> >
> > > I think, simple API change would be to accommodate "rte_shared_ctx
> > > *ctx, rte_flow_action *action" modes
> > > without introducing the emulation for one or other mode, will be.
> > >
> > > enum rte_flow_action_update_type {
> > > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> > > RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> > > };
> > >
> > > struct rte_flow_action_update_type_param {
> > > enum rte_flow_action_update_type type;
> > > union {
> > > struct rte_flow_action_update_type_shared_action_param {
> > > rte_shared_ctx *ctx;
> > > } shared_action;
> > > struct rte_flow_action_update_type_shared_action_param {
> > > rte_flow *flow,
> > > rte_flow_action *action;
> > > } action;
> > > }
> > > }
> > >
> > Thank you for the idea but I fall to see how your suggested API is simpler than
> the one suggested by me?
>
> My thought process with the below-proposed API[1] is that It is
> dictates "_shared_action_" in API name as
> well as arguments. I just thought of expressing it as either-or case
> hence I thought [2] is better. i.e The PMD does not support
> shared_action, not even need to create one to use
> rte_flow_action_update() to avoid the confusion. Thoughts?
>
> [1]
> rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> rte_flow_action *action, error)
>
> [2]
> rte_flow_action_update(uint16_port port, struct
> rte_flow_action_update_type_param *param, error)
>
Let me see if I understand you correctly, your suggestion is to allow
the application to change one action in one flow, but instead of creating
the context the application will just supply the rte_flow and the new actions
do I understand correctly?
If so this it is a nice idea, but there are some issues with it,
1. The PMD must save the flow which will result in memory consumption.
2. Assume that two flows are using the same RSS action for example, so the PMD
reuse the RSS object he created for the first flow also for the second. Now changing
this RSS flow may result in also changing the second flow. (this can be solved by always
creating new action)
3. It doesn't handle the main use case that the application wants to change number of
flows at the same time, which is the idea of this feature.
I also think that all PMD that support option 2 can support option 1 since
they can save in the ctx a list of flows and simply apply them again. So by
definition if PMD supports [2] it also support [1] while the other
way is not correct since it forces the PMD to save flows which like I said waste memory.
I suggest that we will go with option [1], and if needed in the future we will update the code.
using option [2] will result in dead code since at least for the current time no PMD will implement this
API.
I can suggest one more thing maybe to change the name from shared_ctx to just ctx
which implicitly mean it can be shared but not a must. What do you think? (but again
I think by definition if a PMD can implement number [2] it can also implement it to number
of flows using API [2].
> > In my suggestion the PMD simply needs to check if the new action and
> change the
> > context and to that action, or just change parameters in the action, if it is the
> same action.
> >
> > Let's go with the original patch API modified to support like you requested
> also changing the action,
> > based on my comments.
> >
> > > rte_flow_action_update(uint16_port port, struct
> > > rte_flow_action_update_type_param *param, error)
> > >
> > > >
[..nip..]
Best,
Ori
On Wed, Jul 8, 2020 at 2:33 AM Ori Kam <orika@mellanox.com> wrote:
>
> Hi Jerin,
Hi Ori,
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> >
> > On Tue, Jul 7, 2020 at 11:51 AM Ori Kam <orika@mellanox.com> wrote:
> > >
> > > Hi Jerin,
> > > Thanks you for your quick reply.
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> > > >
> > > > On Mon, Jul 6, 2020 at 7:02 PM Andrey Vesnovaty
> > > > <andrey.vesnovaty@gmail.com> wrote:
> > > > >
> > > > > Hi, Jerin.
> > > >
> > > > Hi Ori and Andrey,
> > > >
> > > >
> > > > >
> > > > > Please see below Ori's suggestion below to implement your
> > > > rte_flow_action_update() idea
> > > > > with some API changes of rte_flow_shared_action_xxx API changes.
> > > > >
> > > > > On Mon, Jul 6, 2020 at 3:28 PM Ori Kam <orika@mellanox.com> wrote:
> > > > >>
> > > > >> Hi Jerin,
> > > > >>
> > > > >> > -----Original Message-----
> > > > >> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > >> > Sent: Monday, July 6, 2020 12:00 PM
> > > > >> > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> > > > >> >
> > > > >> > On Sun, Jul 5, 2020 at 3:56 PM Ori Kam <orika@mellanox.com> wrote:
> > > > >> > >
> > > > >> > > Hi Jerin,
> > > > >> > > PSB,
> > > > >> > >
> > > > >> > > Thanks,
> > > > >> > > Ori
> > > > >> > >
> > > > >> > > > -----Original Message-----
> > > > >> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > >> > > > Sent: Saturday, July 4, 2020 3:33 PM
> > > > >> > > > dpdk-dev <dev@dpdk.org>
> > > > >> > > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
> > > > >> > > >
> > > > >> > > > On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty
> > > > >> > > > <andrey.vesnovaty@gmail.com> wrote:
> > > > >> > > > >
> > > > >> > > > >
> > > > >> > > > > Thanks,
> > > > >> > > > >
> > > > >> > > > > Andrey Vesnovaty
> > > > >> > > > > (+972)526775512 | Skype: andrey775512
> > > > >> > > > >
> > > > >> > >
> > > > >> > >
> > > > >> > > [..Nip ..]
> > > > >> > >
> > > > >> > > > > I need to mention the locking issue once again.
> > > > >> > > > > If there is a need to maintain "shared session" in the generic
> > > > rte_flow
> > > > >> > layer
> > > > >> > > > all
> > > > >> > > > > calls to flow_create() with shared action & all delete need to take
> > > > >> > > > sharedsession
> > > > >> > > > > management locks at least for verification. Lock partitioning is
> > also
> > > > bit
> > > > >> > > > problematic
> > > > >> > > > > since one flow may have more than one shared action.
> > > > >> > > >
> > > > >> > > > Then, I think better 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. This would allow HW's
> > > > >> > > > without have the shared context ID
> > > > >> > > > to use the action update.
> > > > >> > >
> > > > >> > > Can you please explain your idea?
> > > > >> >
> > > > >> > I see two types of HW schemes supporting action updates without
> > going
> > > > >> > through call `rte_flow_destroy()` and call `rte_flow_create()`
> > > > >> > - The shared HW action context feature
> > > > >> > - The HW has "pattern" and "action" mapped to different HW objects
> > and
> > > > >> > action can be updated any time.
> > > > >> > Other than above-mentioned RSS use case, another use case would be
> > to
> > > > >> > a) create rte_flow and set the action as DROP (Kind of reserving the
> > HW
> > > > object)
> > > > >> > b) Update the action only when the rest of the requirements ready.
> > > > >> >
> > > > >> > Any API schematic that supports both notions of HW is fine with me.
> > > > >> >
> > > > >> I have an idea if the API will be changed to something like this,
> > > > >> Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> > > > rte_flow_action *action, error)
> > > > >> This will enable the application to send a different action than the
> > original
> > > > one to be switched.
> > > > >> Assuming the PMD supports this.
> > > > >> Does it answer your concerns?
> > > > >
> > > > >
> > > > > This allows both:
> > > > > 1. Update action configuration
> > > > > 2. Replace action by some other action
> > > > > For 2 pure software implementation may carate shred action (that can be
> > > > shared
> > > > > with one flow only, depends on PMD) and later on
> > > > rte_flow_shared_action_update may replace this
> > > > > action with some other action by handle returned from
> > > > rte_flow_shared_action_create
> > > > > Doesign between 1 and 2 is per PMD.
> > > >
> > > > struct rte_flow * object holds the driver representation of the
> > > > pattern + action.
> > > > So in order to update the action, we would need struct rte_flow * in API.
> > > >
> > > Why is that? The idea is to change the action, the action itself is connected to
> > flows.
> > > The PMD can save in the shared_ctx all flows that are connected to this
> > action.
> > >
> > > > I think, simple API change would be to accommodate "rte_shared_ctx
> > > > *ctx, rte_flow_action *action" modes
> > > > without introducing the emulation for one or other mode, will be.
> > > >
> > > > enum rte_flow_action_update_type {
> > > > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> > > > RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> > > > };
> > > >
> > > > struct rte_flow_action_update_type_param {
> > > > enum rte_flow_action_update_type type;
> > > > union {
> > > > struct rte_flow_action_update_type_shared_action_param {
> > > > rte_shared_ctx *ctx;
> > > > } shared_action;
> > > > struct rte_flow_action_update_type_shared_action_param {
> > > > rte_flow *flow,
> > > > rte_flow_action *action;
> > > > } action;
> > > > }
> > > > }
> > > >
> > > Thank you for the idea but I fall to see how your suggested API is simpler than
> > the one suggested by me?
> >
> > My thought process with the below-proposed API[1] is that It is
> > dictates "_shared_action_" in API name as
> > well as arguments. I just thought of expressing it as either-or case
> > hence I thought [2] is better. i.e The PMD does not support
> > shared_action, not even need to create one to use
> > rte_flow_action_update() to avoid the confusion. Thoughts?
> >
> > [1]
> > rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> > rte_flow_action *action, error)
> >
> > [2]
> > rte_flow_action_update(uint16_port port, struct
> > rte_flow_action_update_type_param *param, error)
> >
> Let me see if I understand you correctly, your suggestion is to allow
> the application to change one action in one flow, but instead of creating
> the context the application will just supply the rte_flow and the new actions
> do I understand correctly?
Yes.
>
> If so this it is a nice idea, but there are some issues with it,
> 1. The PMD must save the flow which will result in memory consumption.
struct rte_flow * driver handle any way store that information to as
it would be needed
for other rte_flow related APIs.
> 2. Assume that two flows are using the same RSS action for example, so the PMD
> reuse the RSS object he created for the first flow also for the second. Now changing
> this RSS flow may result in also changing the second flow. (this can be solved by always
> creating new action)
It is not resuing the action, it more of updating the action. So the
above said issue won't happen.
It is removing the need for call `rte_flow_destroy()` and call
`rte_flow_create()` if only action
needs to update for THE given flow. That's it.
> 3. It doesn't handle the main use case that the application wants to change number of
> flows at the same time, which is the idea of this feature.
We discussed this in detail and tried approach for the common code to
make everything
as shared action. Andrey quickly realizes that it is difficult without
HW support.
>
> I also think that all PMD that support option 2 can support option 1 since
> they can save in the ctx a list of flows and simply apply them again. So by
> definition if PMD supports [2] it also support [1] while the other
> way is not correct since it forces the PMD to save flows which like I said waste memory.
If we use "rte_flow_shared_action_update(uint16_port port,
rte_shared_ctx *ctx, rte_flow_action *action, error)",
What would be ctx value for the HW does not support a shared context?
That's is the only reason for
my proposal. I understand, your concern about supporting two modes in
PMD, I don't think,
PMD needs to support RTE_FLOW_ACTION_UPDATE_TYPE_ACTION if
RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION supported.
>
> I suggest that we will go with option [1], and if needed in the future we will update the code.
> using option [2] will result in dead code since at least for the current time no PMD will implement this
> API.
We are planning to update our PMD to support this once API is finalized.
>
> I can suggest one more thing maybe to change the name from shared_ctx to just ctx
> which implicitly mean it can be shared but not a must. What do you think? (but again
> I think by definition if a PMD can implement number [2] it can also implement it to number
> of flows using API [2].
Just void *type is fine too, but we need an argument for type to cast
it in application and/or driver.
enum rte_flow_action_update_type {
RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
};
>
> > > In my suggestion the PMD simply needs to check if the new action and
> > change the
> > > context and to that action, or just change parameters in the action, if it is the
> > same action.
> > >
> > > Let's go with the original patch API modified to support like you requested
> > also changing the action,
> > > based on my comments.
> > >
> > > > rte_flow_action_update(uint16_port port, struct
> > > > rte_flow_action_update_type_param *param, error)
> > > >
> > > > >
> [..nip..]
>
> Best,
> Ori
Hi Jerin
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
>
> On Wed, Jul 8, 2020 at 2:33 AM Ori Kam <orika@mellanox.com> wrote:
> >
> > Hi Jerin,
>
> Hi Ori,
>
> >
> > > -----Original Message-----
[..nip ..]
> > > > > I think, simple API change would be to accommodate "rte_shared_ctx
> > > > > *ctx, rte_flow_action *action" modes
> > > > > without introducing the emulation for one or other mode, will be.
> > > > >
> > > > > enum rte_flow_action_update_type {
> > > > > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> > > > > RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> > > > > };
> > > > >
> > > > > struct rte_flow_action_update_type_param {
> > > > > enum rte_flow_action_update_type type;
> > > > > union {
> > > > > struct rte_flow_action_update_type_shared_action_param
> {
> > > > > rte_shared_ctx *ctx;
> > > > > } shared_action;
> > > > > struct
> rte_flow_action_update_type_shared_action_param {
> > > > > rte_flow *flow,
> > > > > rte_flow_action *action;
> > > > > } action;
> > > > > }
> > > > > }
> > > > >
> > > > Thank you for the idea but I fall to see how your suggested API is simpler
> than
> > > the one suggested by me?
> > >
> > > My thought process with the below-proposed API[1] is that It is
> > > dictates "_shared_action_" in API name as
> > > well as arguments. I just thought of expressing it as either-or case
> > > hence I thought [2] is better. i.e The PMD does not support
> > > shared_action, not even need to create one to use
> > > rte_flow_action_update() to avoid the confusion. Thoughts?
> > >
> > > [1]
> > > rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> > > rte_flow_action *action, error)
> > >
> > > [2]
> > > rte_flow_action_update(uint16_port port, struct
> > > rte_flow_action_update_type_param *param, error)
> > >
> > Let me see if I understand you correctly, your suggestion is to allow
> > the application to change one action in one flow, but instead of creating
> > the context the application will just supply the rte_flow and the new actions
> > do I understand correctly?
>
> Yes.
>
> >
> > If so this it is a nice idea, but there are some issues with it,
> > 1. The PMD must save the flow which will result in memory consumption.
>
> struct rte_flow * driver handle any way store that information to as
> it would be needed
> for other rte_flow related APIs.
>
The driver for example in Mellanox case save only the pointer to the flow so it can
destroy it on request. It can't remove it and add it again since it is missing critical
info. To save the info will cost memory. I assume this goes to other drivers.
There is no real need to save anything but the flow pointer.
>
> > 2. Assume that two flows are using the same RSS action for example, so the
> PMD
> > reuse the RSS object he created for the first flow also for the second. Now
> changing
> > this RSS flow may result in also changing the second flow. (this can be solved
> by always
> > creating new action)
>
> It is not resuing the action, it more of updating the action. So the
> above said issue won't happen.
> It is removing the need for call `rte_flow_destroy()` and call
> `rte_flow_create()` if only action
> needs to update for THE given flow. That's it.
>
Again this means that the driver must save all flows so it will waste memory.
also this doesn’t save any time, since the application can just do it the same way
as the PMD there is no value to do it inside the PMD.
>
> > 3. It doesn't handle the main use case that the application wants to change
> number of
> > flows at the same time, which is the idea of this feature.
>
> We discussed this in detail and tried approach for the common code to
> make everything
> as shared action. Andrey quickly realizes that it is difficult without
> HW support.
Like everything in RTE flow this is all about HW support 😊
If the HW doesn’t support it don't do it.
>
> >
> > I also think that all PMD that support option 2 can support option 1 since
> > they can save in the ctx a list of flows and simply apply them again. So by
> > definition if PMD supports [2] it also support [1] while the other
> > way is not correct since it forces the PMD to save flows which like I said
> waste memory.
>
> If we use "rte_flow_shared_action_update(uint16_port port,
> rte_shared_ctx *ctx, rte_flow_action *action, error)",
> What would be ctx value for the HW does not support a shared context?
> That's is the only reason for
> my proposal. I understand, your concern about supporting two modes in
> PMD, I don't think,
> PMD needs to support RTE_FLOW_ACTION_UPDATE_TYPE_ACTION if
> RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION supported.
>
This is the beauty of it ctx is opaque so the PMD can have what ever it wants to have. Just like
an rte_flow that each PMD have different fields and usage.
> >
> > I suggest that we will go with option [1], and if needed in the future we will
> update the code.
> > using option [2] will result in dead code since at least for the current time no
> PMD will implement this
> > API.
>
> We are planning to update our PMD to support this once API is finalized.
>
Great very happy to hear that.
This means that we should push this feature even faster 😊
> >
> > I can suggest one more thing maybe to change the name from shared_ctx to
> just ctx
> > which implicitly mean it can be shared but not a must. What do you think?
> (but again
> > I think by definition if a PMD can implement number [2] it can also
> implement it to number
> > of flows using API [2].
>
> Just void *type is fine too, but we need an argument for type to cast
> it in application and/or driver.
>
Like said above this is opaque so the PMD knows what to expect.
> enum rte_flow_action_update_type {
> RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> };
>
> >
> > > > In my suggestion the PMD simply needs to check if the new action and
> > > change the
> > > > context and to that action, or just change parameters in the action, if it is
> the
> > > same action.
> > > >
> > > > Let's go with the original patch API modified to support like you requested
> > > also changing the action,
> > > > based on my comments.
> > > >
> > > > > rte_flow_action_update(uint16_port port, struct
> > > > > rte_flow_action_update_type_param *param, error)
> > > > >
> > > > > >
> > [..nip..]
> >
> > Best,
> > Ori
On Wed, Jul 8, 2020 at 3:17 PM Ori Kam <orika@mellanox.com> wrote:
>
> Hi Jerin
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> >
> > On Wed, Jul 8, 2020 at 2:33 AM Ori Kam <orika@mellanox.com> wrote:
> > >
> > > Hi Jerin,
> >
> > Hi Ori,
> >
> > >
> > > > -----Original Message-----
> [..nip ..]
>
> > > > > > I think, simple API change would be to accommodate "rte_shared_ctx
> > > > > > *ctx, rte_flow_action *action" modes
> > > > > > without introducing the emulation for one or other mode, will be.
> > > > > >
> > > > > > enum rte_flow_action_update_type {
> > > > > > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> > > > > > RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> > > > > > };
> > > > > >
> > > > > > struct rte_flow_action_update_type_param {
> > > > > > enum rte_flow_action_update_type type;
> > > > > > union {
> > > > > > struct rte_flow_action_update_type_shared_action_param
> > {
> > > > > > rte_shared_ctx *ctx;
> > > > > > } shared_action;
> > > > > > struct
> > rte_flow_action_update_type_shared_action_param {
> > > > > > rte_flow *flow,
> > > > > > rte_flow_action *action;
> > > > > > } action;
> > > > > > }
> > > > > > }
> > > > > >
> > > > > Thank you for the idea but I fall to see how your suggested API is simpler
> > than
> > > > the one suggested by me?
> > > >
> > > > My thought process with the below-proposed API[1] is that It is
> > > > dictates "_shared_action_" in API name as
> > > > well as arguments. I just thought of expressing it as either-or case
> > > > hence I thought [2] is better. i.e The PMD does not support
> > > > shared_action, not even need to create one to use
> > > > rte_flow_action_update() to avoid the confusion. Thoughts?
> > > >
> > > > [1]
> > > > rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> > > > rte_flow_action *action, error)
> > > >
> > > > [2]
> > > > rte_flow_action_update(uint16_port port, struct
> > > > rte_flow_action_update_type_param *param, error)
> > > >
> > > Let me see if I understand you correctly, your suggestion is to allow
> > > the application to change one action in one flow, but instead of creating
> > > the context the application will just supply the rte_flow and the new actions
> > > do I understand correctly?
> >
> > Yes.
> >
> > >
> > > If so this it is a nice idea, but there are some issues with it,
> > > 1. The PMD must save the flow which will result in memory consumption.
> >
> > struct rte_flow * driver handle any way store that information to as
> > it would be needed
> > for other rte_flow related APIs.
> >
> The driver for example in Mellanox case save only the pointer to the flow so it can
> destroy it on request. It can't remove it and add it again since it is missing critical
> info. To save the info will cost memory. I assume this goes to other drivers.
> There is no real need to save anything but the flow pointer.
If driver _need_ this feature, then the driver needs to store some
info in the flow object,
In our case, it will be the index of the MCAM action, etc.
Yes. it up to driver the how they want to support it.
>
> >
> > > 2. Assume that two flows are using the same RSS action for example, so the
> > PMD
> > > reuse the RSS object he created for the first flow also for the second. Now
> > changing
> > > this RSS flow may result in also changing the second flow. (this can be solved
> > by always
> > > creating new action)
> >
> > It is not resuing the action, it more of updating the action. So the
> > above said issue won't happen.
> > It is removing the need for call `rte_flow_destroy()` and call
> > `rte_flow_create()` if only action
> > needs to update for THE given flow. That's it.
> >
> Again this means that the driver must save all flows so it will waste memory.
> also this doesn’t save any time, since the application can just do it the same way
> as the PMD there is no value to do it inside the PMD.
Why driver need to save all the flow, We need to save all the flow ONLY when
we need to emulate the shared context. We need to save only additional
info like,
MCAM index where the action was allocated or such, so that it can be replaced.
>
> >
> > > 3. It doesn't handle the main use case that the application wants to change
> > number of
> > > flows at the same time, which is the idea of this feature.
> >
> > We discussed this in detail and tried approach for the common code to
> > make everything
> > as shared action. Andrey quickly realizes that it is difficult without
> > HW support.
> Like everything in RTE flow this is all about HW support
> If the HW doesn’t support it don't do it.
>
> >
> > >
> > > I also think that all PMD that support option 2 can support option 1 since
> > > they can save in the ctx a list of flows and simply apply them again. So by
> > > definition if PMD supports [2] it also support [1] while the other
> > > way is not correct since it forces the PMD to save flows which like I said
> > waste memory.
> >
> > If we use "rte_flow_shared_action_update(uint16_port port,
> > rte_shared_ctx *ctx, rte_flow_action *action, error)",
> > What would be ctx value for the HW does not support a shared context?
> > That's is the only reason for
> > my proposal. I understand, your concern about supporting two modes in
> > PMD, I don't think,
> > PMD needs to support RTE_FLOW_ACTION_UPDATE_TYPE_ACTION if
> > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION supported.
> >
> This is the beauty of it ctx is opaque so the PMD can have what ever it wants to have. Just like
> an rte_flow that each PMD have different fields and usage.
>
> > >
> > > I suggest that we will go with option [1], and if needed in the future we will
> > update the code.
> > > using option [2] will result in dead code since at least for the current time no
> > PMD will implement this
> > > API.
> >
> > We are planning to update our PMD to support this once API is finalized.
> >
> Great very happy to hear that.
> This means that we should push this feature even faster
>
> > >
> > > I can suggest one more thing maybe to change the name from shared_ctx to
> > just ctx
> > > which implicitly mean it can be shared but not a must. What do you think?
> > (but again
> > > I think by definition if a PMD can implement number [2] it can also
> > implement it to number
> > > of flows using API [2].
> >
> > Just void *type is fine too, but we need an argument for type to cast
> > it in application and/or driver.
> >
> Like said above this is opaque so the PMD knows what to expect.
Driver side it is OKAY but how the application can update it? The PMD does not
need or does not have a shared object.
Otherway to ask is, Could you have share the API call sequence using
"rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
rte_flow_action *action, error)"
to enable support for the following category of HW as I mentioned earlier.
- The HW has "pattern" and "action" mapped to different HW objects and
action can be updated any time without destroying and create.(a,k,a
Does not have shared HW object)
>
> > enum rte_flow_action_update_type {
> > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> > RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> > };
> >
>
> > >
> > > > > In my suggestion the PMD simply needs to check if the new action and
> > > > change the
> > > > > context and to that action, or just change parameters in the action, if it is
> > the
> > > > same action.
> > > > >
> > > > > Let's go with the original patch API modified to support like you requested
> > > > also changing the action,
> > > > > based on my comments.
> > > > >
> > > > > > rte_flow_action_update(uint16_port port, struct
> > > > > > rte_flow_action_update_type_param *param, error)
> > > > > >
> > > > > > >
> > > [..nip..]
> > >
> > > Best,
> > > Ori
08/07/2020 13:00, Jerin Jacob:
> Driver side it is OKAY but how the application can update it? The PMD does not
> need or does not have a shared object.
>
> Otherway to ask is, Could you have share the API call sequence using
> "rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> rte_flow_action *action, error)"
>
> to enable support for the following category of HW as I mentioned earlier.
> - The HW has "pattern" and "action" mapped to different HW objects and
> action can be updated any time without destroying and create.(a,k,a
> Does not have shared HW object)
Yes an example of API usage may help.
We should get one today.
Hi Jerin and Andrey,
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Subject: Re: [dpdk-dev] [PATCH] add flow shared action API
>
> On Wed, Jul 8, 2020 at 3:17 PM Ori Kam <orika@mellanox.com> wrote:
> >
> > Hi Jerin
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > >
> > > On Wed, Jul 8, 2020 at 2:33 AM Ori Kam <orika@mellanox.com> wrote:
> > > >
> > > > Hi Jerin,
> > >
> > > Hi Ori,
> > >
> > > >
> > > > > -----Original Message-----
> > [..nip ..]
> >
> > > > > > > I think, simple API change would be to accommodate
> "rte_shared_ctx
> > > > > > > *ctx, rte_flow_action *action" modes
> > > > > > > without introducing the emulation for one or other mode, will be.
> > > > > > >
> > > > > > > enum rte_flow_action_update_type {
> > > > > > > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> > > > > > > RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> > > > > > > };
> > > > > > >
> > > > > > > struct rte_flow_action_update_type_param {
> > > > > > > enum rte_flow_action_update_type type;
> > > > > > > union {
> > > > > > > struct
> rte_flow_action_update_type_shared_action_param
> > > {
> > > > > > > rte_shared_ctx *ctx;
> > > > > > > } shared_action;
> > > > > > > struct
> > > rte_flow_action_update_type_shared_action_param {
> > > > > > > rte_flow *flow,
> > > > > > > rte_flow_action *action;
> > > > > > > } action;
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > Thank you for the idea but I fall to see how your suggested API is
> simpler
> > > than
> > > > > the one suggested by me?
> > > > >
> > > > > My thought process with the below-proposed API[1] is that It is
> > > > > dictates "_shared_action_" in API name as
> > > > > well as arguments. I just thought of expressing it as either-or case
> > > > > hence I thought [2] is better. i.e The PMD does not support
> > > > > shared_action, not even need to create one to use
> > > > > rte_flow_action_update() to avoid the confusion. Thoughts?
> > > > >
> > > > > [1]
> > > > > rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> > > > > rte_flow_action *action, error)
> > > > >
> > > > > [2]
> > > > > rte_flow_action_update(uint16_port port, struct
> > > > > rte_flow_action_update_type_param *param, error)
> > > > >
> > > > Let me see if I understand you correctly, your suggestion is to allow
> > > > the application to change one action in one flow, but instead of creating
> > > > the context the application will just supply the rte_flow and the new
> actions
> > > > do I understand correctly?
> > >
> > > Yes.
> > >
> > > >
> > > > If so this it is a nice idea, but there are some issues with it,
> > > > 1. The PMD must save the flow which will result in memory consumption.
> > >
> > > struct rte_flow * driver handle any way store that information to as
> > > it would be needed
> > > for other rte_flow related APIs.
> > >
> > The driver for example in Mellanox case save only the pointer to the flow so
> it can
> > destroy it on request. It can't remove it and add it again since it is missing
> critical
> > info. To save the info will cost memory. I assume this goes to other drivers.
> > There is no real need to save anything but the flow pointer.
>
> If driver _need_ this feature, then the driver needs to store some
> info in the flow object,
> In our case, it will be the index of the MCAM action, etc.
> Yes. it up to driver the how they want to support it.
>
Or to save the flow in the context, but just like you said it is PMD dependent.
>
> >
> > >
> > > > 2. Assume that two flows are using the same RSS action for example, so
> the
> > > PMD
> > > > reuse the RSS object he created for the first flow also for the second. Now
> > > changing
> > > > this RSS flow may result in also changing the second flow. (this can be
> solved
> > > by always
> > > > creating new action)
> > >
> > > It is not resuing the action, it more of updating the action. So the
> > > above said issue won't happen.
> > > It is removing the need for call `rte_flow_destroy()` and call
> > > `rte_flow_create()` if only action
> > > needs to update for THE given flow. That's it.
> > >
> > Again this means that the driver must save all flows so it will waste memory.
> > also this doesn’t save any time, since the application can just do it the same
> way
> > as the PMD there is no value to do it inside the PMD.
>
> Why driver need to save all the flow, We need to save all the flow ONLY when
> we need to emulate the shared context. We need to save only additional
> info like,
> MCAM index where the action was allocated or such, so that it can be
> replaced.
That’s depends on PMD and HW.
>
>
> >
> > >
> > > > 3. It doesn't handle the main use case that the application wants to
> change
> > > number of
> > > > flows at the same time, which is the idea of this feature.
> > >
> > > We discussed this in detail and tried approach for the common code to
> > > make everything
> > > as shared action. Andrey quickly realizes that it is difficult without
> > > HW support.
> > Like everything in RTE flow this is all about HW support
> > If the HW doesn’t support it don't do it.
> >
> > >
> > > >
> > > > I also think that all PMD that support option 2 can support option 1 since
> > > > they can save in the ctx a list of flows and simply apply them again. So by
> > > > definition if PMD supports [2] it also support [1] while the other
> > > > way is not correct since it forces the PMD to save flows which like I said
> > > waste memory.
> > >
> > > If we use "rte_flow_shared_action_update(uint16_port port,
> > > rte_shared_ctx *ctx, rte_flow_action *action, error)",
> > > What would be ctx value for the HW does not support a shared context?
> > > That's is the only reason for
> > > my proposal. I understand, your concern about supporting two modes in
> > > PMD, I don't think,
> > > PMD needs to support RTE_FLOW_ACTION_UPDATE_TYPE_ACTION if
> > > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION supported.
> > >
> > This is the beauty of it ctx is opaque so the PMD can have what ever it wants
> to have. Just like
> > an rte_flow that each PMD have different fields and usage.
> >
> > > >
> > > > I suggest that we will go with option [1], and if needed in the future we
> will
> > > update the code.
> > > > using option [2] will result in dead code since at least for the current time
> no
> > > PMD will implement this
> > > > API.
> > >
> > > We are planning to update our PMD to support this once API is finalized.
> > >
> > Great very happy to hear that.
> > This means that we should push this feature even faster
> >
> > > >
> > > > I can suggest one more thing maybe to change the name from shared_ctx
> to
> > > just ctx
> > > > which implicitly mean it can be shared but not a must. What do you think?
> > > (but again
> > > > I think by definition if a PMD can implement number [2] it can also
> > > implement it to number
> > > > of flows using API [2].
> > >
> > > Just void *type is fine too, but we need an argument for type to cast
> > > it in application and/or driver.
> > >
> > Like said above this is opaque so the PMD knows what to expect.
>
> Driver side it is OKAY but how the application can update it? The PMD does not
> need or does not have a shared object.
>
The PMD is getting the shared object that the PMD created so the PMD has all information.
It is the PMD that control what is in the ctx. The application just saves this pointer and gives it
to the MD on modify and destroy or the creation of flow.
From application point of view he asks the DPDK to change the ctx that it has
to do a different action, the application doesn’t know or care what is inside the
the ctx.
> Otherway to ask is, Could you have share the API call sequence using
> "rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx,
> rte_flow_action *action, error)"
>
I hope that by the end of today Andrey will post such a code.
> to enable support for the following category of HW as I mentioned earlier.
> - The HW has "pattern" and "action" mapped to different HW objects and
> action can be updated any time without destroying and create.(a,k,a
> Does not have shared HW object)
I'm not sure I understand this comment.
Let's advance in the following way.
Andrey please send V2 that changes the API to use rte_flow_action and a sample code
and some PMD code so we can see how it should work.
>
>
> >
> > > enum rte_flow_action_update_type {
> > > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION,
> > > RTE_FLOW_ACTION_UPDATE_TYPE_ACTION,
> > > };
> > >
> >
> > > >
> > > > > > In my suggestion the PMD simply needs to check if the new action and
> > > > > change the
> > > > > > context and to that action, or just change parameters in the action, if
> it is
> > > the
> > > > > same action.
> > > > > >
> > > > > > Let's go with the original patch API modified to support like you
> requested
> > > > > also changing the action,
> > > > > > based on my comments.
> > > > > >
> > > > > > > rte_flow_action_update(uint16_port port, struct
> > > > > > > rte_flow_action_update_type_param *param, error)
> > > > > > >
> > > > > > > >
> > > > [..nip..]
> > > >
> > > > Best,
> > > > Ori
On 08/07/2020 21:40, Andrey Vesnovaty wrote:
> From: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
>
> 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 multiple
> flows
>
> Change description
> ===
>
> Shared action
> ===
> In order to represent flow action shared by multiple flows new action
> type RTE_FLOW_ACTION_TYPE_SHARED is 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 across multiple flows.
>
> Shared action create/use/destroy
> ===
> Shared action may be reused by some or none flow rules at any given
> moment, i.e. shared action reside outside of the context of any flow.
> Shared action represent HW resources/objects used for action offloading
> implementation.
> API for shared action create (see `rte_flow_shared_action_create()`):
> - should allocate HW resources and make related initializations required
> for shared action implementation.
> - make necessary preparations to maintain shared access to
> the action resources, configuration and state.
> API for shared action destroy (see `rte_flow_shared_action_destroy()`)
> should release HW resources and make related cleanups required for shared
> action implementation.
>
> In order to share some flow action reuse the handle of type
> `struct rte_flow_shared_action` 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 any further i.e. result of the usage is
> undefined.
>
> Shared action re-configuration
> ===
> Shared action behavior defined by its configuration can be updated via
> rte_flow_shared_action_update() (see "example" section). The shared
> action update operation modifies HW related resources/objects allocated
> on the action creation. 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 update 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 */
> struct rte_flow_shared_action *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, 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 | 148 ++++++++++++++++++++++-
> lib/librte_ethdev/rte_flow_driver.h | 22 ++++
> 4 files changed, 256 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index 7155056045..119d84976a 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -241,4 +241,10 @@ EXPERIMENTAL {
> __rte_ethdev_trace_rx_burst;
> __rte_ethdev_trace_tx_burst;
> rte_flow_get_aged_flows;
> +
> + # added in 20.08
> + rte_flow_shared_action_create;
> + rte_flow_shared_action_destroy;
> + 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 1685be5f73..0ac4d31a13 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -1250,3 +1250,84 @@ rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> NULL, rte_strerror(ENOTSUP));
> }
> +
> +struct rte_flow_shared_action *
> +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];
> + struct rte_flow_shared_action *shared_action;
> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> +
> + if (unlikely(!ops))
> + return NULL;
> + if (likely(!!ops->shared_action_create)) {
> + shared_action = ops->shared_action_create(dev, action, error);
> + if (shared_action == NULL)
> + flow_err(port_id, -rte_errno, error);
> + return shared_action;
> + }
> + rte_flow_error_set(error, ENOSYS, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> + NULL, rte_strerror(ENOSYS));
> + return NULL;
> +}
> +
> +int
> +rte_flow_shared_action_destroy(uint16_t port_id,
> + struct rte_flow_shared_action *action,
> + 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, action, 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,
> + struct rte_flow_shared_action *action,
> + const struct rte_flow_action *update,
> + 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, action,
> + update, 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 struct rte_flow_shared_action *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 b0e4199192..257456b14a 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1681,7 +1681,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.
> @@ -2099,6 +2100,14 @@ enum rte_flow_action_type {
> * see enum RTE_ETH_EVENT_FLOW_AGED
> */
> RTE_FLOW_ACTION_TYPE_AGE,
> +
> + /**
> + * Describes action shared a cross multiple flow rules.
> + *
> + * Enables multiple rules reference the same action by handle (see
> + * struct rte_flow_shared_action).
> + */
> + RTE_FLOW_ACTION_TYPE_SHARED,
> };
>
> /**
> @@ -2660,6 +2669,20 @@ struct rte_flow_action_set_dscp {
> uint8_t dscp;
> };
>
> +
> +/**
> + * RTE_FLOW_ACTION_TYPE_SHARED
> + *
> + * Opaque type returned after successfully creating a shared action.
> + *
> + * This handle can be used to manage and query the related action:
> + * - share it a cross multiple flow rules
> + * - update action configuration
> + * - query action data
> + * - destroy action
> + */
> +struct rte_flow_shared_action;
> +
> /* Mbuf dynamic field offset for metadata. */
> extern int32_t rte_flow_dynf_metadata_offs;
>
> @@ -3324,6 +3347,129 @@ int
> rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
> uint32_t nb_contexts, 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
> +struct rte_flow_shared_action *
> +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_destroy(uint16_t port_id,
> + struct rte_flow_shared_action *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 *update* 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] update
> + * Action specification used to modify the action pointed by handle.
> + * *update* should be of same type with the action pointed by the *action*
> + * handle argument, otherwise considered as invalid.
> + * @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 *update* invalid.
> + * - (-ENOTSUP) if *update* 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,
> + struct rte_flow_shared_action *action,
> + const struct rte_flow_action *update,
> + 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 struct rte_flow_shared_action *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 881cc469b7..a2cae1b53c 100644
> --- a/lib/librte_ethdev/rte_flow_driver.h
> +++ b/lib/librte_ethdev/rte_flow_driver.h
> @@ -107,6 +107,28 @@ struct rte_flow_ops {
> void **context,
> uint32_t nb_contexts,
> struct rte_flow_error *err);
> + /** See rte_flow_shared_action_create() */
> + struct rte_flow_shared_action *(*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,
> + struct rte_flow_shared_action *shared_action,
> + struct rte_flow_error *error);
> + /** See rte_flow_shared_action_update() */
> + int (*shared_action_update)
> + (struct rte_eth_dev *dev,
> + struct rte_flow_shared_action *shared_action,
> + const struct rte_flow_action *update,
> + struct rte_flow_error *error);
> + /** See rte_flow_shared_action_query() */
> + int (*shared_action_query)
> + (struct rte_eth_dev *dev,
> + const struct rte_flow_shared_action *shared_action,
> + void *data,
> + struct rte_flow_error *error);
> };
>
> /**
>
The modification of "struct rte_flow_ops", looks fine.
Acked-by: Ray Kinsella <mdr@ashroe.eu>
On 08/07/2020 21:40, Andrey Vesnovaty wrote:
> Implement mlx5_devx_cmd_modify_tir() to modify TIR object using DevX
> API.
> Add related structs in mlx5_prm.h.
>
> Signed-off-by: Andrey Vesnovaty <andreyv@mellanox.com>
> ---
> drivers/common/mlx5/mlx5_devx_cmds.c | 84 +++++++++++++++++++
> drivers/common/mlx5/mlx5_devx_cmds.h | 10 +++
> drivers/common/mlx5/mlx5_prm.h | 29 +++++++
> .../common/mlx5/rte_common_mlx5_version.map | 1 +
> 4 files changed, 124 insertions(+)
>
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
> index 2179a83983..2a7098ec6d 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.c
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.c
> @@ -825,6 +825,90 @@ mlx5_devx_cmd_create_tir(void *ctx,
> return tir;
> }
>
> +/**
> + * Modify TIR using DevX API.
> + *
> + * @param[in] tir
> + * Pointer to TIR DevX object structure.
> + * @param [in] modify_tir_attr
> + * Pointer to TIR modification attributes structure.
> + *
> + * @return
> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +int
> +mlx5_devx_cmd_modify_tir(struct mlx5_devx_obj *tir,
> + struct mlx5_devx_modify_tir_attr *modify_tir_attr)
> +{
> + struct mlx5_devx_tir_attr *tir_attr = &modify_tir_attr->tir;
> + uint32_t in[MLX5_ST_SZ_DW(modify_tir_in)] = {0};
> + uint32_t out[MLX5_ST_SZ_DW(modify_tir_out)] = {0};
> + void *tir_ctx;
> + int ret;
> +
> + MLX5_SET(modify_tir_in, in, opcode, MLX5_CMD_OP_MODIFY_TIR);
> + MLX5_SET(modify_tir_in, in, tirn, modify_tir_attr->tirn);
> + MLX5_SET64(modify_tir_in, in, modify_bitmask,
> + modify_tir_attr->modify_bitmask);
> +
> + tir_ctx = MLX5_ADDR_OF(modify_rq_in, in, ctx);
> + if (modify_tir_attr->modify_bitmask &
> + MLX5_MODIFY_TIR_IN_MODIFY_BITMASK_LRO) {
> + MLX5_SET(tirc, tir_ctx, lro_timeout_period_usecs,
> + tir_attr->lro_timeout_period_usecs);
> + MLX5_SET(tirc, tir_ctx, lro_enable_mask,
> + tir_attr->lro_enable_mask);
> + MLX5_SET(tirc, tir_ctx, lro_max_msg_sz,
> + tir_attr->lro_max_msg_sz);
> + }
> + if (modify_tir_attr->modify_bitmask &
> + MLX5_MODIFY_TIR_IN_MODIFY_BITMASK_INDIRECT_TABLE)
> + MLX5_SET(tirc, tir_ctx, indirect_table,
> + tir_attr->indirect_table);
> + if (modify_tir_attr->modify_bitmask &
> + MLX5_MODIFY_TIR_IN_MODIFY_BITMASK_HASH) {
> + int i;
> + void *outer, *inner;
> + MLX5_SET(tirc, tir_ctx, rx_hash_symmetric,
> + tir_attr->rx_hash_symmetric);
> + MLX5_SET(tirc, tir_ctx, rx_hash_fn, tir_attr->rx_hash_fn);
> + for (i = 0; i < 10; i++) {
> + MLX5_SET(tirc, tir_ctx, rx_hash_toeplitz_key[i],
> + tir_attr->rx_hash_toeplitz_key[i]);
> + }
> + outer = MLX5_ADDR_OF(tirc, tir_ctx,
> + rx_hash_field_selector_outer);
> + MLX5_SET(rx_hash_field_select, outer, l3_prot_type,
> + tir_attr->rx_hash_field_selector_outer.l3_prot_type);
> + MLX5_SET(rx_hash_field_select, outer, l4_prot_type,
> + tir_attr->rx_hash_field_selector_outer.l4_prot_type);
> + MLX5_SET
> + (rx_hash_field_select, outer, selected_fields,
> + tir_attr->rx_hash_field_selector_outer.selected_fields);
> + inner = MLX5_ADDR_OF(tirc, tir_ctx,
> + rx_hash_field_selector_inner);
> + MLX5_SET(rx_hash_field_select, inner, l3_prot_type,
> + tir_attr->rx_hash_field_selector_inner.l3_prot_type);
> + MLX5_SET(rx_hash_field_select, inner, l4_prot_type,
> + tir_attr->rx_hash_field_selector_inner.l4_prot_type);
> + MLX5_SET
> + (rx_hash_field_select, inner, selected_fields,
> + tir_attr->rx_hash_field_selector_inner.selected_fields);
> + }
> + if (modify_tir_attr->modify_bitmask &
> + MLX5_MODIFY_TIR_IN_MODIFY_BITMASK_SELF_LB_EN) {
> + MLX5_SET(tirc, tir_ctx, self_lb_block, tir_attr->self_lb_block);
> + }
> + ret = mlx5_glue->devx_obj_modify(tir->obj, in, sizeof(in),
> + out, sizeof(out));
> + if (ret) {
> + DRV_LOG(ERR, "Failed to modify TIR using DevX");
> + rte_errno = errno;
> + return -errno;
> + }
> + return ret;
> +}
> +
> /**
> * Create RQT using DevX API.
> *
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h b/drivers/common/mlx5/mlx5_devx_cmds.h
> index 25704efc1f..9b07b39e66 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.h
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.h
> @@ -178,6 +178,13 @@ struct mlx5_devx_tir_attr {
> struct mlx5_rx_hash_field_select rx_hash_field_selector_inner;
> };
>
> +/* TIR attributes structure, used by TIR modify */
> +struct mlx5_devx_modify_tir_attr {
> + uint32_t tirn:24;
> + uint64_t modify_bitmask;
> + struct mlx5_devx_tir_attr tir;
> +};
> +
> /* RQT attributes structure, used by RQT operations. */
> struct mlx5_devx_rqt_attr {
> uint8_t rq_type;
> @@ -372,6 +379,9 @@ int mlx5_devx_cmd_modify_qp_state(struct mlx5_devx_obj *qp,
> __rte_internal
> int mlx5_devx_cmd_modify_rqt(struct mlx5_devx_obj *rqt,
> struct mlx5_devx_rqt_attr *rqt_attr);
> +__rte_internal
> +int mlx5_devx_cmd_modify_tir(struct mlx5_devx_obj *tir,
> + struct mlx5_devx_modify_tir_attr *tir_attr);
>
> /**
> * Create virtio queue counters object DevX API.
> diff --git a/drivers/common/mlx5/mlx5_prm.h b/drivers/common/mlx5/mlx5_prm.h
> index c63795fc84..029767ea34 100644
> --- a/drivers/common/mlx5/mlx5_prm.h
> +++ b/drivers/common/mlx5/mlx5_prm.h
> @@ -746,6 +746,7 @@ enum {
> MLX5_CMD_OP_QUERY_NIC_VPORT_CONTEXT = 0x754,
> MLX5_CMD_OP_ALLOC_TRANSPORT_DOMAIN = 0x816,
> MLX5_CMD_OP_CREATE_TIR = 0x900,
> + MLX5_CMD_OP_MODIFY_TIR = 0x901,
> MLX5_CMD_OP_CREATE_SQ = 0X904,
> MLX5_CMD_OP_MODIFY_SQ = 0X905,
> MLX5_CMD_OP_CREATE_RQ = 0x908,
> @@ -1753,6 +1754,34 @@ struct mlx5_ifc_create_tir_in_bits {
> struct mlx5_ifc_tirc_bits ctx;
> };
>
> +enum {
> + MLX5_MODIFY_TIR_IN_MODIFY_BITMASK_LRO = 1ULL << 0,
> + MLX5_MODIFY_TIR_IN_MODIFY_BITMASK_INDIRECT_TABLE = 1ULL << 1,
> + MLX5_MODIFY_TIR_IN_MODIFY_BITMASK_HASH = 1ULL << 2,
> + /* bit 3 - tunneled_offload_en modify not supported */
> + MLX5_MODIFY_TIR_IN_MODIFY_BITMASK_SELF_LB_EN = 1ULL << 4,
> +};
> +
> +struct mlx5_ifc_modify_tir_out_bits {
> + u8 status[0x8];
> + u8 reserved_at_8[0x18];
> + u8 syndrome[0x20];
> + u8 reserved_at_40[0x40];
> +};
> +
> +struct mlx5_ifc_modify_tir_in_bits {
> + u8 opcode[0x10];
> + u8 uid[0x10];
> + u8 reserved_at_20[0x10];
> + u8 op_mod[0x10];
> + u8 reserved_at_40[0x8];
> + u8 tirn[0x18];
> + u8 reserved_at_60[0x20];
> + u8 modify_bitmask[0x40];
> + u8 reserved_at_c0[0x40];
> + struct mlx5_ifc_tirc_bits ctx;
> +};
> +
> enum {
> MLX5_INLINE_Q_TYPE_RQ = 0x0,
> MLX5_INLINE_Q_TYPE_VIRTQ = 0x1,
> diff --git a/drivers/common/mlx5/rte_common_mlx5_version.map b/drivers/common/mlx5/rte_common_mlx5_version.map
> index ae57ebdba5..0bfedab32f 100644
> --- a/drivers/common/mlx5/rte_common_mlx5_version.map
> +++ b/drivers/common/mlx5/rte_common_mlx5_version.map
> @@ -29,6 +29,7 @@ INTERNAL {
> mlx5_devx_cmd_modify_rq;
> mlx5_devx_cmd_modify_rqt;
> mlx5_devx_cmd_modify_sq;
> + mlx5_devx_cmd_modify_tir;
> mlx5_devx_cmd_modify_virtq;
> mlx5_devx_cmd_qp_query_tis_td;
> mlx5_devx_cmd_query_hca_attr;
Acked-by: Ray Kinsella <mdr@ashroe.eu>
On 7/13/20 11:04 AM, Kinsella, Ray wrote:
>
>
> On 08/07/2020 21:40, Andrey Vesnovaty wrote:
>> From: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
>>
>> 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
It *allows* to reduce the overhead of multiple RSS flow rules
reconfiguration. I.e. it is not mandatory. PMD may do it in SW,
if HW does not support it. I see no problem to allow it.
Even if it is done in PMD, it is already an optimization since
applications have unified interface and internally it should
be cheaper to do it.
I'd consider to implement generic SW helper API for PMDs which
cannot have shared actions in HW, but would like to simulate it
in SW. It would allow to avoid the fallback in applications.
>> - optimize resource utilization by sharing action across 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 is 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 across multiple flows.
>>
>> Shared action create/use/destroy
>> ===
>> Shared action may be reused by some or none flow rules at any given
>> moment, i.e. shared action reside outside of the context of any flow.
>> Shared action represent HW resources/objects used for action offloading
>> implementation.
>> API for shared action create (see `rte_flow_shared_action_create()`):
>> - should allocate HW resources and make related initializations required
>> for shared action implementation.
>> - make necessary preparations to maintain shared access to
>> the action resources, configuration and state.
>> API for shared action destroy (see `rte_flow_shared_action_destroy()`)
>> should release HW resources and make related cleanups required for shared
>> action implementation.
>>
>> In order to share some flow action reuse the handle of type
>> `struct rte_flow_shared_action` 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 any further i.e. result of the usage is
>> undefined.
May be it makes sense to implement housekeeping in ethdev
layer? I.e. guarantee consequency using reference counters etc.
Will applications benefit from it?
>>
>> Shared action re-configuration
>> ===
>> Shared action behavior defined by its configuration can be updated via
>> rte_flow_shared_action_update() (see "example" section). The shared
>> action update operation modifies HW related resources/objects allocated
>> on the action creation. 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 update API action behavior should be
>> according to updated configuration for all flows sharing the action.
If shared action is simulated in SW, number of operations to do
reconfiguration will depend on a number of flow rules using it.
I think it is not a problem. So, *should not* used above is OK.
Consider:
"should not be dependent on" -> "should not depend on"
>>
>> Shared action query
>> ===
>> Provide separate API to query shared action sate (see
sate -> state
>> 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 */
>> struct rte_flow_shared_action *handle = rte_flow_shared_action_create(
>> port_id, &action, &error);
It should be possible to have many actions in shared action.
I.e. similar to below, it should be an array terminated by
RTE_FLOW_ACTION_TYPE_END. It is unclear from the example
above.
>> 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, 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 | 148 ++++++++++++++++++++++-
>> lib/librte_ethdev/rte_flow_driver.h | 22 ++++
>> 4 files changed, 256 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
>> index 7155056045..119d84976a 100644
>> --- a/lib/librte_ethdev/rte_ethdev_version.map
>> +++ b/lib/librte_ethdev/rte_ethdev_version.map
>> @@ -241,4 +241,10 @@ EXPERIMENTAL {
>> __rte_ethdev_trace_rx_burst;
>> __rte_ethdev_trace_tx_burst;
>> rte_flow_get_aged_flows;
>> +
>> + # added in 20.08
>> + rte_flow_shared_action_create;
>> + rte_flow_shared_action_destroy;
>> + 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 1685be5f73..0ac4d31a13 100644
>> --- a/lib/librte_ethdev/rte_flow.c
>> +++ b/lib/librte_ethdev/rte_flow.c
>> @@ -1250,3 +1250,84 @@ rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
>> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>> NULL, rte_strerror(ENOTSUP));
>> }
>> +
>> +struct rte_flow_shared_action *
>> +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];
>> + struct rte_flow_shared_action *shared_action;
>> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
>> +
>> + if (unlikely(!ops))
>> + return NULL;
>> + if (likely(!!ops->shared_action_create)) {
>> + shared_action = ops->shared_action_create(dev, action, error);
>> + if (shared_action == NULL)
>> + flow_err(port_id, -rte_errno, error);
>> + return shared_action;
>> + }
>> + rte_flow_error_set(error, ENOSYS, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>> + NULL, rte_strerror(ENOSYS));
>> + return NULL;
>> +}
>> +
>> +int
>> +rte_flow_shared_action_destroy(uint16_t port_id,
>> + struct rte_flow_shared_action *action,
>> + 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, action, 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,
>> + struct rte_flow_shared_action *action,
>> + const struct rte_flow_action *update,
>> + 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, action,
>> + update, 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 struct rte_flow_shared_action *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 b0e4199192..257456b14a 100644
>> --- a/lib/librte_ethdev/rte_flow.h
>> +++ b/lib/librte_ethdev/rte_flow.h
>> @@ -1681,7 +1681,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.
>> @@ -2099,6 +2100,14 @@ enum rte_flow_action_type {
>> * see enum RTE_ETH_EVENT_FLOW_AGED
>> */
>> RTE_FLOW_ACTION_TYPE_AGE,
>> +
>> + /**
>> + * Describes action shared a cross multiple flow rules.
Both options are possible, but I'd propose to stick to:
Describes -> Describe
Or even:
Use actions shared across multiple flow rules.
>> + *
>> + * Enables multiple rules reference the same action by handle (see
Enables -> Allow
>> + * struct rte_flow_shared_action).
>> + */
>> + RTE_FLOW_ACTION_TYPE_SHARED,
>> };
>>
>> /**
>> @@ -2660,6 +2669,20 @@ struct rte_flow_action_set_dscp {
>> uint8_t dscp;
>> };
>>
>> +
>> +/**
>> + * RTE_FLOW_ACTION_TYPE_SHARED
>> + *
>> + * Opaque type returned after successfully creating a shared action.
>> + *
>> + * This handle can be used to manage and query the related action:
>> + * - share it a cross multiple flow rules
>> + * - update action configuration
>> + * - query action data
>> + * - destroy action
>> + */
>> +struct rte_flow_shared_action;
>> +
>> /* Mbuf dynamic field offset for metadata. */
>> extern int32_t rte_flow_dynf_metadata_offs;
>>
>> @@ -3324,6 +3347,129 @@ int
>> rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
>> uint32_t nb_contexts, 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
>> +struct rte_flow_shared_action *
>> +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.
Destroys -> Destroy
>> + *
>> + * @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_destroy(uint16_t port_id,
>> + struct rte_flow_shared_action *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
Updates -> Update
inplace -> in-place
>> + * with the configuration provided as *update* argument.
>> + * The update of the shared action configuration effects all flow rules reusing
May be: reusing -> using
>> + * the action via handle.
The interesting question what to do, if some rule cannot have a
new action (i.e. new action is invalid for a rule).
May be it is better to highlight it.
>> + *
>> + * @param[in] port_id
>> + * The port identifier of the Ethernet device.
>> + * @param[in] action
>> + * Handle for the shared action to be updated.
>> + * @param[in] update
>> + * Action specification used to modify the action pointed by handle.
>> + * *update* should be of same type with the action pointed by the *action*
>> + * handle argument, otherwise considered as invalid.
Why? If it is a generic rule, it should be checked on a generic
ethdev layer, but I would not impose the limitation. If PMD
may change it, why generic interface specification should
restrict it.
>> + * @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 *update* invalid.
>> + * - (-ENOTSUP) if *update* 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,
>> + struct rte_flow_shared_action *action,
>> + const struct rte_flow_action *update,
>> + 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.
"This function allows retrieving" -> Retrieve or Get or Query
>> + * 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 struct rte_flow_shared_action *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 881cc469b7..a2cae1b53c 100644
>> --- a/lib/librte_ethdev/rte_flow_driver.h
>> +++ b/lib/librte_ethdev/rte_flow_driver.h
>> @@ -107,6 +107,28 @@ struct rte_flow_ops {
>> void **context,
>> uint32_t nb_contexts,
>> struct rte_flow_error *err);
>> + /** See rte_flow_shared_action_create() */
>> + struct rte_flow_shared_action *(*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,
>> + struct rte_flow_shared_action *shared_action,
>> + struct rte_flow_error *error);
>> + /** See rte_flow_shared_action_update() */
>> + int (*shared_action_update)
>> + (struct rte_eth_dev *dev,
>> + struct rte_flow_shared_action *shared_action,
>> + const struct rte_flow_action *update,
>> + struct rte_flow_error *error);
>> + /** See rte_flow_shared_action_query() */
>> + int (*shared_action_query)
>> + (struct rte_eth_dev *dev,
>> + const struct rte_flow_shared_action *shared_action,
>> + void *data,
>> + struct rte_flow_error *error);
>> };
>>
>> /**
>>
>
> The modification of "struct rte_flow_ops", looks fine.
>
> Acked-by: Ray Kinsella <mdr@ashroe.eu>
>
On 7/13/20 11:04 AM, Kinsella, Ray wrote:
>
>
> On 08/07/2020 21:40, Andrey Vesnovaty wrote:
>> From: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
>>
>> 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
It *allows* to reduce the overhead of multiple RSS flow rules
reconfiguration. I.e. it is not mandatory. PMD may do it in SW,
if HW does not support it. I see no problem to allow it.
Even if it is done in PMD, it is already an optimization since
applications have unified interface and internally it should
be cheaper to do it.
I'd consider to implement generic SW helper API for PMDs which
cannot have shared actions in HW, but would like to simulate it
in SW. It would allow to avoid the fallback in applications.
>> - optimize resource utilization by sharing action across 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 is 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 across multiple flows.
>>
>> Shared action create/use/destroy
>> ===
>> Shared action may be reused by some or none flow rules at any given
>> moment, i.e. shared action reside outside of the context of any flow.
>> Shared action represent HW resources/objects used for action offloading
>> implementation.
>> API for shared action create (see `rte_flow_shared_action_create()`):
>> - should allocate HW resources and make related initializations required
>> for shared action implementation.
>> - make necessary preparations to maintain shared access to
>> the action resources, configuration and state.
>> API for shared action destroy (see `rte_flow_shared_action_destroy()`)
>> should release HW resources and make related cleanups required for shared
>> action implementation.
>>
>> In order to share some flow action reuse the handle of type
>> `struct rte_flow_shared_action` 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 any further i.e. result of the usage is
>> undefined.
May be it makes sense to implement housekeeping in ethdev
layer? I.e. guarantee consequency using reference counters etc.
Will applications benefit from it?
>>
>> Shared action re-configuration
>> ===
>> Shared action behavior defined by its configuration can be updated via
>> rte_flow_shared_action_update() (see "example" section). The shared
>> action update operation modifies HW related resources/objects allocated
>> on the action creation. 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 update API action behavior should be
>> according to updated configuration for all flows sharing the action.
If shared action is simulated in SW, number of operations to do
reconfiguration will depend on a number of flow rules using it.
I think it is not a problem. So, *should not* used above is OK.
Consider:
"should not be dependent on" -> "should not depend on"
>>
>> Shared action query
>> ===
>> Provide separate API to query shared action sate (see
sate -> state
>> 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 */
>> struct rte_flow_shared_action *handle = rte_flow_shared_action_create(
>> port_id, &action, &error);
It should be possible to have many actions in shared action.
I.e. similar to below, it should be an array terminated by
RTE_FLOW_ACTION_TYPE_END. It is unclear from the example
above.
>> 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, 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 | 148 ++++++++++++++++++++++-
>> lib/librte_ethdev/rte_flow_driver.h | 22 ++++
>> 4 files changed, 256 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev_version.map
>> b/lib/librte_ethdev/rte_ethdev_version.map
>> index 7155056045..119d84976a 100644
>> --- a/lib/librte_ethdev/rte_ethdev_version.map
>> +++ b/lib/librte_ethdev/rte_ethdev_version.map
>> @@ -241,4 +241,10 @@ EXPERIMENTAL {
>> __rte_ethdev_trace_rx_burst;
>> __rte_ethdev_trace_tx_burst;
>> rte_flow_get_aged_flows;
>> +
>> + # added in 20.08
>> + rte_flow_shared_action_create;
>> + rte_flow_shared_action_destroy;
>> + 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 1685be5f73..0ac4d31a13 100644
>> --- a/lib/librte_ethdev/rte_flow.c
>> +++ b/lib/librte_ethdev/rte_flow.c
>> @@ -1250,3 +1250,84 @@ rte_flow_get_aged_flows(uint16_t port_id, void
>> **contexts,
>> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>> NULL, rte_strerror(ENOTSUP));
>> }
>> +
>> +struct rte_flow_shared_action *
>> +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];
>> + struct rte_flow_shared_action *shared_action;
>> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
>> +
>> + if (unlikely(!ops))
>> + return NULL;
>> + if (likely(!!ops->shared_action_create)) {
>> + shared_action = ops->shared_action_create(dev, action, error);
>> + if (shared_action == NULL)
>> + flow_err(port_id, -rte_errno, error);
>> + return shared_action;
>> + }
>> + rte_flow_error_set(error, ENOSYS, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>> + NULL, rte_strerror(ENOSYS));
>> + return NULL;
>> +}
>> +
>> +int
>> +rte_flow_shared_action_destroy(uint16_t port_id,
>> + struct rte_flow_shared_action *action,
>> + 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, action, 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,
>> + struct rte_flow_shared_action *action,
>> + const struct rte_flow_action *update,
>> + 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, action,
>> + update, 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 struct rte_flow_shared_action *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 b0e4199192..257456b14a 100644
>> --- a/lib/librte_ethdev/rte_flow.h
>> +++ b/lib/librte_ethdev/rte_flow.h
>> @@ -1681,7 +1681,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.
>> @@ -2099,6 +2100,14 @@ enum rte_flow_action_type {
>> * see enum RTE_ETH_EVENT_FLOW_AGED
>> */
>> RTE_FLOW_ACTION_TYPE_AGE,
>> +
>> + /**
>> + * Describes action shared a cross multiple flow rules.
Both options are possible, but I'd propose to stick to:
Describes -> Describe
Or even:
Use actions shared across multiple flow rules.
>> + *
>> + * Enables multiple rules reference the same action by handle (see
Enables -> Allow
>> + * struct rte_flow_shared_action).
>> + */
>> + RTE_FLOW_ACTION_TYPE_SHARED,
>> };
>> /**
>> @@ -2660,6 +2669,20 @@ struct rte_flow_action_set_dscp {
>> uint8_t dscp;
>> };
>> +
>> +/**
>> + * RTE_FLOW_ACTION_TYPE_SHARED
>> + *
>> + * Opaque type returned after successfully creating a shared action.
>> + *
>> + * This handle can be used to manage and query the related action:
>> + * - share it a cross multiple flow rules
>> + * - update action configuration
>> + * - query action data
>> + * - destroy action
>> + */
>> +struct rte_flow_shared_action;
>> +
>> /* Mbuf dynamic field offset for metadata. */
>> extern int32_t rte_flow_dynf_metadata_offs;
>> @@ -3324,6 +3347,129 @@ int
>> rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
>> uint32_t nb_contexts, 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
>> +struct rte_flow_shared_action *
>> +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.
Destroys -> Destroy
>> + *
>> + * @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_destroy(uint16_t port_id,
>> + struct rte_flow_shared_action *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
Updates -> Update
inplace -> in-place
>> + * with the configuration provided as *update* argument.
>> + * The update of the shared action configuration effects all flow
>> rules reusing
May be: reusing -> using
>> + * the action via handle.
The interesting question what to do, if some rule cannot have a
new action (i.e. new action is invalid for a rule).
May be it is better to highlight it.
>> + *
>> + * @param[in] port_id
>> + * The port identifier of the Ethernet device.
>> + * @param[in] action
>> + * Handle for the shared action to be updated.
>> + * @param[in] update
>> + * Action specification used to modify the action pointed by handle.
>> + * *update* should be of same type with the action pointed by the
>> *action*
>> + * handle argument, otherwise considered as invalid.
Why? If it is a generic rule, it should be checked on a generic
ethdev layer, but I would not impose the limitation. If PMD
may change it, why generic interface specification should
restrict it.
>> + * @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 *update* invalid.
>> + * - (-ENOTSUP) if *update* 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,
>> + struct rte_flow_shared_action *action,
>> + const struct rte_flow_action *update,
>> + 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.
"This function allows retrieving" -> Retrieve or Get or Query
>> + * 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 struct rte_flow_shared_action *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 881cc469b7..a2cae1b53c 100644
>> --- a/lib/librte_ethdev/rte_flow_driver.h
>> +++ b/lib/librte_ethdev/rte_flow_driver.h
>> @@ -107,6 +107,28 @@ struct rte_flow_ops {
>> void **context,
>> uint32_t nb_contexts,
>> struct rte_flow_error *err);
>> + /** See rte_flow_shared_action_create() */
>> + struct rte_flow_shared_action *(*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,
>> + struct rte_flow_shared_action *shared_action,
>> + struct rte_flow_error *error);
>> + /** See rte_flow_shared_action_update() */
>> + int (*shared_action_update)
>> + (struct rte_eth_dev *dev,
>> + struct rte_flow_shared_action *shared_action,
>> + const struct rte_flow_action *update,
>> + struct rte_flow_error *error);
>> + /** See rte_flow_shared_action_query() */
>> + int (*shared_action_query)
>> + (struct rte_eth_dev *dev,
>> + const struct rte_flow_shared_action *shared_action,
>> + void *data,
>> + struct rte_flow_error *error);
>> };
>> /**
>>
>
> The modification of "struct rte_flow_ops", looks fine.
> Acked-by: Ray Kinsella <mdr@ashroe.eu>
>
I'm sorry for a duplicate of my message from July, 13.
The problem is that I've not found my reply in patchwork and
mail thread. Now I found out that I'm replying to Ray's mail
and the mail is not in the thread since has invalid message
headers.
Andrew.
On 7/15/20 11:54 AM, Andrew Rybchenko wrote:
> On 7/13/20 11:04 AM, Kinsella, Ray wrote:
>>
>>
>> On 08/07/2020 21:40, Andrey Vesnovaty wrote:
>>> From: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
>>>
>>> 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
>
> It *allows* to reduce the overhead of multiple RSS flow rules
> reconfiguration. I.e. it is not mandatory. PMD may do it in SW,
> if HW does not support it. I see no problem to allow it.
> Even if it is done in PMD, it is already an optimization since
> applications have unified interface and internally it should
> be cheaper to do it.
> I'd consider to implement generic SW helper API for PMDs which
> cannot have shared actions in HW, but would like to simulate it
> in SW. It would allow to avoid the fallback in applications.
>
>>> - optimize resource utilization by sharing action across 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 is 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 across multiple flows.
>>>
>>> Shared action create/use/destroy
>>> ===
>>> Shared action may be reused by some or none flow rules at any given
>>> moment, i.e. shared action reside outside of the context of any flow.
>>> Shared action represent HW resources/objects used for action offloading
>>> implementation.
>>> API for shared action create (see `rte_flow_shared_action_create()`):
>>> - should allocate HW resources and make related initializations required
>>> for shared action implementation.
>>> - make necessary preparations to maintain shared access to
>>> the action resources, configuration and state.
>>> API for shared action destroy (see `rte_flow_shared_action_destroy()`)
>>> should release HW resources and make related cleanups required for shared
>>> action implementation.
>>>
>>> In order to share some flow action reuse the handle of type
>>> `struct rte_flow_shared_action` 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 any further i.e. result of the usage is
>>> undefined.
>
> May be it makes sense to implement housekeeping in ethdev
> layer? I.e. guarantee consequency using reference counters etc.
> Will applications benefit from it?
>
>>>
>>> Shared action re-configuration
>>> ===
>>> Shared action behavior defined by its configuration can be updated via
>>> rte_flow_shared_action_update() (see "example" section). The shared
>>> action update operation modifies HW related resources/objects allocated
>>> on the action creation. 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 update API action behavior should be
>>> according to updated configuration for all flows sharing the action.
>
> If shared action is simulated in SW, number of operations to do
> reconfiguration will depend on a number of flow rules using it.
> I think it is not a problem. So, *should not* used above is OK.
>
> Consider:
> "should not be dependent on" -> "should not depend on"
>
>>>
>>> Shared action query
>>> ===
>>> Provide separate API to query shared action sate (see
>
> sate -> state
>
>>> 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 */
>>> struct rte_flow_shared_action *handle = rte_flow_shared_action_create(
>>> port_id, &action, &error);
>
> It should be possible to have many actions in shared action.
> I.e. similar to below, it should be an array terminated by
> RTE_FLOW_ACTION_TYPE_END. It is unclear from the example
> above.
>
>>> 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, 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 | 148 ++++++++++++++++++++++-
>>> lib/librte_ethdev/rte_flow_driver.h | 22 ++++
>>> 4 files changed, 256 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev_version.map
>>> b/lib/librte_ethdev/rte_ethdev_version.map
>>> index 7155056045..119d84976a 100644
>>> --- a/lib/librte_ethdev/rte_ethdev_version.map
>>> +++ b/lib/librte_ethdev/rte_ethdev_version.map
>>> @@ -241,4 +241,10 @@ EXPERIMENTAL {
>>> __rte_ethdev_trace_rx_burst;
>>> __rte_ethdev_trace_tx_burst;
>>> rte_flow_get_aged_flows;
>>> +
>>> + # added in 20.08
>>> + rte_flow_shared_action_create;
>>> + rte_flow_shared_action_destroy;
>>> + 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 1685be5f73..0ac4d31a13 100644
>>> --- a/lib/librte_ethdev/rte_flow.c
>>> +++ b/lib/librte_ethdev/rte_flow.c
>>> @@ -1250,3 +1250,84 @@ rte_flow_get_aged_flows(uint16_t port_id, void
>>> **contexts,
>>> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>> NULL, rte_strerror(ENOTSUP));
>>> }
>>> +
>>> +struct rte_flow_shared_action *
>>> +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];
>>> + struct rte_flow_shared_action *shared_action;
>>> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
>>> +
>>> + if (unlikely(!ops))
>>> + return NULL;
>>> + if (likely(!!ops->shared_action_create)) {
>>> + shared_action = ops->shared_action_create(dev, action, error);
>>> + if (shared_action == NULL)
>>> + flow_err(port_id, -rte_errno, error);
>>> + return shared_action;
>>> + }
>>> + rte_flow_error_set(error, ENOSYS, RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>>> + NULL, rte_strerror(ENOSYS));
>>> + return NULL;
>>> +}
>>> +
>>> +int
>>> +rte_flow_shared_action_destroy(uint16_t port_id,
>>> + struct rte_flow_shared_action *action,
>>> + 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, action, 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,
>>> + struct rte_flow_shared_action *action,
>>> + const struct rte_flow_action *update,
>>> + 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, action,
>>> + update, 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 struct rte_flow_shared_action *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 b0e4199192..257456b14a 100644
>>> --- a/lib/librte_ethdev/rte_flow.h
>>> +++ b/lib/librte_ethdev/rte_flow.h
>>> @@ -1681,7 +1681,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.
>>> @@ -2099,6 +2100,14 @@ enum rte_flow_action_type {
>>> * see enum RTE_ETH_EVENT_FLOW_AGED
>>> */
>>> RTE_FLOW_ACTION_TYPE_AGE,
>>> +
>>> + /**
>>> + * Describes action shared a cross multiple flow rules.
>
> Both options are possible, but I'd propose to stick to:
> Describes -> Describe
>
> Or even:
> Use actions shared across multiple flow rules.
>
>>> + *
>>> + * Enables multiple rules reference the same action by handle (see
>
> Enables -> Allow
>
>>> + * struct rte_flow_shared_action).
>>> + */
>>> + RTE_FLOW_ACTION_TYPE_SHARED,
>>> };
>>> /**
>>> @@ -2660,6 +2669,20 @@ struct rte_flow_action_set_dscp {
>>> uint8_t dscp;
>>> };
>>> +
>>> +/**
>>> + * RTE_FLOW_ACTION_TYPE_SHARED
>>> + *
>>> + * Opaque type returned after successfully creating a shared action.
>>> + *
>>> + * This handle can be used to manage and query the related action:
>>> + * - share it a cross multiple flow rules
>>> + * - update action configuration
>>> + * - query action data
>>> + * - destroy action
>>> + */
>>> +struct rte_flow_shared_action;
>>> +
>>> /* Mbuf dynamic field offset for metadata. */
>>> extern int32_t rte_flow_dynf_metadata_offs;
>>> @@ -3324,6 +3347,129 @@ int
>>> rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
>>> uint32_t nb_contexts, 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
>>> +struct rte_flow_shared_action *
>>> +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.
>
> Destroys -> Destroy
>
>>> + *
>>> + * @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_destroy(uint16_t port_id,
>>> + struct rte_flow_shared_action *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
>
> Updates -> Update
> inplace -> in-place
>
>>> + * with the configuration provided as *update* argument.
>>> + * The update of the shared action configuration effects all flow
>>> rules reusing
>
> May be: reusing -> using
>
>>> + * the action via handle.
>
> The interesting question what to do, if some rule cannot have a
> new action (i.e. new action is invalid for a rule).
> May be it is better to highlight it.
>
>>> + *
>>> + * @param[in] port_id
>>> + * The port identifier of the Ethernet device.
>>> + * @param[in] action
>>> + * Handle for the shared action to be updated.
>>> + * @param[in] update
>>> + * Action specification used to modify the action pointed by handle.
>>> + * *update* should be of same type with the action pointed by the
>>> *action*
>>> + * handle argument, otherwise considered as invalid.
>
> Why? If it is a generic rule, it should be checked on a generic
> ethdev layer, but I would not impose the limitation. If PMD
> may change it, why generic interface specification should
> restrict it.
>
>>> + * @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 *update* invalid.
>>> + * - (-ENOTSUP) if *update* 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,
>>> + struct rte_flow_shared_action *action,
>>> + const struct rte_flow_action *update,
>>> + 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.
>
> "This function allows retrieving" -> Retrieve or Get or Query
>
>>> + * 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 struct rte_flow_shared_action *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 881cc469b7..a2cae1b53c 100644
>>> --- a/lib/librte_ethdev/rte_flow_driver.h
>>> +++ b/lib/librte_ethdev/rte_flow_driver.h
>>> @@ -107,6 +107,28 @@ struct rte_flow_ops {
>>> void **context,
>>> uint32_t nb_contexts,
>>> struct rte_flow_error *err);
>>> + /** See rte_flow_shared_action_create() */
>>> + struct rte_flow_shared_action *(*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,
>>> + struct rte_flow_shared_action *shared_action,
>>> + struct rte_flow_error *error);
>>> + /** See rte_flow_shared_action_update() */
>>> + int (*shared_action_update)
>>> + (struct rte_eth_dev *dev,
>>> + struct rte_flow_shared_action *shared_action,
>>> + const struct rte_flow_action *update,
>>> + struct rte_flow_error *error);
>>> + /** See rte_flow_shared_action_query() */
>>> + int (*shared_action_query)
>>> + (struct rte_eth_dev *dev,
>>> + const struct rte_flow_shared_action *shared_action,
>>> + void *data,
>>> + struct rte_flow_error *error);
>>> };
>>> /**
>>>
>>
>> The modification of "struct rte_flow_ops", looks fine.
>> Acked-by: Ray Kinsella <mdr@ashroe.eu>
>>
>
Hi, Andrew,
I'm back to this patch set to make it into 20.11.
Most of the items you raised regarding SW implementation already discussed
with Jerin [1] and answered by Ori [2] and me [3]. Please follow my & Ori
answer. In general, the idea of SW implementation for shared action looks
nice from application perspective but the changes required in ethdev/rte_flow
generic layer about to impact existing rte_flow APIs.
Please follow my [3] & Ori's [2] answers & update on your suggestion to the
Proposed API.
I'll take your suggestions for rephrasing in upcoming patches version (V3).
Thanks,
Andrey
---
[1] https://www.mail-archive.com/dev@dpdk.org/msg173461.html
[2] https://www.mail-archive.com/dev@dpdk.org/msg173478.html
[3] https://www.mail-archive.com/dev@dpdk.org/msg172921.html
> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Wednesday, July 15, 2020 11:54 AM
> To: Andrey Vesnovaty <andreyv@mellanox.com>; Neil Horman
> <nhorman@tuxdriver.com>; Thomas Monjalon <thomas@monjalon.net>;
> Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Ori Kam <orika@mellanox.com>
> Cc: Kinsella, Ray <mdr@ashroe.eu>; dev@dpdk.org; jerinjacobk@gmail.com;
> stephen@networkplumber.org; bruce.richardson@intel.com; Slava Ovsiienko
> <viacheslavo@mellanox.com>; andrey.vesnovaty@gmail.com
> Subject: Re: [dpdk-dev] [PATCH v2 1/6] ethdev: add flow shared action API
> Importance: High
>
> On 7/13/20 11:04 AM, Kinsella, Ray wrote:
> >
> >
> > On 08/07/2020 21:40, Andrey Vesnovaty wrote:
> >> From: Andrey Vesnovaty <andrey.vesnovaty@gmail.com>
> >>
> >> 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
>
> It *allows* to reduce the overhead of multiple RSS flow rules
> reconfiguration. I.e. it is not mandatory. PMD may do it in SW,
> if HW does not support it. I see no problem to allow it.
> Even if it is done in PMD, it is already an optimization since
> applications have unified interface and internally it should
> be cheaper to do it.
> I'd consider to implement generic SW helper API for PMDs which
> cannot have shared actions in HW, but would like to simulate it
> in SW. It would allow to avoid the fallback in applications.
>
> >> - optimize resource utilization by sharing action across 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 is 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 across multiple flows.
> >>
> >> Shared action create/use/destroy
> >> ===
> >> Shared action may be reused by some or none flow rules at any given
> >> moment, i.e. shared action reside outside of the context of any flow.
> >> Shared action represent HW resources/objects used for action offloading
> >> implementation.
> >> API for shared action create (see `rte_flow_shared_action_create()`):
> >> - should allocate HW resources and make related initializations required
> >> for shared action implementation.
> >> - make necessary preparations to maintain shared access to
> >> the action resources, configuration and state.
> >> API for shared action destroy (see `rte_flow_shared_action_destroy()`)
> >> should release HW resources and make related cleanups required for shared
> >> action implementation.
> >>
> >> In order to share some flow action reuse the handle of type
> >> `struct rte_flow_shared_action` 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 any further i.e. result of the usage is
> >> undefined.
>
> May be it makes sense to implement housekeeping in ethdev
> layer? I.e. guarantee consequency using reference counters etc.
> Will applications benefit from it?
>
> >>
> >> Shared action re-configuration
> >> ===
> >> Shared action behavior defined by its configuration can be updated via
> >> rte_flow_shared_action_update() (see "example" section). The shared
> >> action update operation modifies HW related resources/objects allocated
> >> on the action creation. 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 update API action behavior should be
> >> according to updated configuration for all flows sharing the action.
>
> If shared action is simulated in SW, number of operations to do
> reconfiguration will depend on a number of flow rules using it.
> I think it is not a problem. So, *should not* used above is OK.
>
> Consider:
> "should not be dependent on" -> "should not depend on"
>
> >>
> >> Shared action query
> >> ===
> >> Provide separate API to query shared action sate (see
>
> sate -> state
>
> >> 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 */
> >> struct rte_flow_shared_action *handle = rte_flow_shared_action_create(
> >> port_id, &action, &error);
>
> It should be possible to have many actions in shared action.
> I.e. similar to below, it should be an array terminated by
> RTE_FLOW_ACTION_TYPE_END. It is unclear from the example
> above.
>
> >> 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, 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 | 148 ++++++++++++++++++++++-
> >> lib/librte_ethdev/rte_flow_driver.h | 22 ++++
> >> 4 files changed, 256 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/librte_ethdev/rte_ethdev_version.map
> >> b/lib/librte_ethdev/rte_ethdev_version.map
> >> index 7155056045..119d84976a 100644
> >> --- a/lib/librte_ethdev/rte_ethdev_version.map
> >> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> >> @@ -241,4 +241,10 @@ EXPERIMENTAL {
> >> __rte_ethdev_trace_rx_burst;
> >> __rte_ethdev_trace_tx_burst;
> >> rte_flow_get_aged_flows;
> >> +
> >> + # added in 20.08
> >> + rte_flow_shared_action_create;
> >> + rte_flow_shared_action_destroy;
> >> + 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 1685be5f73..0ac4d31a13 100644
> >> --- a/lib/librte_ethdev/rte_flow.c
> >> +++ b/lib/librte_ethdev/rte_flow.c
> >> @@ -1250,3 +1250,84 @@ rte_flow_get_aged_flows(uint16_t port_id, void
> >> **contexts,
> >> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >> NULL, rte_strerror(ENOTSUP));
> >> }
> >> +
> >> +struct rte_flow_shared_action *
> >> +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];
> >> + struct rte_flow_shared_action *shared_action;
> >> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> >> +
> >> + if (unlikely(!ops))
> >> + return NULL;
> >> + if (likely(!!ops->shared_action_create)) {
> >> + shared_action = ops->shared_action_create(dev, action, error);
> >> + if (shared_action == NULL)
> >> + flow_err(port_id, -rte_errno, error);
> >> + return shared_action;
> >> + }
> >> + rte_flow_error_set(error, ENOSYS,
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >> + NULL, rte_strerror(ENOSYS));
> >> + return NULL;
> >> +}
> >> +
> >> +int
> >> +rte_flow_shared_action_destroy(uint16_t port_id,
> >> + struct rte_flow_shared_action *action,
> >> + 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, action, 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,
> >> + struct rte_flow_shared_action *action,
> >> + const struct rte_flow_action *update,
> >> + 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, action,
> >> + update, 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 struct rte_flow_shared_action *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 b0e4199192..257456b14a 100644
> >> --- a/lib/librte_ethdev/rte_flow.h
> >> +++ b/lib/librte_ethdev/rte_flow.h
> >> @@ -1681,7 +1681,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.
> >> @@ -2099,6 +2100,14 @@ enum rte_flow_action_type {
> >> * see enum RTE_ETH_EVENT_FLOW_AGED
> >> */
> >> RTE_FLOW_ACTION_TYPE_AGE,
> >> +
> >> + /**
> >> + * Describes action shared a cross multiple flow rules.
>
> Both options are possible, but I'd propose to stick to:
> Describes -> Describe
>
> Or even:
> Use actions shared across multiple flow rules.
>
> >> + *
> >> + * Enables multiple rules reference the same action by handle (see
>
> Enables -> Allow
>
> >> + * struct rte_flow_shared_action).
> >> + */
> >> + RTE_FLOW_ACTION_TYPE_SHARED,
> >> };
> >> /**
> >> @@ -2660,6 +2669,20 @@ struct rte_flow_action_set_dscp {
> >> uint8_t dscp;
> >> };
> >> +
> >> +/**
> >> + * RTE_FLOW_ACTION_TYPE_SHARED
> >> + *
> >> + * Opaque type returned after successfully creating a shared action.
> >> + *
> >> + * This handle can be used to manage and query the related action:
> >> + * - share it a cross multiple flow rules
> >> + * - update action configuration
> >> + * - query action data
> >> + * - destroy action
> >> + */
> >> +struct rte_flow_shared_action;
> >> +
> >> /* Mbuf dynamic field offset for metadata. */
> >> extern int32_t rte_flow_dynf_metadata_offs;
> >> @@ -3324,6 +3347,129 @@ int
> >> rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
> >> uint32_t nb_contexts, 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
> >> +struct rte_flow_shared_action *
> >> +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.
>
> Destroys -> Destroy
>
> >> + *
> >> + * @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_destroy(uint16_t port_id,
> >> + struct rte_flow_shared_action *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
>
> Updates -> Update
> inplace -> in-place
>
> >> + * with the configuration provided as *update* argument.
> >> + * The update of the shared action configuration effects all flow
> >> rules reusing
>
> May be: reusing -> using
>
> >> + * the action via handle.
>
> The interesting question what to do, if some rule cannot have a
> new action (i.e. new action is invalid for a rule).
> May be it is better to highlight it.
>
> >> + *
> >> + * @param[in] port_id
> >> + * The port identifier of the Ethernet device.
> >> + * @param[in] action
> >> + * Handle for the shared action to be updated.
> >> + * @param[in] update
> >> + * Action specification used to modify the action pointed by handle.
> >> + * *update* should be of same type with the action pointed by the
> >> *action*
> >> + * handle argument, otherwise considered as invalid.
>
> Why? If it is a generic rule, it should be checked on a generic
> ethdev layer, but I would not impose the limitation. If PMD
> may change it, why generic interface specification should
> restrict it.
>
> >> + * @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 *update* invalid.
> >> + * - (-ENOTSUP) if *update* 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,
> >> + struct rte_flow_shared_action *action,
> >> + const struct rte_flow_action *update,
> >> + 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.
>
> "This function allows retrieving" -> Retrieve or Get or Query
>
> >> + * 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 struct rte_flow_shared_action *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 881cc469b7..a2cae1b53c 100644
> >> --- a/lib/librte_ethdev/rte_flow_driver.h
> >> +++ b/lib/librte_ethdev/rte_flow_driver.h
> >> @@ -107,6 +107,28 @@ struct rte_flow_ops {
> >> void **context,
> >> uint32_t nb_contexts,
> >> struct rte_flow_error *err);
> >> + /** See rte_flow_shared_action_create() */
> >> + struct rte_flow_shared_action *(*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,
> >> + struct rte_flow_shared_action *shared_action,
> >> + struct rte_flow_error *error);
> >> + /** See rte_flow_shared_action_update() */
> >> + int (*shared_action_update)
> >> + (struct rte_eth_dev *dev,
> >> + struct rte_flow_shared_action *shared_action,
> >> + const struct rte_flow_action *update,
> >> + struct rte_flow_error *error);
> >> + /** See rte_flow_shared_action_query() */
> >> + int (*shared_action_query)
> >> + (struct rte_eth_dev *dev,
> >> + const struct rte_flow_shared_action *shared_action,
> >> + void *data,
> >> + struct rte_flow_error *error);
> >> };
> >> /**
> >>
> >
> > The modification of "struct rte_flow_ops", looks fine.
> > Acked-by: Ray Kinsella <mdr@ashroe.eu>
> >
>
@@ -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;
};
@@ -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));
}
+
+struct rte_flow_shared_action *
+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];
+ struct rte_flow_shared_action *shared_action;
+ const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
+
+ if (unlikely(!ops))
+ return NULL;
+ if (likely(!!ops->shared_action_create)) {
+ shared_action = ops->shared_action_create(dev, action, error);
+ if (shared_action == NULL)
+ flow_err(port_id, -rte_errno, error);
+ return shared_action;
+ }
+ 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,
+ struct rte_flow_shared_action *action,
+ 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, action, 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,
+ struct rte_flow_shared_action *action,
+ 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, action,
+ 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 struct rte_flow_shared_action *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));
+}
@@ -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
+ * struct rte_flow_shared_action).
+ */
+ RTE_FLOW_ACTION_TYPE_SHARED,
};
/**
@@ -2593,6 +2602,20 @@ struct rte_flow_action_set_dscp {
uint8_t dscp;
};
+
+/**
+ * RTE_FLOW_ACTION_TYPE_SHARED
+ *
+ * Opaque type returned after successfully creating a shared action.
+ *
+ * This handle can be used to manage and query the related action:
+ * - share it a cross multiple flow rules
+ * - update action configuration
+ * - query action data
+ * - destroy action
+ */
+struct rte_flow_shared_action;
+
/* Mbuf dynamic field offset for metadata. */
extern int rte_flow_dynf_metadata_offs;
@@ -3224,6 +3247,129 @@ 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
+struct rte_flow_shared_action *
+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,
+ struct rte_flow_shared_action *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,
+ struct rte_flow_shared_action *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 struct rte_flow_shared_action *action,
+ void *data,
+ struct rte_flow_error *error);
+
#ifdef __cplusplus
}
#endif
@@ -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() */
+ struct rte_flow_shared_action *(*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,
+ struct rte_flow_shared_action *shared_action,
+ struct rte_flow_error *error);
+ /** See rte_flow_shared_action_update() */
+ int (*shared_action_update)
+ (struct rte_eth_dev *dev,
+ struct rte_flow_shared_action *shared_action,
+ 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 struct rte_flow_shared_action *shared_action,
+ void *data,
+ struct rte_flow_error *error);
};
/**