Re: [dpdk-dev] [PATCH v2 1/4] ethdev: introduce indirect action APIs

[Andrey Vesnovaty <andreyv@nvidia.com>]

> -----Original Message-----
> From: Bing Zhao <bingz@nvidia.com>
> Sent: Saturday, April 10, 2021 5:04 PM
> To: Ori Kam <orika@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> andrew.rybchenko@oktetlabs.ru; Matan Azrad <matan@nvidia.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>
> Cc: dev@dpdk.org; ajit.khaparde@broadcom.com; Gregory Etelson
> <getelson@nvidia.com>; Andrey Vesnovaty <andreyv@nvidia.com>
> Subject: [PATCH v2 1/4] ethdev: introduce indirect action APIs
> 
> Right now, rte_flow_shared_action_* APIs are used for some shared
> actions, like RSS, count. The shared action should be created before
> using it inside a flow. These shared actions sometimes are not
> really shared but just some indirect actions decoupled from a flow.
> 
> The new functions rte_flow_action_handle_* are added to replace
> the current shared functions rte_flow_shared_action_*.
> 
> There are two types of flow actions:
> 1. the direct (normal) actions that could be created and stored
>    within a flow rule. Such action is tied to its flow rule and
>    cannot be reused.
> 2. the indirect action, in the past, named shared_action. It is
>    created from a direct actioni, like count or rss, and then used
>    in the flow rules with an object handle. The PMD will take care
>    of the retrieve from indirect action to the direct action
>    when it is referenced.
> 
> The indirect action is accessed (update / query) w/o any flow rule,
> just via the action object handle. For example, when querying or
> resetting a counter, it could be done out of any flow using this
> counter, but only the handle of the counter action object is
> required.
> The indirect action object could be shared by different flows or
> used by a single flow, depending on the direct action type and
> the real-life requirements.
> The handle of an indirect action object is opaque and defined in
> each driver and possibly different per direct action type.
> 
> The old name "shared" is improper in a sense and should be replaced.
> 
> All the command lines in testpmd application with "shared_action*"
> are replaced with "indirect_action*".
> 
> The parameter of "update" interface is also changed. A general
> pointer will replace the rte_flow_action struct pointer due to the
> facts:
> 1. Some action may not support fields updating. In the example of a
>    counter, the only "update" supported should be the reset. So
>    passing a rte_flow_action struct pointer is meaningless and
>    there is even no such corresponding action struct. What's more,
>    if more than one operations should be supported, for some other
>    action, such pointer parameter may not meet the need.
> 2. Some action may need conditional or partial update, the current
>    parameter will not provide the ability to indicate which part(s)
>    to update.
>    For different types of indirect action objects, the pointer could
>    either be the same of rte_flow_action* struct - in order not to
>    break the current driver implementation, or some wrapper
>    structures with bits as masks to indicate which part to be
>    updated, depending on real needs of the corresponding direct
>    action. For different direct actions, the structures of indirect
>    action objects updating will be different.
> 
> All the underlayer PMD callbacks will be moved to these new APIs.
> 
> The RTE_FLOW_ACTION_TYPE_SHARED is kept for now in order not to
> break the ABI. All the implementations are changed by using
> RTE_FLOW_ACTION_TYPE_INDIRECT.
> 
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> ---
>  doc/guides/rel_notes/release_21_05.rst |   3 +
>  lib/librte_ethdev/rte_flow.c           |  56 ++++++++--------
>  lib/librte_ethdev/rte_flow.h           | 118 +++++++++++++++++++--------------
>  lib/librte_ethdev/rte_flow_driver.h    |  26 ++++----
>  lib/librte_ethdev/version.map          |   8 +--
>  5 files changed, 115 insertions(+), 96 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_21_05.rst
> b/doc/guides/rel_notes/release_21_05.rst
> index 374d6d9..6c0ac46 100644
> --- a/doc/guides/rel_notes/release_21_05.rst
> +++ b/doc/guides/rel_notes/release_21_05.rst
> @@ -164,6 +164,9 @@ API Changes
>    from ``rte_thread_tls_*`` to ``rte_thread_*`` to avoid naming redundancy
>    and confusion with the transport layer security term.
> 
> +* ethdev: The experimental shared action APIs in ``rte_flow.h`` has been
> +  replaced from ``rte_flow_shared_action_*`` to indirect action APIs named
> +  ``rte_flow_action_handle_*``.
> 
>  ABI Changes
>  -----------
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index e07e617..27a1615 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -180,12 +180,12 @@ struct rte_flow_desc_data {
>  	MK_FLOW_ACTION(MODIFY_FIELD,
>  		       sizeof(struct rte_flow_action_modify_field)),
>  	/**
> -	 * Shared action represented as handle of type
> -	 * (struct rte_flow_shared action *) stored in conf field (see
> +	 * Indirect action represented as handle of type
> +	 * (struct rte_flow_action_handle *) stored in conf field (see
>  	 * struct rte_flow_action); no need for additional structure to * store
> -	 * shared action handle.
> +	 * indirect action handle.
>  	 */
> -	MK_FLOW_ACTION(SHARED, 0),
> +	MK_FLOW_ACTION(INDIRECT, 0),
>  };
> 
>  int
> @@ -1067,53 +1067,53 @@ enum rte_flow_conv_item_spec_type {
>  				  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,
> +struct rte_flow_action_handle *
> +rte_flow_action_handle_create(uint16_t port_id,
> +			      const struct rte_flow_indir_action_conf *conf,
>  			      const struct rte_flow_action *action,
>  			      struct rte_flow_error *error)
>  {
> -	struct rte_flow_shared_action *shared_action;
> +	struct rte_flow_action_handle *handle;
>  	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> 
>  	if (unlikely(!ops))
>  		return NULL;
> -	if (unlikely(!ops->shared_action_create)) {
> +	if (unlikely(!ops->action_handle_create)) {
>  		rte_flow_error_set(error, ENOSYS,
>  				   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> NULL,
>  				   rte_strerror(ENOSYS));
>  		return NULL;
>  	}
> -	shared_action = ops->shared_action_create(&rte_eth_devices[port_id],
> -						  conf, action, error);
> -	if (shared_action == NULL)
> +	handle = ops->action_handle_create(&rte_eth_devices[port_id],
> +					   conf, action, error);
> +	if (handle == NULL)
>  		flow_err(port_id, -rte_errno, error);
> -	return shared_action;
> +	return handle;
>  }
> 
>  int
> -rte_flow_shared_action_destroy(uint16_t port_id,
> -			      struct rte_flow_shared_action *action,
> -			      struct rte_flow_error *error)
> +rte_flow_action_handle_destroy(uint16_t port_id,
> +			       struct rte_flow_action_handle *handle,
> +			       struct rte_flow_error *error)
>  {
>  	int ret;
>  	const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> 
>  	if (unlikely(!ops))
>  		return -rte_errno;
> -	if (unlikely(!ops->shared_action_destroy))
> +	if (unlikely(!ops->action_handle_destroy))
>  		return rte_flow_error_set(error, ENOSYS,
> 
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  					  NULL, rte_strerror(ENOSYS));
> -	ret = ops->shared_action_destroy(&rte_eth_devices[port_id], action,
> -					 error);
> +	ret = ops->action_handle_destroy(&rte_eth_devices[port_id],
> +					 handle, error);
>  	return flow_err(port_id, ret, error);
>  }
> 
>  int
> -rte_flow_shared_action_update(uint16_t port_id,
> -			      struct rte_flow_shared_action *action,
> -			      const struct rte_flow_action *update,
> +rte_flow_action_handle_update(uint16_t port_id,
> +			      struct rte_flow_action_handle *handle,
> +			      const void *update,
>  			      struct rte_flow_error *error)
>  {
>  	int ret;
> @@ -1121,18 +1121,18 @@ struct rte_flow_shared_action *
> 
>  	if (unlikely(!ops))
>  		return -rte_errno;
> -	if (unlikely(!ops->shared_action_update))
> +	if (unlikely(!ops->action_handle_update))
>  		return rte_flow_error_set(error, ENOSYS,
> 
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  					  NULL, rte_strerror(ENOSYS));
> -	ret = ops->shared_action_update(&rte_eth_devices[port_id], action,
> +	ret = ops->action_handle_update(&rte_eth_devices[port_id], handle,
>  					update, error);
>  	return flow_err(port_id, ret, error);
>  }
> 
>  int
> -rte_flow_shared_action_query(uint16_t port_id,
> -			     const struct rte_flow_shared_action *action,
> +rte_flow_action_handle_query(uint16_t port_id,
> +			     const struct rte_flow_action_handle *handle,
>  			     void *data,
>  			     struct rte_flow_error *error)
>  {
> @@ -1141,11 +1141,11 @@ struct rte_flow_shared_action *
> 
>  	if (unlikely(!ops))
>  		return -rte_errno;
> -	if (unlikely(!ops->shared_action_query))
> +	if (unlikely(!ops->action_handle_query))
>  		return rte_flow_error_set(error, ENOSYS,
> 
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  					  NULL, rte_strerror(ENOSYS));
> -	ret = ops->shared_action_query(&rte_eth_devices[port_id], action,
> +	ret = ops->action_handle_query(&rte_eth_devices[port_id], handle,
>  				       data, error);
>  	return flow_err(port_id, ret, error);
>  }
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 6cc5713..91ae25b 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1821,7 +1821,7 @@ enum rte_flow_action_type {
>  	 * Enables counters for this flow rule.
>  	 *
>  	 * These counters can be retrieved and reset through rte_flow_query()
> or
> -	 * rte_flow_shared_action_query() if the action provided via handle,
> +	 * rte_flow_action_handle_query() if the action provided via handle,
>  	 * see struct rte_flow_query_count.
>  	 *
>  	 * See struct rte_flow_action_count.
> @@ -2267,6 +2267,16 @@ enum rte_flow_action_type {
>  	 * See struct rte_flow_action_modify_field.
>  	 */
>  	RTE_FLOW_ACTION_TYPE_MODIFY_FIELD,
> +
> +	/**
> +	 * Describe indirect action that could be used by a single flow rule
> +	 * or multiple flow rules.
> +	 *
> +	 * Allow flow rule(s) reference the same action by the indirect action
> +	 * handle (see struct rte_flow_action_handle), rules could be on the
> +	 * same port or across different ports.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_INDIRECT,
>  };
> 
>  /**
> @@ -2357,7 +2367,7 @@ struct rte_flow_query_age {
>   * ``struct rte_flow_query_count``.
>   *
>   * @deprecated Shared attribute is deprecated, use generic
> - * RTE_FLOW_ACTION_TYPE_SHARED action.
> + * RTE_FLOW_ACTION_TYPE_INDIRECT action.
>   *
>   * The shared flag indicates whether the counter is unique to the flow rule the
>   * action is specified with, or whether it is a shared counter.
> @@ -2847,17 +2857,23 @@ struct rte_flow_action_set_dscp {
>  };
> 
>  /**
> - * RTE_FLOW_ACTION_TYPE_SHARED
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_INDIRECT
>   *
> - * Opaque type returned after successfully creating a shared action.
> + * Opaque type returned after successfully creating an indirect action object.
> + * The definition of the object handle will be different per driver or
> + * per immediate action type.
>   *
> - * 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
> + * This handle can be used to manage and query the related immediate action:
> + * - referenced in single flow rule or across multiple flow rules
> + *   over multiple ports
> + * - update action object configuration
> + * - query action object data
> + * - destroy action object
>   */
> -struct rte_flow_shared_action;
> +struct rte_flow_action_handle;
> 
>  /**
>   * Field IDs for MODIFY_FIELD action.
> @@ -3628,25 +3644,22 @@ struct rte_flow_desc {
>  			uint32_t nb_contexts, struct rte_flow_error *error);
> 
>  /**
> - * Specify shared action configuration
> + * Specify indirect action object configuration
>   */
> -struct rte_flow_shared_action_conf {
> +struct rte_flow_indir_action_conf {
>  	/**
> -	 * Flow direction for shared action configuration.
> +	 * Flow direction for the indirect action configuration.
>  	 *
> -	 * Shared action should be valid at least for one flow direction,
> +	 * 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. */
> -
>  	/**
>  	 * When set to 1, indicates that the action is valid for
>  	 * transfer traffic; otherwise, for non-transfer traffic.
> -	 *
> -	 * See struct rte_flow_attr.
>  	 */
>  	uint32_t transfer:1;
>  };
> @@ -3655,16 +3668,17 @@ struct rte_flow_shared_action_conf {
>   * @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.
> + * Create an indirect action object that can be used by flow create, and
> + * could also be shared by different flows.
> + * The created object handle has single state and configuration
> + * across all the flow rules using it.
>   *
>   * @param[in] port_id
>   *    The port identifier of the Ethernet device.
>   * @param[in] conf
> - *   Shared action configuration.
> + *   Action configuration for the indirect action object creation.
>   * @param[in] action
> - *   Action configuration for shared action creation.
> + *   Specific configuration of the indirect action object.
>   * @param[out] error
>   *   Perform verbose error reporting if not NULL. PMDs initialize this
>   *   structure in case of error only.
> @@ -3678,9 +3692,9 @@ struct rte_flow_shared_action_conf {
>   *   - (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,
> +struct rte_flow_action_handle *
> +rte_flow_action_handle_create(uint16_t port_id,
> +			      const struct rte_flow_indir_action_conf *conf,
>  			      const struct rte_flow_action *action,
>  			      struct rte_flow_error *error);
> 
> @@ -3688,12 +3702,12 @@ struct rte_flow_shared_action *
>   * @warning
>   * @b EXPERIMENTAL: this API may change without prior notice.
>   *
> - * Destroy the shared action by handle.
> + * Destroy indirect 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[in] handle
> + *   Handle for the indirect action object to be destroyed.
>   * @param[out] error
>   *   Perform verbose error reporting if not NULL. PMDs initialize this
>   *   structure in case of error only.
> @@ -3708,27 +3722,30 @@ struct rte_flow_shared_action *
>   */
>  __rte_experimental
>  int
> -rte_flow_shared_action_destroy(uint16_t port_id,
> -			       struct rte_flow_shared_action *action,
> +rte_flow_action_handle_destroy(uint16_t port_id,
> +			       struct rte_flow_action_handle *handle,
>  			       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.
> + * Update in-place the action configuration and / or state pointed
> + * by action *handle* with the configuration provided as *update* argument.
> + * The update of the action configuration effects all flow rules reusing
> + * the action via *handle*.
> + * The update general pointer provides the ability of partial updating.
>   *
>   * @param[in] port_id
>   *    The port identifier of the Ethernet device.
> - * @param[in] action
> - *   Handle for the shared action to be updated.
> + * @param[in] handle
> + *   Handle for the indirect action object 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.
> + *   Update profile specification used to modify the action pointed by handle.
> + *   *update* could be with the same type of the immediate action
> corresponding
> + *   to the *handle* argument when creating, or a wrapper structure includes
> + *   action configuration to be updated and bit fields to indicate the member
> + *   of fields inside the action to update.
>   * @param[out] error
>   *   Perform verbose error reporting if not NULL. PMDs initialize this
>   *   structure in case of error only.
> @@ -3739,32 +3756,32 @@ struct rte_flow_shared_action *
>   *   - (-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.
> + *   - (-ENOENT) if indirect action object pointed by *handle* 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,
> +rte_flow_action_handle_update(uint16_t port_id,
> +			      struct rte_flow_action_handle *handle,
> +			      const void *update,
>  			      struct rte_flow_error *error);
> 
>  /**
>   * @warning
>   * @b EXPERIMENTAL: this API may change without prior notice.
>   *
> - * Query the shared action by handle.
> + * Query the direct action by corresponding indirect action object 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
> + * @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] handle
> + *   Handle for the action object to query.
>   * @param[in, out] data
>   *   Pointer to storage for the associated query data type.
>   * @param[out] error
> @@ -3776,10 +3793,9 @@ struct rte_flow_shared_action *
>   */
>  __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);
> +rte_flow_action_handle_query(uint16_t port_id,
> +			     const struct rte_flow_action_handle *handle,
> +			     void *data, struct rte_flow_error *error);
> 
>  /* Tunnel has a type and the key information. */
>  struct rte_flow_tunnel {
> diff --git a/lib/librte_ethdev/rte_flow_driver.h
> b/lib/librte_ethdev/rte_flow_driver.h
> index da594d9..8d825eb 100644
> --- a/lib/librte_ethdev/rte_flow_driver.h
> +++ b/lib/librte_ethdev/rte_flow_driver.h
> @@ -83,27 +83,27 @@ 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)
> +	/** See rte_flow_action_handle_create() */
> +	struct rte_flow_action_handle *(*action_handle_create)
>  		(struct rte_eth_dev *dev,
> -		 const struct rte_flow_shared_action_conf *conf,
> +		 const struct rte_flow_indir_action_conf *conf,
>  		 const struct rte_flow_action *action,
>  		 struct rte_flow_error *error);
> -	/** See rte_flow_shared_action_destroy() */
> -	int (*shared_action_destroy)
> +	/** See rte_flow_action_handle_destroy() */
> +	int (*action_handle_destroy)
>  		(struct rte_eth_dev *dev,
> -		 struct rte_flow_shared_action *shared_action,
> +		 struct rte_flow_action_handle *handle,
>  		 struct rte_flow_error *error);
> -	/** See rte_flow_shared_action_update() */
> -	int (*shared_action_update)
> +	/** See rte_flow_action_handle_update() */
> +	int (*action_handle_update)
>  		(struct rte_eth_dev *dev,
> -		 struct rte_flow_shared_action *shared_action,
> -		 const struct rte_flow_action *update,
> +		 struct rte_flow_action_handle *handle,
> +		 const void *update,
>  		 struct rte_flow_error *error);
> -	/** See rte_flow_shared_action_query() */
> -	int (*shared_action_query)
> +	/** See rte_flow_action_handle_query() */
> +	int (*action_handle_query)
>  		(struct rte_eth_dev *dev,
> -		 const struct rte_flow_shared_action *shared_action,
> +		 const struct rte_flow_action_handle *handle,
>  		 void *data,
>  		 struct rte_flow_error *error);
>  	/** See rte_flow_tunnel_decap_set() */
> diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map
> index 93ad388..4eb561a 100644
> --- a/lib/librte_ethdev/version.map
> +++ b/lib/librte_ethdev/version.map
> @@ -231,10 +231,6 @@ EXPERIMENTAL {
>  	rte_eth_fec_get_capability;
>  	rte_eth_fec_get;
>  	rte_eth_fec_set;
> -	rte_flow_shared_action_create;
> -	rte_flow_shared_action_destroy;
> -	rte_flow_shared_action_query;
> -	rte_flow_shared_action_update;
>  	rte_flow_tunnel_decap_set;
>  	rte_flow_tunnel_match;
>  	rte_flow_get_restore_info;
> @@ -246,6 +242,10 @@ EXPERIMENTAL {
> 
>  	# added in 21.05
>  	rte_eth_representor_info_get;
> +	rte_flow_action_handle_create;
> +	rte_flow_action_handle_destroy;
> +	rte_flow_action_handle_update;
> +	rte_flow_action_handle_query;
>  };
> 
>  INTERNAL {
> --
> 1.8.3.1


Acked-by: Andrey Vesnovaty <andreyv@nvidia.com>