Re: [dpdk-dev] [PATCH v2 1/6] ethdev: add flow shared action API

[Andrey Vesnovaty <andreyv@nvidia.com>]

Hi, Andrew,

I'm back to this patch set to make it into 20.11. 

Most of the items you raised regarding SW implementation already discussed
with Jerin [1] and answered by Ori [2] and me [3]. Please follow my & Ori
answer. In general, the idea of SW implementation for shared action looks
nice from application perspective but the changes required in ethdev/rte_flow
generic layer about to impact existing rte_flow APIs.

Please follow my [3] & Ori's [2] answers & update on your suggestion to the
Proposed API.

I'll take your suggestions for rephrasing in upcoming patches version (V3).

Thanks,
Andrey
---
[1] https://www.mail-archive.com/dev@dpdk.org/msg173461.html
[2] https://www.mail-archive.com/dev@dpdk.org/msg173478.html
[3] https://www.mail-archive.com/dev@dpdk.org/msg172921.html

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