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.rybche...@oktetlabs.ru> > Sent: Monday, October 12, 2020 5:19 PM > To: Andrey Vesnovaty <andr...@nvidia.com>; dev@dpdk.org > Cc: j...@marvell.com; jerinjac...@gmail.com; NBU-Contact-Thomas Monjalon > <tho...@monjalon.net>; ferruh.yi...@intel.com; > step...@networkplumber.org; bruce.richard...@intel.com; Ori Kam > <or...@nvidia.com>; Slava Ovsiienko <viachesl...@nvidia.com>; > andrey.vesnov...@gmail.com; m...@ashroe.eu; nhor...@tuxdriver.com; > ajit.khapa...@broadcom.com; samik.gu...@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 <andr...@nvidia.com> > > Acked-by: Ori Kam <or...@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); > > }; > > > > /** > >