[v7,1/2] ethdev: add flow shared action API
Checks
Commit Message
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 affects 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 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 resides 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 depend on the 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 state (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. This API doesn't query shared action configuration
since it is controlled by rte_flow_shared_action_create() and
rte_flow_shared_action_update() APIs and no supposed to change by other
means.
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 (port) create {action_id (id)} (action) / end
flow shared_action (port) update (id) (action) / end
flow shared_action (port) destroy action_id (id) {action_id (id) [...]}
flow shared_action (port) query (id)
testpmd example
===
configure rss to queues 1 & 2
> flow shared_action 0 create action_id 100 rss queues 1 2 end / end
create flow rule utilizing shared action
> flow create 0 ingress \
pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
actions shared 100 / end
add 2 more queues
> flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end
example
===
struct rte_flow_action actions[2];
struct rte_flow_shared_action_conf conf;
struct rte_flow_action action;
/* skipped: initialize conf and action */
struct rte_flow_shared_action *handle =
rte_flow_shared_action_create(port_id, &conf, &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@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
doc/guides/prog_guide/rte_flow.rst | 19 +++
doc/guides/rel_notes/release_20_11.rst | 9 ++
lib/librte_ethdev/rte_ethdev_version.map | 4 +
lib/librte_ethdev/rte_flow.c | 84 +++++++++++
lib/librte_ethdev/rte_flow.h | 169 ++++++++++++++++++++++-
lib/librte_ethdev/rte_flow_driver.h | 23 +++
6 files changed, 307 insertions(+), 1 deletion(-)
Comments
On Thu, Oct 8, 2020 at 4:51 AM Andrey Vesnovaty <andreyv@nvidia.com> wrote:
>
> 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 affects 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 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 resides 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 depend on the 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 state (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. This API doesn't query shared action configuration
> since it is controlled by rte_flow_shared_action_create() and
> rte_flow_shared_action_update() APIs and no supposed to change by other
> means.
>
> 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 (port) create {action_id (id)} (action) / end
> flow shared_action (port) update (id) (action) / end
> flow shared_action (port) destroy action_id (id) {action_id (id) [...]}
> flow shared_action (port) query (id)
>
> testpmd example
> ===
>
> configure rss to queues 1 & 2
>
> > flow shared_action 0 create action_id 100 rss queues 1 2 end / end
>
> create flow rule utilizing shared action
>
> > flow create 0 ingress \
> pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
> actions shared 100 / end
>
> add 2 more queues
>
> > flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end
>
> example
> ===
>
> struct rte_flow_action actions[2];
> struct rte_flow_shared_action_conf conf;
> struct rte_flow_action action;
> /* skipped: initialize conf and action */
> struct rte_flow_shared_action *handle =
> rte_flow_shared_action_create(port_id, &conf, &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@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>
Since this is an ethdev patch, the testpmd description is really not required.
Moreover they are not in sync with the direction and other changes you made
in the testpmd patch. Also there is a typo inline. Other than that..
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
> doc/guides/prog_guide/rte_flow.rst | 19 +++
> doc/guides/rel_notes/release_20_11.rst | 9 ++
> lib/librte_ethdev/rte_ethdev_version.map | 4 +
> lib/librte_ethdev/rte_flow.c | 84 +++++++++++
> lib/librte_ethdev/rte_flow.h | 169 ++++++++++++++++++++++-
> lib/librte_ethdev/rte_flow_driver.h | 23 +++
> 6 files changed, 307 insertions(+), 1 deletion(-)
>
[snip]
> +
> +/**
> + * 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 across 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;
>
> @@ -3357,6 +3380,150 @@ int
> rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
> uint32_t nb_contexts, struct rte_flow_error *error);
>
> +/**
> + * Specify shared action configuration
> + */
> +struct rte_flow_shared_action_conf {
> + /**
> + * Flow direction for shared action configuration.
> + *
> + * Shred action should be valid at least for one flow direction,
s/Shred/Shared
> + * otherwise it is invalid for both ingress and egress rules.
> + */
> + uint32_t ingress:1;
> + /**< Action valid for rules applied to ingress traffic. */
> + uint32_t egress:1;
> + /**< Action valid for rules applied to egress traffic. */
> +};
[snip]
"add flow shared action API"
Is flow shared? May be "add shared actions to flow API".
On 10/8/20 2:51 PM, Andrey Vesnovaty wrote:
> This commit introduces extension of DPDK flow action API enabling
"This commit" and "DPDK" are not necessary in description.
It is a commit description and the patch is to DPDK tree.
Consider just:
"Introduce extension of 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 affects 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 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 resides 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 depend on the 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 state (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. This API doesn't query shared action configuration
> since it is controlled by rte_flow_shared_action_create() and
> rte_flow_shared_action_update() APIs and no supposed to change by other
> means.
>
> 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 (port) create {action_id (id)} (action) / end
> flow shared_action (port) update (id) (action) / end
> flow shared_action (port) destroy action_id (id) {action_id (id) [...]}
> flow shared_action (port) query (id)
>
> testpmd example
> ===
>
> configure rss to queues 1 & 2
>
>> flow shared_action 0 create action_id 100 rss queues 1 2 end / end
>
> create flow rule utilizing shared action
>
>> flow create 0 ingress \
> pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
> actions shared 100 / end
>
> add 2 more queues
>
>> flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end
testpmd is out-of-scope of the patch and it is better to
remove the description to avoid unsync if testpmd
commands are changed.
>
> example
> ===
>
> struct rte_flow_action actions[2];
> struct rte_flow_shared_action_conf conf;
> struct rte_flow_action action;
> /* skipped: initialize conf and action */
> struct rte_flow_shared_action *handle =
> rte_flow_shared_action_create(port_id, &conf, &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@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>
LGTM
> ---
> doc/guides/prog_guide/rte_flow.rst | 19 +++
> doc/guides/rel_notes/release_20_11.rst | 9 ++
> lib/librte_ethdev/rte_ethdev_version.map | 4 +
> lib/librte_ethdev/rte_flow.c | 84 +++++++++++
> lib/librte_ethdev/rte_flow.h | 169 ++++++++++++++++++++++-
> lib/librte_ethdev/rte_flow_driver.h | 23 +++
> 6 files changed, 307 insertions(+), 1 deletion(-)
>
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 119b128739..8cff8a0440 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2666,6 +2666,25 @@ timeout passed without any matching on the flow.
> | ``context`` | user input flow context |
> +--------------+---------------------------------+
>
> +Action: ``SHARED``
> +^^^^^^^^^^^^^^^^^^
> +
> +Flow Utilize shared action by handle as returned from
Utilize -> utilize
> +``rte_flow_shared_action_create()``.
> +
> +The behaviour of the shared action defined by ``action`` argument of type
> +``struct rte_flow_action`` passed to ``rte_flow_shared_action_create()``.
> +
> +.. _table_rte_flow_shared_action:
> +
> +.. table:: SHARED
> +
> + +---------------+
> + | Field |
> + +===============+
> + | no properties |
> + +---------------+
> +
> Negative types
> ~~~~~~~~~~~~~~
>
> diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
> index 0b2a3700c3..87c90909be 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -109,6 +109,15 @@ New Features
> * Extern objects and functions can be plugged into the pipeline.
> * Transaction-oriented table updates.
>
> +* **Add shared action support for rte flow.**
I think it required "ethdev: " prefix.
Add -> Added, rte -> RTE, plus API, i.e.:
**ethdev: Added shared action support to RTE flow API."
> +
> + Added shared action support to utilize single rte flow action in multiple
rte -> RTE, but I'd consider to drop RTE in both description,
here and below.
> + rte flow rules. An update of shared action configuration alters the behavior
> + of all rte flow rules using it.
> +
> + * Added new action: ``RTE_FLOW_ACTION_TYPE_SHARED`` to use shared action
> + as rte flow action.
> + * Added new rte flow APIs to create/update/destroy/query shared action.
Missing one more empty line here.
>
> Removed Items
> -------------
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index c95ef5157a..a8a4821dbb 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -229,6 +229,10 @@ EXPERIMENTAL {
> # added in 20.11
> rte_eth_link_speed_to_str;
> rte_eth_link_to_str;
> + rte_flow_shared_action_create;
> + rte_flow_shared_action_destroy;
> + rte_flow_shared_action_update;
> + rte_flow_shared_action_query;
Shouldn't it be alphabetically sorted?
> };
>
> INTERNAL {
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index f8fdd68fe9..9afa8905df 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -174,6 +174,7 @@ static const struct rte_flow_desc_data rte_flow_desc_action[] = {
> MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct rte_flow_action_set_dscp)),
> MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct rte_flow_action_set_dscp)),
> MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> + MK_FLOW_ACTION(SHARED, 0),
It looks correct, it deserves a comment which explains
why size is 0 here.
> };
>
> int
> @@ -1251,3 +1252,86 @@ 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_shared_action_conf *conf,
> + const struct rte_flow_action *action,
> + struct rte_flow_error *error)
> +{
> + struct rte_eth_dev *dev = &rte_eth_devices[port_id];
Sorry, but it unsafe to initialize it before port_id check.
It is too error-prone for the future maintenance of the
code. Since, port_id is checked indirection here via
rte_flow_ops_get(), please, initialize the variable or
even move it completely to be just before usage.
I realize that nearby code does the same, but IMHO it is
never later to turn into the right direction.
> + 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, conf, 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];
same here
> + 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];
same here
> + 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);
Sorry, but above is very hard to follow what is where. May be
helper variable sfor op result should be used to make it
readable.
> + 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];
same here
> + 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);
Sorry, but above is very hard to follow what is where. May be
helper variable sfor op result should be used to make it
readable.
> + 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 da8bfa5489..9050adec23 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1714,7 +1714,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.
> @@ -2132,6 +2133,14 @@ enum rte_flow_action_type {
> * see enum RTE_ETH_EVENT_FLOW_AGED
> */
> RTE_FLOW_ACTION_TYPE_AGE,
> +
> + /**
> + * Describe action shared across multiple flow rules.
> + *
> + * Allow multiple rules reference the same action by handle (see
> + * struct rte_flow_shared_action).
> + */
> + RTE_FLOW_ACTION_TYPE_SHARED,
> };
>
> /**
> @@ -2693,6 +2702,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 across 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;
>
> @@ -3357,6 +3380,150 @@ int
> rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
> uint32_t nb_contexts, struct rte_flow_error *error);
>
> +/**
> + * Specify shared action configuration
> + */
> +struct rte_flow_shared_action_conf {
> + /**
> + * Flow direction for shared action configuration.
> + *
> + * Shred action should be valid at least for one flow direction,
> + * otherwise it is invalid for both ingress and egress rules.
> + */
> + uint32_t ingress:1;
> + /**< Action valid for rules applied to ingress traffic. */
> + uint32_t egress:1;
> + /**< Action valid for rules applied to egress traffic. */
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Create shared action for reuse in multiple flow rules.
> + * The created shared action has single state and configuration
> + * across all flow rules using it.
> + *
> + * @param[in] port_id
> + * The port identifier of the Ethernet device.
> + * @param[in] conf
> + * Shared action configuration.
> + * @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.
ENODEV ?
> + */
> +__rte_experimental
> +struct rte_flow_shared_action *
> +rte_flow_shared_action_create(uint16_t port_id,
> + const struct rte_flow_shared_action_conf *conf,
> + const struct rte_flow_action *action,
> + struct rte_flow_error *error);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Destroy 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.
-ENODEV?
> + */
> +__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.
> + *
> + * Update in-place 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.
-ENODEV ?
> + * 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.
> + *
> + * Retrieve 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 3ee871d3eb..adaace47ea 100644
> --- a/lib/librte_ethdev/rte_flow_driver.h
> +++ b/lib/librte_ethdev/rte_flow_driver.h
> @@ -108,6 +108,29 @@ 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_shared_action_conf *conf,
> + 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);
> };
>
> /**
>
Hi Andrew.
Thanks for the input.
All spelling & rephrases will be applied PSB for the rest.
Will publish v8 very soon.
Thanks,
Andrey
> -----Original Message-----
> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> Sent: Monday, October 12, 2020 5:19 PM
> To: Andrey Vesnovaty <andreyv@nvidia.com>; dev@dpdk.org
> Cc: jer@marvell.com; jerinjacobk@gmail.com; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> stephen@networkplumber.org; bruce.richardson@intel.com; Ori Kam
> <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
> andrey.vesnovaty@gmail.com; mdr@ashroe.eu; nhorman@tuxdriver.com;
> ajit.khaparde@broadcom.com; samik.gupta@broadcom.com
> Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: add flow shared action API
>
> "add flow shared action API"
>
> Is flow shared? May be "add shared actions to flow API".
Accepted, will update commit message.
>
> On 10/8/20 2:51 PM, Andrey Vesnovaty wrote:
> > This commit introduces extension of DPDK flow action API enabling
>
> "This commit" and "DPDK" are not necessary in description.
> It is a commit description and the patch is to DPDK tree.
> Consider just:
> "Introduce extension of flow action API enabling..."
Accepted, will update commit message.
>
> > 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 affects 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 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 resides 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 depend on the 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 state (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. This API doesn't query shared action configuration
> > since it is controlled by rte_flow_shared_action_create() and
> > rte_flow_shared_action_update() APIs and no supposed to change by other
> > means.
> >
> > 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 (port) create {action_id (id)} (action) / end
> > flow shared_action (port) update (id) (action) / end
> > flow shared_action (port) destroy action_id (id) {action_id (id) [...]}
> > flow shared_action (port) query (id)
> >
> > testpmd example
> > ===
> >
> > configure rss to queues 1 & 2
> >
> >> flow shared_action 0 create action_id 100 rss queues 1 2 end / end
> >
> > create flow rule utilizing shared action
> >
> >> flow create 0 ingress \
> > pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
> > actions shared 100 / end
> >
> > add 2 more queues
> >
> >> flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end
>
> testpmd is out-of-scope of the patch and it is better to
> remove the description to avoid unsync if testpmd
> commands are changed.
There is still added value is testpmd example demonstrating usage of
shared action with rte flows. I will update the example to reflect the current
testpmd syntax for shared action.
>
> >
> > example
> > ===
> >
> > struct rte_flow_action actions[2];
> > struct rte_flow_shared_action_conf conf;
> > struct rte_flow_action action;
> > /* skipped: initialize conf and action */
> > struct rte_flow_shared_action *handle =
> > rte_flow_shared_action_create(port_id, &conf, &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@nvidia.com>
> > Acked-by: Ori Kam <orika@nvidia.com>
>
> LGTM
>
> > ---
> > doc/guides/prog_guide/rte_flow.rst | 19 +++
> > doc/guides/rel_notes/release_20_11.rst | 9 ++
> > lib/librte_ethdev/rte_ethdev_version.map | 4 +
> > lib/librte_ethdev/rte_flow.c | 84 +++++++++++
> > lib/librte_ethdev/rte_flow.h | 169 ++++++++++++++++++++++-
> > lib/librte_ethdev/rte_flow_driver.h | 23 +++
> > 6 files changed, 307 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> > index 119b128739..8cff8a0440 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -2666,6 +2666,25 @@ timeout passed without any matching on the flow.
> > | ``context`` | user input flow context |
> > +--------------+---------------------------------+
> >
> > +Action: ``SHARED``
> > +^^^^^^^^^^^^^^^^^^
> > +
> > +Flow Utilize shared action by handle as returned from
>
> Utilize -> utilize
Will fix.
>
> > +``rte_flow_shared_action_create()``.
> > +
> > +The behaviour of the shared action defined by ``action`` argument of type
> > +``struct rte_flow_action`` passed to ``rte_flow_shared_action_create()``.
> > +
> > +.. _table_rte_flow_shared_action:
> > +
> > +.. table:: SHARED
> > +
> > + +---------------+
> > + | Field |
> > + +===============+
> > + | no properties |
> > + +---------------+
> > +
> > Negative types
> > ~~~~~~~~~~~~~~
> >
> > diff --git a/doc/guides/rel_notes/release_20_11.rst
> b/doc/guides/rel_notes/release_20_11.rst
> > index 0b2a3700c3..87c90909be 100644
> > --- a/doc/guides/rel_notes/release_20_11.rst
> > +++ b/doc/guides/rel_notes/release_20_11.rst
> > @@ -109,6 +109,15 @@ New Features
> > * Extern objects and functions can be plugged into the pipeline.
> > * Transaction-oriented table updates.
> >
> > +* **Add shared action support for rte flow.**
>
> I think it required "ethdev: " prefix.
> Add -> Added, rte -> RTE, plus API, i.e.:
> **ethdev: Added shared action support to RTE flow API."
>
"ethdev:" prefix used in 'API Changes '. I'll change it to:
* **Added ethdev API to support shared action for RTE flow.**
> > +
> > + Added shared action support to utilize single rte flow action in multiple
>
> rte -> RTE, but I'd consider to drop RTE in both description,
> here and below.
>
> > + rte flow rules. An update of shared action configuration alters the behavior
> > + of all rte flow rules using it.
> > +
> > + * Added new action: ``RTE_FLOW_ACTION_TYPE_SHARED`` to use shared
> action
> > + as rte flow action.
> > + * Added new rte flow APIs to create/update/destroy/query shared action.
>
> Missing one more empty line here.
Will fix.
>
> >
> > Removed Items
> > -------------
> > diff --git a/lib/librte_ethdev/rte_ethdev_version.map
> b/lib/librte_ethdev/rte_ethdev_version.map
> > index c95ef5157a..a8a4821dbb 100644
> > --- a/lib/librte_ethdev/rte_ethdev_version.map
> > +++ b/lib/librte_ethdev/rte_ethdev_version.map
> > @@ -229,6 +229,10 @@ EXPERIMENTAL {
> > # added in 20.11
> > rte_eth_link_speed_to_str;
> > rte_eth_link_to_str;
> > + rte_flow_shared_action_create;
> > + rte_flow_shared_action_destroy;
> > + rte_flow_shared_action_update;
> > + rte_flow_shared_action_query;
>
> Shouldn't it be alphabetically sorted?
Query before update?
>
> > };
> >
> > INTERNAL {
> > diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> > index f8fdd68fe9..9afa8905df 100644
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -174,6 +174,7 @@ static const struct rte_flow_desc_data
> rte_flow_desc_action[] = {
> > MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct
> rte_flow_action_set_dscp)),
> > MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> rte_flow_action_set_dscp)),
> > MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> > + MK_FLOW_ACTION(SHARED, 0),
>
> It looks correct, it deserves a comment which explains
> why size is 0 here.
Comment will be added.
>
> > };
> >
> > int
> > @@ -1251,3 +1252,86 @@ 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_shared_action_conf *conf,
> > + const struct rte_flow_action *action,
> > + struct rte_flow_error *error)
> > +{
> > + struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>
> Sorry, but it unsafe to initialize it before port_id check.
> It is too error-prone for the future maintenance of the
> code. Since, port_id is checked indirection here via
> rte_flow_ops_get(), please, initialize the variable or
> even move it completely to be just before usage.
>
> I realize that nearby code does the same, but IMHO it is
> never later to turn into the right direction.
Will fix.
>
> > + 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, conf, 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];
>
> same here
Will fix.
>
> > + 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];
>
> same here
Will fix.
>
> > + 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);
>
> Sorry, but above is very hard to follow what is where. May be
> helper variable sfor op result should be used to make it
> readable.
Agree, refactoring needed.
>
> > + 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];
>
> same here
Will fix.
>
> > + 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);
>
> Sorry, but above is very hard to follow what is where. May be
> helper variable sfor op result should be used to make it
> readable.
Agree, refactoring needed.
>
> > + 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 da8bfa5489..9050adec23 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -1714,7 +1714,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.
> > @@ -2132,6 +2133,14 @@ enum rte_flow_action_type {
> > * see enum RTE_ETH_EVENT_FLOW_AGED
> > */
> > RTE_FLOW_ACTION_TYPE_AGE,
> > +
> > + /**
> > + * Describe action shared across multiple flow rules.
> > + *
> > + * Allow multiple rules reference the same action by handle (see
> > + * struct rte_flow_shared_action).
> > + */
> > + RTE_FLOW_ACTION_TYPE_SHARED,
> > };
> >
> > /**
> > @@ -2693,6 +2702,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 across 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;
> >
> > @@ -3357,6 +3380,150 @@ int
> > rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
> > uint32_t nb_contexts, struct rte_flow_error *error);
> >
> > +/**
> > + * Specify shared action configuration
> > + */
> > +struct rte_flow_shared_action_conf {
> > + /**
> > + * Flow direction for shared action configuration.
> > + *
> > + * Shred action should be valid at least for one flow direction,
> > + * otherwise it is invalid for both ingress and egress rules.
> > + */
> > + uint32_t ingress:1;
> > + /**< Action valid for rules applied to ingress traffic. */
> > + uint32_t egress:1;
> > + /**< Action valid for rules applied to egress traffic. */
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Create shared action for reuse in multiple flow rules.
> > + * The created shared action has single state and configuration
> > + * across all flow rules using it.
> > + *
> > + * @param[in] port_id
> > + * The port identifier of the Ethernet device.
> > + * @param[in] conf
> > + * Shared action configuration.
> > + * @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.
>
> ENODEV ?
Can you elaborate?
>
> > + */
> > +__rte_experimental
> > +struct rte_flow_shared_action *
> > +rte_flow_shared_action_create(uint16_t port_id,
> > + const struct rte_flow_shared_action_conf *conf,
> > + const struct rte_flow_action *action,
> > + struct rte_flow_error *error);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Destroy 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.
>
> -ENODEV?
Same here.
>
> > + */
> > +__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.
> > + *
> > + * Update in-place 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.
>
> -ENODEV ?
And once more.
>
> > + * 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.
> > + *
> > + * Retrieve 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 3ee871d3eb..adaace47ea 100644
> > --- a/lib/librte_ethdev/rte_flow_driver.h
> > +++ b/lib/librte_ethdev/rte_flow_driver.h
> > @@ -108,6 +108,29 @@ 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_shared_action_conf *conf,
> > + 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);
> > };
> >
> > /**
> >
Hi Andrey,
On 10/13/20 11:06 PM, Andrey Vesnovaty wrote:
> Hi Andrew.
>
> Thanks for the input.
> All spelling & rephrases will be applied PSB for the rest.
> Will publish v8 very soon.
>
> Thanks,
> Andrey
>
>> -----Original Message-----
>> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
>> Sent: Monday, October 12, 2020 5:19 PM
>> To: Andrey Vesnovaty <andreyv@nvidia.com>; dev@dpdk.org
>> Cc: jer@marvell.com; jerinjacobk@gmail.com; NBU-Contact-Thomas Monjalon
>> <thomas@monjalon.net>; ferruh.yigit@intel.com;
>> stephen@networkplumber.org; bruce.richardson@intel.com; Ori Kam
>> <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
>> andrey.vesnovaty@gmail.com; mdr@ashroe.eu; nhorman@tuxdriver.com;
>> ajit.khaparde@broadcom.com; samik.gupta@broadcom.com
>> Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: add flow shared action API
>>
>> "add flow shared action API"
>>
>> Is flow shared? May be "add shared actions to flow API".
>
> Accepted, will update commit message.
>>
>> On 10/8/20 2:51 PM, Andrey Vesnovaty wrote:
>>> This commit introduces extension of DPDK flow action API enabling
>>
>> "This commit" and "DPDK" are not necessary in description.
>> It is a commit description and the patch is to DPDK tree.
>> Consider just:
>> "Introduce extension of flow action API enabling..."
>
> Accepted, will update commit message.
>>
>>> 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 affects 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 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 resides 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 depend on the 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 state (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. This API doesn't query shared action configuration
>>> since it is controlled by rte_flow_shared_action_create() and
>>> rte_flow_shared_action_update() APIs and no supposed to change by other
>>> means.
>>>
>>> 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 (port) create {action_id (id)} (action) / end
>>> flow shared_action (port) update (id) (action) / end
>>> flow shared_action (port) destroy action_id (id) {action_id (id) [...]}
>>> flow shared_action (port) query (id)
>>>
>>> testpmd example
>>> ===
>>>
>>> configure rss to queues 1 & 2
>>>
>>>> flow shared_action 0 create action_id 100 rss queues 1 2 end / end
>>>
>>> create flow rule utilizing shared action
>>>
>>>> flow create 0 ingress \
>>> pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
>>> actions shared 100 / end
>>>
>>> add 2 more queues
>>>
>>>> flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end
>>
>> testpmd is out-of-scope of the patch and it is better to
>> remove the description to avoid unsync if testpmd
>> commands are changed.
>
> There is still added value is testpmd example demonstrating usage of
> shared action with rte flows. I will update the example to reflect the current
> testpmd syntax for shared action.
Yes and no. IMHO It would be OK for series description etc,
but not OK for the changeset description which will be
kept in the history and will become misleading when
commands are changed. I think that no information is better
than potentially wrong information.
[snip]
>>> diff --git a/lib/librte_ethdev/rte_ethdev_version.map
>> b/lib/librte_ethdev/rte_ethdev_version.map
>>> index c95ef5157a..a8a4821dbb 100644
>>> --- a/lib/librte_ethdev/rte_ethdev_version.map
>>> +++ b/lib/librte_ethdev/rte_ethdev_version.map
>>> @@ -229,6 +229,10 @@ EXPERIMENTAL {
>>> # added in 20.11
>>> rte_eth_link_speed_to_str;
>>> rte_eth_link_to_str;
>>> + rte_flow_shared_action_create;
>>> + rte_flow_shared_action_destroy;
>>> + rte_flow_shared_action_update;
>>> + rte_flow_shared_action_query;
>>
>> Shouldn't it be alphabetically sorted?
>
> Query before update?
yes
[snip]
>>> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
>>> index da8bfa5489..9050adec23 100644
>>> --- a/lib/librte_ethdev/rte_flow.h
>>> +++ b/lib/librte_ethdev/rte_flow.h
[snip]
>>> @@ -3357,6 +3380,150 @@ int
>>> rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
>>> uint32_t nb_contexts, struct rte_flow_error *error);
>>>
>>> +/**
>>> + * Specify shared action configuration
>>> + */
>>> +struct rte_flow_shared_action_conf {
>>> + /**
>>> + * Flow direction for shared action configuration.
>>> + *
>>> + * Shred action should be valid at least for one flow direction,
>>> + * otherwise it is invalid for both ingress and egress rules.
>>> + */
>>> + uint32_t ingress:1;
>>> + /**< Action valid for rules applied to ingress traffic. */
>>> + uint32_t egress:1;
>>> + /**< Action valid for rules applied to egress traffic. */
>>> +};
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Create shared action for reuse in multiple flow rules.
>>> + * The created shared action has single state and configuration
>>> + * across all flow rules using it.
>>> + *
>>> + * @param[in] port_id
>>> + * The port identifier of the Ethernet device.
>>> + * @param[in] conf
>>> + * Shared action configuration.
>>> + * @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.
>>
>> ENODEV ?
>
> Can you elaborate?
ENODEV is returned if port_id is invalid and it should be
documented here if you try to list all return values.
14/10/2020 08:49, Andrew Rybchenko:
> On 10/13/20 11:06 PM, Andrey Vesnovaty wrote:
> > From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> >>> Shared action create/use/destroy
> >>> ===
> >>> Shared action may be reused by some or none flow rules at any given
> >>> moment, i.e. shared action resides 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 depend on the 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 state (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. This API doesn't query shared action configuration
> >>> since it is controlled by rte_flow_shared_action_create() and
> >>> rte_flow_shared_action_update() APIs and no supposed to change by other
> >>> means.
> >>>
> >>> PMD support
> >>> ===
> >>> The support of introduced API is pure PMD specific design and
> >>> responsibility for each action type (see struct rte_flow_ops).
This sentence is strange.
Of course PMD implementation is a PMD specific design.
I think you can remove it.
> >>> testpmd
> >>> ===
> >>> In order to utilize introduced API testpmd cli may implement following
> >>> extension
> >>> create/update/destroy/query shared action accordingly
> >>>
> >>> flow shared_action (port) create {action_id (id)} (action) / end
> >>> flow shared_action (port) update (id) (action) / end
> >>> flow shared_action (port) destroy action_id (id) {action_id (id) [...]}
> >>> flow shared_action (port) query (id)
> >>>
> >>> testpmd example
> >>> ===
> >>>
> >>> configure rss to queues 1 & 2
> >>>
> >>>> flow shared_action 0 create action_id 100 rss queues 1 2 end / end
> >>>
> >>> create flow rule utilizing shared action
> >>>
> >>>> flow create 0 ingress \
> >>> pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
> >>> actions shared 100 / end
> >>>
> >>> add 2 more queues
> >>>
> >>>> flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end
> >>
> >> testpmd is out-of-scope of the patch and it is better to
> >> remove the description to avoid unsync if testpmd
> >> commands are changed.
> >
> > There is still added value is testpmd example demonstrating usage of
> > shared action with rte flows. I will update the example to reflect the current
> > testpmd syntax for shared action.
>
> Yes and no. IMHO It would be OK for series description etc,
> but not OK for the changeset description which will be
> kept in the history and will become misleading when
> commands are changed. I think that no information is better
> than potentially wrong information.
I agree with Andrew, the API explanation should be enough.
Talking about testpmd commands in the ethdev patch is not appropriate.
Hi Andrew
All your suggestions applied in v8 series.
Thanks,
Andrey
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Wednesday, October 14, 2020 9:50 AM
> To: Andrey Vesnovaty <andreyv@nvidia.com>; dev@dpdk.org
> Cc: jer@marvell.com; jerinjacobk@gmail.com; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> stephen@networkplumber.org; bruce.richardson@intel.com; Ori Kam
> <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
> andrey.vesnovaty@gmail.com; mdr@ashroe.eu; nhorman@tuxdriver.com;
> ajit.khaparde@broadcom.com; samik.gupta@broadcom.com
> Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: add flow shared action API
>
> Hi Andrey,
>
> On 10/13/20 11:06 PM, Andrey Vesnovaty wrote:
> > Hi Andrew.
> >
> > Thanks for the input.
> > All spelling & rephrases will be applied PSB for the rest.
> > Will publish v8 very soon.
> >
> > Thanks,
> > Andrey
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> >> Sent: Monday, October 12, 2020 5:19 PM
> >> To: Andrey Vesnovaty <andreyv@nvidia.com>; dev@dpdk.org
> >> Cc: jer@marvell.com; jerinjacobk@gmail.com; NBU-Contact-Thomas
> Monjalon
> >> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> >> stephen@networkplumber.org; bruce.richardson@intel.com; Ori Kam
> >> <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
> >> andrey.vesnovaty@gmail.com; mdr@ashroe.eu; nhorman@tuxdriver.com;
> >> ajit.khaparde@broadcom.com; samik.gupta@broadcom.com
> >> Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: add flow shared action API
> >>
> >> "add flow shared action API"
> >>
> >> Is flow shared? May be "add shared actions to flow API".
> >
> > Accepted, will update commit message.
> >>
> >> On 10/8/20 2:51 PM, Andrey Vesnovaty wrote:
> >>> This commit introduces extension of DPDK flow action API enabling
> >>
> >> "This commit" and "DPDK" are not necessary in description.
> >> It is a commit description and the patch is to DPDK tree.
> >> Consider just:
> >> "Introduce extension of flow action API enabling..."
> >
> > Accepted, will update commit message.
> >>
> >>> 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 affects 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 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 resides 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 depend on the 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 state (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. This API doesn't query shared action configuration
> >>> since it is controlled by rte_flow_shared_action_create() and
> >>> rte_flow_shared_action_update() APIs and no supposed to change by other
> >>> means.
> >>>
> >>> 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 (port) create {action_id (id)} (action) / end
> >>> flow shared_action (port) update (id) (action) / end
> >>> flow shared_action (port) destroy action_id (id) {action_id (id) [...]}
> >>> flow shared_action (port) query (id)
> >>>
> >>> testpmd example
> >>> ===
> >>>
> >>> configure rss to queues 1 & 2
> >>>
> >>>> flow shared_action 0 create action_id 100 rss queues 1 2 end / end
> >>>
> >>> create flow rule utilizing shared action
> >>>
> >>>> flow create 0 ingress \
> >>> pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
> >>> actions shared 100 / end
> >>>
> >>> add 2 more queues
> >>>
> >>>> flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end
> >>
> >> testpmd is out-of-scope of the patch and it is better to
> >> remove the description to avoid unsync if testpmd
> >> commands are changed.
> >
> > There is still added value is testpmd example demonstrating usage of
> > shared action with rte flows. I will update the example to reflect the current
> > testpmd syntax for shared action.
>
> Yes and no. IMHO It would be OK for series description etc,
> but not OK for the changeset description which will be
> kept in the history and will become misleading when
> commands are changed. I think that no information is better
> than potentially wrong information.
>
> [snip]
>
> >>> diff --git a/lib/librte_ethdev/rte_ethdev_version.map
> >> b/lib/librte_ethdev/rte_ethdev_version.map
> >>> index c95ef5157a..a8a4821dbb 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev_version.map
> >>> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> >>> @@ -229,6 +229,10 @@ EXPERIMENTAL {
> >>> # added in 20.11
> >>> rte_eth_link_speed_to_str;
> >>> rte_eth_link_to_str;
> >>> + rte_flow_shared_action_create;
> >>> + rte_flow_shared_action_destroy;
> >>> + rte_flow_shared_action_update;
> >>> + rte_flow_shared_action_query;
> >>
> >> Shouldn't it be alphabetically sorted?
> >
> > Query before update?
>
> yes
>
> [snip]
>
> >>> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> >>> index da8bfa5489..9050adec23 100644
> >>> --- a/lib/librte_ethdev/rte_flow.h
> >>> +++ b/lib/librte_ethdev/rte_flow.h
>
> [snip]
>
> >>> @@ -3357,6 +3380,150 @@ int
> >>> rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
> >>> uint32_t nb_contexts, struct rte_flow_error *error);
> >>>
> >>> +/**
> >>> + * Specify shared action configuration
> >>> + */
> >>> +struct rte_flow_shared_action_conf {
> >>> + /**
> >>> + * Flow direction for shared action configuration.
> >>> + *
> >>> + * Shred action should be valid at least for one flow direction,
> >>> + * otherwise it is invalid for both ingress and egress rules.
> >>> + */
> >>> + uint32_t ingress:1;
> >>> + /**< Action valid for rules applied to ingress traffic. */
> >>> + uint32_t egress:1;
> >>> + /**< Action valid for rules applied to egress traffic. */
> >>> +};
> >>> +
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Create shared action for reuse in multiple flow rules.
> >>> + * The created shared action has single state and configuration
> >>> + * across all flow rules using it.
> >>> + *
> >>> + * @param[in] port_id
> >>> + * The port identifier of the Ethernet device.
> >>> + * @param[in] conf
> >>> + * Shared action configuration.
> >>> + * @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.
> >>
> >> ENODEV ?
> >
> > Can you elaborate?
>
>
> ENODEV is returned if port_id is invalid and it should be
> documented here if you try to list all return values.
Hi Thomas
All your suggestions applied in v8 series.
Thanks,
Andrey
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, October 14, 2020 10:23 AM
> To: Andrey Vesnovaty <andreyv@nvidia.com>
> Cc: dev@dpdk.org; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>;
> jerinj@marvell.com; ferruh.yigit@intel.com; stephen@networkplumber.org;
> bruce.richardson@intel.com; Ori Kam <orika@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; andrey.vesnovaty@gmail.com;
> ajit.khaparde@broadcom.com; samik.gupta@broadcom.com
> Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: add flow shared action API
>
> 14/10/2020 08:49, Andrew Rybchenko:
> > On 10/13/20 11:06 PM, Andrey Vesnovaty wrote:
> > > From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> > >>> Shared action create/use/destroy
> > >>> ===
> > >>> Shared action may be reused by some or none flow rules at any given
> > >>> moment, i.e. shared action resides 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 depend on the 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 state (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. This API doesn't query shared action configuration
> > >>> since it is controlled by rte_flow_shared_action_create() and
> > >>> rte_flow_shared_action_update() APIs and no supposed to change by
> other
> > >>> means.
> > >>>
> > >>> PMD support
> > >>> ===
> > >>> The support of introduced API is pure PMD specific design and
> > >>> responsibility for each action type (see struct rte_flow_ops).
>
> This sentence is strange.
> Of course PMD implementation is a PMD specific design.
> I think you can remove it.
>
> > >>> testpmd
> > >>> ===
> > >>> In order to utilize introduced API testpmd cli may implement following
> > >>> extension
> > >>> create/update/destroy/query shared action accordingly
> > >>>
> > >>> flow shared_action (port) create {action_id (id)} (action) / end
> > >>> flow shared_action (port) update (id) (action) / end
> > >>> flow shared_action (port) destroy action_id (id) {action_id (id) [...]}
> > >>> flow shared_action (port) query (id)
> > >>>
> > >>> testpmd example
> > >>> ===
> > >>>
> > >>> configure rss to queues 1 & 2
> > >>>
> > >>>> flow shared_action 0 create action_id 100 rss queues 1 2 end / end
> > >>>
> > >>> create flow rule utilizing shared action
> > >>>
> > >>>> flow create 0 ingress \
> > >>> pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
> > >>> actions shared 100 / end
> > >>>
> > >>> add 2 more queues
> > >>>
> > >>>> flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end
> > >>
> > >> testpmd is out-of-scope of the patch and it is better to
> > >> remove the description to avoid unsync if testpmd
> > >> commands are changed.
> > >
> > > There is still added value is testpmd example demonstrating usage of
> > > shared action with rte flows. I will update the example to reflect the current
> > > testpmd syntax for shared action.
> >
> > Yes and no. IMHO It would be OK for series description etc,
> > but not OK for the changeset description which will be
> > kept in the history and will become misleading when
> > commands are changed. I think that no information is better
> > than potentially wrong information.
>
> I agree with Andrew, the API explanation should be enough.
> Talking about testpmd commands in the ethdev patch is not appropriate.
>
>
Hi Ajit
All your suggestions applied in v8 series.
BW Thomas & Andrew pointed to testpmd example in RTE flow API.
Thanks,
Andrey
> -----Original Message-----
> From: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Sent: Friday, October 9, 2020 1:31 AM
> To: Andrey Vesnovaty <andreyv@nvidia.com>
> Cc: dpdk-dev <dev@dpdk.org>; jer@marvell.com; Jerin Jacob
> <jerinjacobk@gmail.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Stephen
> Hemminger <stephen@networkplumber.org>; Bruce Richardson
> <bruce.richardson@intel.com>; Ori Kam <orika@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; andrey.vesnovaty@gmail.com; Ray Kinsella
> <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>; Samik Gupta
> <samik.gupta@broadcom.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Subject: Re: [PATCH v7 1/2] ethdev: add flow shared action API
>
> On Thu, Oct 8, 2020 at 4:51 AM Andrey Vesnovaty <andreyv@nvidia.com>
> wrote:
> >
> > 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 affects 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 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 resides 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 depend on the 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 state (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. This API doesn't query shared action configuration
> > since it is controlled by rte_flow_shared_action_create() and
> > rte_flow_shared_action_update() APIs and no supposed to change by other
> > means.
> >
> > 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 (port) create {action_id (id)} (action) / end
> > flow shared_action (port) update (id) (action) / end
> > flow shared_action (port) destroy action_id (id) {action_id (id) [...]}
> > flow shared_action (port) query (id)
> >
> > testpmd example
> > ===
> >
> > configure rss to queues 1 & 2
> >
> > > flow shared_action 0 create action_id 100 rss queues 1 2 end / end
> >
> > create flow rule utilizing shared action
> >
> > > flow create 0 ingress \
> > pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
> > actions shared 100 / end
> >
> > add 2 more queues
> >
> > > flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end
> >
> > example
> > ===
> >
> > struct rte_flow_action actions[2];
> > struct rte_flow_shared_action_conf conf;
> > struct rte_flow_action action;
> > /* skipped: initialize conf and action */
> > struct rte_flow_shared_action *handle =
> > rte_flow_shared_action_create(port_id, &conf, &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@nvidia.com>
> > Acked-by: Ori Kam <orika@nvidia.com>
> Since this is an ethdev patch, the testpmd description is really not required.
> Moreover they are not in sync with the direction and other changes you made
> in the testpmd patch. Also there is a typo inline. Other than that..
>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>
> > ---
> > doc/guides/prog_guide/rte_flow.rst | 19 +++
> > doc/guides/rel_notes/release_20_11.rst | 9 ++
> > lib/librte_ethdev/rte_ethdev_version.map | 4 +
> > lib/librte_ethdev/rte_flow.c | 84 +++++++++++
> > lib/librte_ethdev/rte_flow.h | 169 ++++++++++++++++++++++-
> > lib/librte_ethdev/rte_flow_driver.h | 23 +++
> > 6 files changed, 307 insertions(+), 1 deletion(-)
> >
> [snip]
>
> > +
> > +/**
> > + * 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 across 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;
> >
> > @@ -3357,6 +3380,150 @@ int
> > rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
> > uint32_t nb_contexts, struct rte_flow_error *error);
> >
> > +/**
> > + * Specify shared action configuration
> > + */
> > +struct rte_flow_shared_action_conf {
> > + /**
> > + * Flow direction for shared action configuration.
> > + *
> > + * Shred action should be valid at least for one flow direction,
> s/Shred/Shared
>
> > + * otherwise it is invalid for both ingress and egress rules.
> > + */
> > + uint32_t ingress:1;
> > + /**< Action valid for rules applied to ingress traffic. */
> > + uint32_t egress:1;
> > + /**< Action valid for rules applied to egress traffic. */
> > +};
> [snip]
@@ -2666,6 +2666,25 @@ timeout passed without any matching on the flow.
| ``context`` | user input flow context |
+--------------+---------------------------------+
+Action: ``SHARED``
+^^^^^^^^^^^^^^^^^^
+
+Flow Utilize shared action by handle as returned from
+``rte_flow_shared_action_create()``.
+
+The behaviour of the shared action defined by ``action`` argument of type
+``struct rte_flow_action`` passed to ``rte_flow_shared_action_create()``.
+
+.. _table_rte_flow_shared_action:
+
+.. table:: SHARED
+
+ +---------------+
+ | Field |
+ +===============+
+ | no properties |
+ +---------------+
+
Negative types
~~~~~~~~~~~~~~
@@ -109,6 +109,15 @@ New Features
* Extern objects and functions can be plugged into the pipeline.
* Transaction-oriented table updates.
+* **Add shared action support for rte flow.**
+
+ Added shared action support to utilize single rte flow action in multiple
+ rte flow rules. An update of shared action configuration alters the behavior
+ of all rte flow rules using it.
+
+ * Added new action: ``RTE_FLOW_ACTION_TYPE_SHARED`` to use shared action
+ as rte flow action.
+ * Added new rte flow APIs to create/update/destroy/query shared action.
Removed Items
-------------
@@ -229,6 +229,10 @@ EXPERIMENTAL {
# added in 20.11
rte_eth_link_speed_to_str;
rte_eth_link_to_str;
+ rte_flow_shared_action_create;
+ rte_flow_shared_action_destroy;
+ rte_flow_shared_action_update;
+ rte_flow_shared_action_query;
};
INTERNAL {
@@ -174,6 +174,7 @@ static const struct rte_flow_desc_data rte_flow_desc_action[] = {
MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct rte_flow_action_set_dscp)),
MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct rte_flow_action_set_dscp)),
MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
+ MK_FLOW_ACTION(SHARED, 0),
};
int
@@ -1251,3 +1252,86 @@ 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_shared_action_conf *conf,
+ 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, conf, 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));
+}
@@ -1714,7 +1714,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.
@@ -2132,6 +2133,14 @@ enum rte_flow_action_type {
* see enum RTE_ETH_EVENT_FLOW_AGED
*/
RTE_FLOW_ACTION_TYPE_AGE,
+
+ /**
+ * Describe action shared across multiple flow rules.
+ *
+ * Allow multiple rules reference the same action by handle (see
+ * struct rte_flow_shared_action).
+ */
+ RTE_FLOW_ACTION_TYPE_SHARED,
};
/**
@@ -2693,6 +2702,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 across 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;
@@ -3357,6 +3380,150 @@ int
rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
uint32_t nb_contexts, struct rte_flow_error *error);
+/**
+ * Specify shared action configuration
+ */
+struct rte_flow_shared_action_conf {
+ /**
+ * Flow direction for shared action configuration.
+ *
+ * Shred action should be valid at least for one flow direction,
+ * otherwise it is invalid for both ingress and egress rules.
+ */
+ uint32_t ingress:1;
+ /**< Action valid for rules applied to ingress traffic. */
+ uint32_t egress:1;
+ /**< Action valid for rules applied to egress traffic. */
+};
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Create shared action for reuse in multiple flow rules.
+ * The created shared action has single state and configuration
+ * across all flow rules using it.
+ *
+ * @param[in] port_id
+ * The port identifier of the Ethernet device.
+ * @param[in] conf
+ * Shared action configuration.
+ * @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_shared_action_conf *conf,
+ const struct rte_flow_action *action,
+ struct rte_flow_error *error);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Destroy 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.
+ *
+ * Update in-place 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.
+ *
+ * Retrieve 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
@@ -108,6 +108,29 @@ 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_shared_action_conf *conf,
+ 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);
};
/**