Hi, Andrew, I'm back to this patch set to make it into 20.11.
Most of the items you raised regarding SW implementation already discussed with Jerin [1] and answered by Ori [2] and me [3]. Please follow my & Ori answer. In general, the idea of SW implementation for shared action looks nice from application perspective but the changes required in ethdev/rte_flow generic layer about to impact existing rte_flow APIs. Please follow my [3] & Ori's [2] answers & update on your suggestion to the Proposed API. I'll take your suggestions for rephrasing in upcoming patches version (V3). Thanks, Andrey --- [1] https://www.mail-archive.com/dev@dpdk.org/msg173461.html [2] https://www.mail-archive.com/dev@dpdk.org/msg173478.html [3] https://www.mail-archive.com/dev@dpdk.org/msg172921.html > -----Original Message----- > From: Andrew Rybchenko <arybche...@solarflare.com> > Sent: Wednesday, July 15, 2020 11:54 AM > To: Andrey Vesnovaty <andr...@mellanox.com>; Neil Horman > <nhor...@tuxdriver.com>; Thomas Monjalon <tho...@monjalon.net>; > Ferruh Yigit <ferruh.yi...@intel.com>; Andrew Rybchenko > <arybche...@solarflare.com>; Ori Kam <or...@mellanox.com> > Cc: Kinsella, Ray <m...@ashroe.eu>; dev@dpdk.org; jerinjac...@gmail.com; > step...@networkplumber.org; bruce.richard...@intel.com; Slava Ovsiienko > <viachesl...@mellanox.com>; andrey.vesnov...@gmail.com > Subject: Re: [dpdk-dev] [PATCH v2 1/6] ethdev: add flow shared action API > Importance: High > > On 7/13/20 11:04 AM, Kinsella, Ray wrote: > > > > > > On 08/07/2020 21:40, Andrey Vesnovaty wrote: > >> From: Andrey Vesnovaty <andrey.vesnov...@gmail.com> > >> > >> This commit introduces extension of DPDK flow action API enabling > >> sharing of single rte_flow_action in multiple flows. The API intended for > >> PMDs where multiple HW offloaded flows can reuse the same HW > >> essence/object representing flow action and modification of such an > >> essence/object effects all the rules using it. > >> > >> Motivation and example > >> === > >> Adding or removing one or more queues to RSS used by multiple flow rules > >> imposes per rule toll for current DPDK flow API; the scenario requires > >> for each flow sharing cloned RSS action: > >> - call `rte_flow_destroy()` > >> - call `rte_flow_create()` with modified RSS action > >> > >> API for sharing action and its in-place update benefits: > >> - reduce the overhead of multiple RSS flow rules reconfiguration > > It *allows* to reduce the overhead of multiple RSS flow rules > reconfiguration. I.e. it is not mandatory. PMD may do it in SW, > if HW does not support it. I see no problem to allow it. > Even if it is done in PMD, it is already an optimization since > applications have unified interface and internally it should > be cheaper to do it. > I'd consider to implement generic SW helper API for PMDs which > cannot have shared actions in HW, but would like to simulate it > in SW. It would allow to avoid the fallback in applications. > > >> - optimize resource utilization by sharing action across of multiple > >> flows > >> > >> Change description > >> === > >> > >> Shared action > >> === > >> In order to represent flow action shared by multiple flows new action > >> type RTE_FLOW_ACTION_TYPE_SHARED is introduced (see `enum > >> rte_flow_action_type`). > >> Actually the introduced API decouples action from any specific flow and > >> enables sharing of single action by its handle across multiple flows. > >> > >> Shared action create/use/destroy > >> === > >> Shared action may be reused by some or none flow rules at any given > >> moment, i.e. shared action reside outside of the context of any flow. > >> Shared action represent HW resources/objects used for action offloading > >> implementation. > >> API for shared action create (see `rte_flow_shared_action_create()`): > >> - should allocate HW resources and make related initializations required > >> for shared action implementation. > >> - make necessary preparations to maintain shared access to > >> the action resources, configuration and state. > >> API for shared action destroy (see `rte_flow_shared_action_destroy()`) > >> should release HW resources and make related cleanups required for shared > >> action implementation. > >> > >> In order to share some flow action reuse the handle of type > >> `struct rte_flow_shared_action` returned by > >> rte_flow_shared_action_create() as a `conf` field of > >> `struct rte_flow_action` (see "example" section). > >> > >> If some shared action not used by any flow rule all resources allocated > >> by the shared action can be released by rte_flow_shared_action_destroy() > >> (see "example" section). The shared action handle passed as argument to > >> destroy API should not be used any further i.e. result of the usage is > >> undefined. > > May be it makes sense to implement housekeeping in ethdev > layer? I.e. guarantee consequency using reference counters etc. > Will applications benefit from it? > > >> > >> Shared action re-configuration > >> === > >> Shared action behavior defined by its configuration can be updated via > >> rte_flow_shared_action_update() (see "example" section). The shared > >> action update operation modifies HW related resources/objects allocated > >> on the action creation. The number of operations performed by the update > >> operation should not be dependent on number of flows sharing the related > >> action. On return of shared action update API action behavior should be > >> according to updated configuration for all flows sharing the action. > > If shared action is simulated in SW, number of operations to do > reconfiguration will depend on a number of flow rules using it. > I think it is not a problem. So, *should not* used above is OK. > > Consider: > "should not be dependent on" -> "should not depend on" > > >> > >> Shared action query > >> === > >> Provide separate API to query shared action sate (see > > sate -> state > > >> rte_flow_shared_action_update()). Taking a counter as an example: query > >> returns value aggregating all counter increments across all flow rules > >> sharing the counter. > >> > >> PMD support > >> === > >> The support of introduced API is pure PMD specific design and > >> responsibility for each action type (see struct rte_flow_ops). > >> > >> testpmd > >> === > >> In order to utilize introduced API testpmd cli may implement following > >> extension > >> create/update/destroy/query shared action accordingly > >> > >> flow shared_action create {port_id} [index] {action} > >> flow shared_action update {port_id} {index} {action} > >> flow shared_action destroy {port_id} {index} > >> flow shared_action query {port_id} {index} > >> > >> testpmd example > >> === > >> > >> configure rss to queues 1 & 2 > >> > >> testpmd> flow shared_action create 0 100 rss 1 2 > >> > >> create flow rule utilizing shared action > >> > >> testpmd> flow create 0 ingress \ > >> pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \ > >> actions shared 100 end / end > >> > >> add 2 more queues > >> > >> testpmd> flow shared_action modify 0 100 rss 1 2 3 4 > >> > >> example > >> === > >> > >> struct rte_flow_action actions[2]; > >> struct rte_flow_action action; > >> /* skipped: initialize action */ > >> struct rte_flow_shared_action *handle = rte_flow_shared_action_create( > >> port_id, &action, &error); > > It should be possible to have many actions in shared action. > I.e. similar to below, it should be an array terminated by > RTE_FLOW_ACTION_TYPE_END. It is unclear from the example > above. > > >> actions[0].type = RTE_FLOW_ACTION_TYPE_SHARED; > >> actions[0].conf = handle; > >> actions[1].type = RTE_FLOW_ACTION_TYPE_END; > >> /* skipped: init attr0 & pattern0 args */ > >> struct rte_flow *flow0 = rte_flow_create(port_id, &attr0, pattern0, > >> actions, error); > >> /* create more rules reusing shared action */ > >> struct rte_flow *flow1 = rte_flow_create(port_id, &attr1, pattern1, > >> actions, error); > >> /* skipped: for flows 2 till N */ > >> struct rte_flow *flowN = rte_flow_create(port_id, &attrN, patternN, > >> actions, error); > >> /* update shared action */ > >> struct rte_flow_action updated_action; > >> /* > >> * skipped: initialize updated_action according to desired action > >> * configuration change > >> */ > >> rte_flow_shared_action_update(port_id, handle, &updated_action, error); > >> /* > >> * from now on all flows 1 till N will act according to configuration of > >> * updated_action > >> */ > >> /* skipped: destroy all flows 1 till N */ > >> rte_flow_shared_action_destroy(port_id, handle, error); > >> > >> Signed-off-by: Andrey Vesnovaty <andr...@mellanox.com> > >> --- > >> lib/librte_ethdev/rte_ethdev_version.map | 6 + > >> lib/librte_ethdev/rte_flow.c | 81 +++++++++++++ > >> lib/librte_ethdev/rte_flow.h | 148 ++++++++++++++++++++++- > >> lib/librte_ethdev/rte_flow_driver.h | 22 ++++ > >> 4 files changed, 256 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/librte_ethdev/rte_ethdev_version.map > >> b/lib/librte_ethdev/rte_ethdev_version.map > >> index 7155056045..119d84976a 100644 > >> --- a/lib/librte_ethdev/rte_ethdev_version.map > >> +++ b/lib/librte_ethdev/rte_ethdev_version.map > >> @@ -241,4 +241,10 @@ EXPERIMENTAL { > >> __rte_ethdev_trace_rx_burst; > >> __rte_ethdev_trace_tx_burst; > >> rte_flow_get_aged_flows; > >> + > >> + # added in 20.08 > >> + rte_flow_shared_action_create; > >> + rte_flow_shared_action_destroy; > >> + rte_flow_shared_action_update; > >> + rte_flow_shared_action_query; > >> }; > >> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c > >> index 1685be5f73..0ac4d31a13 100644 > >> --- a/lib/librte_ethdev/rte_flow.c > >> +++ b/lib/librte_ethdev/rte_flow.c > >> @@ -1250,3 +1250,84 @@ rte_flow_get_aged_flows(uint16_t port_id, void > >> **contexts, > >> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > >> NULL, rte_strerror(ENOTSUP)); > >> } > >> + > >> +struct rte_flow_shared_action * > >> +rte_flow_shared_action_create(uint16_t port_id, > >> + const struct rte_flow_action *action, > >> + struct rte_flow_error *error) > >> +{ > >> + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > >> + struct rte_flow_shared_action *shared_action; > >> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); > >> + > >> + if (unlikely(!ops)) > >> + return NULL; > >> + if (likely(!!ops->shared_action_create)) { > >> + shared_action = ops->shared_action_create(dev, action, error); > >> + if (shared_action == NULL) > >> + flow_err(port_id, -rte_errno, error); > >> + return shared_action; > >> + } > >> + rte_flow_error_set(error, ENOSYS, > RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > >> + NULL, rte_strerror(ENOSYS)); > >> + return NULL; > >> +} > >> + > >> +int > >> +rte_flow_shared_action_destroy(uint16_t port_id, > >> + struct rte_flow_shared_action *action, > >> + struct rte_flow_error *error) > >> +{ > >> + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > >> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); > >> + > >> + if (unlikely(!ops)) > >> + return -rte_errno; > >> + if (likely(!!ops->shared_action_destroy)) > >> + return flow_err(port_id, > >> + ops->shared_action_destroy(dev, action, error), > >> + error); > >> + return rte_flow_error_set(error, ENOSYS, > >> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > >> + NULL, rte_strerror(ENOSYS)); > >> +} > >> + > >> +int > >> +rte_flow_shared_action_update(uint16_t port_id, > >> + struct rte_flow_shared_action *action, > >> + const struct rte_flow_action *update, > >> + struct rte_flow_error *error) > >> +{ > >> + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > >> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); > >> + > >> + if (unlikely(!ops)) > >> + return -rte_errno; > >> + if (likely(!!ops->shared_action_update)) > >> + return flow_err(port_id, ops->shared_action_update(dev, action, > >> + update, error), > >> + error); > >> + return rte_flow_error_set(error, ENOSYS, > >> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > >> + NULL, rte_strerror(ENOSYS)); > >> +} > >> + > >> +int > >> +rte_flow_shared_action_query(uint16_t port_id, > >> + const struct rte_flow_shared_action *action, > >> + void *data, > >> + struct rte_flow_error *error) > >> +{ > >> + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > >> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); > >> + > >> + if (unlikely(!ops)) > >> + return -rte_errno; > >> + if (likely(!!ops->shared_action_query)) > >> + return flow_err(port_id, ops->shared_action_query(dev, action, > >> + data, error), > >> + error); > >> + return rte_flow_error_set(error, ENOSYS, > >> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > >> + NULL, rte_strerror(ENOSYS)); > >> +} > >> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h > >> index b0e4199192..257456b14a 100644 > >> --- a/lib/librte_ethdev/rte_flow.h > >> +++ b/lib/librte_ethdev/rte_flow.h > >> @@ -1681,7 +1681,8 @@ enum rte_flow_action_type { > >> /** > >> * Enables counters for this flow rule. > >> * > >> - * These counters can be retrieved and reset through rte_flow_query(), > >> + * These counters can be retrieved and reset through rte_flow_query() or > >> + * rte_flow_shared_action_query() if the action provided via handle, > >> * see struct rte_flow_query_count. > >> * > >> * See struct rte_flow_action_count. > >> @@ -2099,6 +2100,14 @@ enum rte_flow_action_type { > >> * see enum RTE_ETH_EVENT_FLOW_AGED > >> */ > >> RTE_FLOW_ACTION_TYPE_AGE, > >> + > >> + /** > >> + * Describes action shared a cross multiple flow rules. > > Both options are possible, but I'd propose to stick to: > Describes -> Describe > > Or even: > Use actions shared across multiple flow rules. > > >> + * > >> + * Enables multiple rules reference the same action by handle (see > > Enables -> Allow > > >> + * struct rte_flow_shared_action). > >> + */ > >> + RTE_FLOW_ACTION_TYPE_SHARED, > >> }; > >> /** > >> @@ -2660,6 +2669,20 @@ struct rte_flow_action_set_dscp { > >> uint8_t dscp; > >> }; > >> + > >> +/** > >> + * RTE_FLOW_ACTION_TYPE_SHARED > >> + * > >> + * Opaque type returned after successfully creating a shared action. > >> + * > >> + * This handle can be used to manage and query the related action: > >> + * - share it a cross multiple flow rules > >> + * - update action configuration > >> + * - query action data > >> + * - destroy action > >> + */ > >> +struct rte_flow_shared_action; > >> + > >> /* Mbuf dynamic field offset for metadata. */ > >> extern int32_t rte_flow_dynf_metadata_offs; > >> @@ -3324,6 +3347,129 @@ int > >> rte_flow_get_aged_flows(uint16_t port_id, void **contexts, > >> uint32_t nb_contexts, struct rte_flow_error *error); > >> +/** > >> + * @warning > >> + * @b EXPERIMENTAL: this API may change without prior notice. > >> + * > >> + * Create shared action for reuse in multiple flow rules. > >> + * > >> + * @param[in] port_id > >> + * The port identifier of the Ethernet device. > >> + * @param[in] action > >> + * Action configuration for shared action creation. > >> + * @param[out] error > >> + * Perform verbose error reporting if not NULL. PMDs initialize this > >> + * structure in case of error only. > >> + * @return > >> + * A valid handle in case of success, NULL otherwise and rte_errno > >> is set > >> + * to one of the error codes defined: > >> + * - (ENOSYS) if underlying device does not support this functionality. > >> + * - (EIO) if underlying device is removed. > >> + * - (EINVAL) if *action* invalid. > >> + * - (ENOTSUP) if *action* valid but unsupported. > >> + */ > >> +__rte_experimental > >> +struct rte_flow_shared_action * > >> +rte_flow_shared_action_create(uint16_t port_id, > >> + const struct rte_flow_action *action, > >> + struct rte_flow_error *error); > >> + > >> +/** > >> + * @warning > >> + * @b EXPERIMENTAL: this API may change without prior notice. > >> + * > >> + * Destroys the shared action by handle. > > Destroys -> Destroy > > >> + * > >> + * @param[in] port_id > >> + * The port identifier of the Ethernet device. > >> + * @param[in] action > >> + * Handle for the shared action to be destroyed. > >> + * @param[out] error > >> + * Perform verbose error reporting if not NULL. PMDs initialize this > >> + * structure in case of error only. > >> + * @return > >> + * - (0) if success. > >> + * - (-ENOSYS) if underlying device does not support this functionality. > >> + * - (-EIO) if underlying device is removed. > >> + * - (-ENOENT) if action pointed by *action* handle was not found. > >> + * - (-ETOOMANYREFS) if action pointed by *action* handle still used > >> by one or > >> + * more rules > >> + * rte_errno is also set. > >> + */ > >> +__rte_experimental > >> +int > >> +rte_flow_shared_action_destroy(uint16_t port_id, > >> + struct rte_flow_shared_action *action, > >> + struct rte_flow_error *error); > >> + > >> +/** > >> + * @warning > >> + * @b EXPERIMENTAL: this API may change without prior notice. > >> + * > >> + * Updates inplace the shared action configuration pointed by > >> *action* handle > > Updates -> Update > inplace -> in-place > > >> + * with the configuration provided as *update* argument. > >> + * The update of the shared action configuration effects all flow > >> rules reusing > > May be: reusing -> using > > >> + * the action via handle. > > The interesting question what to do, if some rule cannot have a > new action (i.e. new action is invalid for a rule). > May be it is better to highlight it. > > >> + * > >> + * @param[in] port_id > >> + * The port identifier of the Ethernet device. > >> + * @param[in] action > >> + * Handle for the shared action to be updated. > >> + * @param[in] update > >> + * Action specification used to modify the action pointed by handle. > >> + * *update* should be of same type with the action pointed by the > >> *action* > >> + * handle argument, otherwise considered as invalid. > > Why? If it is a generic rule, it should be checked on a generic > ethdev layer, but I would not impose the limitation. If PMD > may change it, why generic interface specification should > restrict it. > > >> + * @param[out] error > >> + * Perform verbose error reporting if not NULL. PMDs initialize this > >> + * structure in case of error only. > >> + * @return > >> + * - (0) if success. > >> + * - (-ENOSYS) if underlying device does not support this functionality. > >> + * - (-EIO) if underlying device is removed. > >> + * - (-EINVAL) if *update* invalid. > >> + * - (-ENOTSUP) if *update* valid but unsupported. > >> + * - (-ENOENT) if action pointed by *ctx* was not found. > >> + * rte_errno is also set. > >> + */ > >> +__rte_experimental > >> +int > >> +rte_flow_shared_action_update(uint16_t port_id, > >> + struct rte_flow_shared_action *action, > >> + const struct rte_flow_action *update, > >> + struct rte_flow_error *error); > >> + > >> +/** > >> + * @warning > >> + * @b EXPERIMENTAL: this API may change without prior notice. > >> + * > >> + * Query the shared action by handle. > >> + * > >> + * This function allows retrieving action-specific data such as > >> counters. > > "This function allows retrieving" -> Retrieve or Get or Query > > >> + * Data is gathered by special action which may be present/referenced in > >> + * more than one flow rule definition. > >> + * > >> + * \see RTE_FLOW_ACTION_TYPE_COUNT > >> + * > >> + * @param port_id > >> + * Port identifier of Ethernet device. > >> + * @param[in] action > >> + * Handle for the shared action to query. > >> + * @param[in, out] data > >> + * Pointer to storage for the associated query data type. > >> + * @param[out] error > >> + * Perform verbose error reporting if not NULL. PMDs initialize this > >> + * structure in case of error only. > >> + * > >> + * @return > >> + * 0 on success, a negative errno value otherwise and rte_errno is set. > >> + */ > >> +__rte_experimental > >> +int > >> +rte_flow_shared_action_query(uint16_t port_id, > >> + const struct rte_flow_shared_action *action, > >> + void *data, > >> + struct rte_flow_error *error); > >> + > >> #ifdef __cplusplus > >> } > >> #endif > >> diff --git a/lib/librte_ethdev/rte_flow_driver.h > >> b/lib/librte_ethdev/rte_flow_driver.h > >> index 881cc469b7..a2cae1b53c 100644 > >> --- a/lib/librte_ethdev/rte_flow_driver.h > >> +++ b/lib/librte_ethdev/rte_flow_driver.h > >> @@ -107,6 +107,28 @@ struct rte_flow_ops { > >> void **context, > >> uint32_t nb_contexts, > >> struct rte_flow_error *err); > >> + /** See rte_flow_shared_action_create() */ > >> + struct rte_flow_shared_action *(*shared_action_create) > >> + (struct rte_eth_dev *dev, > >> + const struct rte_flow_action *action, > >> + struct rte_flow_error *error); > >> + /** See rte_flow_shared_action_destroy() */ > >> + int (*shared_action_destroy) > >> + (struct rte_eth_dev *dev, > >> + struct rte_flow_shared_action *shared_action, > >> + struct rte_flow_error *error); > >> + /** See rte_flow_shared_action_update() */ > >> + int (*shared_action_update) > >> + (struct rte_eth_dev *dev, > >> + struct rte_flow_shared_action *shared_action, > >> + const struct rte_flow_action *update, > >> + struct rte_flow_error *error); > >> + /** See rte_flow_shared_action_query() */ > >> + int (*shared_action_query) > >> + (struct rte_eth_dev *dev, > >> + const struct rte_flow_shared_action *shared_action, > >> + void *data, > >> + struct rte_flow_error *error); > >> }; > >> /** > >> > > > > The modification of "struct rte_flow_ops", looks fine. > > Acked-by: Ray Kinsella <m...@ashroe.eu> > > >
