RE: [PATCH 1/2] ethdev: add query_update sync and async function calls

[Ori Kam <orika@nvidia.com>]

Hi Gregory,

Some comments below,
Also you are missing the release rst file update.

> -----Original Message-----
> From: Gregory Etelson <getelson@nvidia.com>
> Sent: Wednesday, 21 December 2022 9:36
> 
> Current API allows either query or update indirect flow action.
> If port hardware allows both update and query in a single operation,
> application still has to issue 2 separate hardware requests.
> 
> The patch adds `rte_flow_action_handle_query_update` function call,
> and it's async version `rte_flow_async_action_handle_query_update`
> to atomically query and update flow action.
> 
> int
> rte_flow_action_handle_query_update
>        (uint16_t port_id, struct rte_flow_action_handle *handle,
>         const void *update, void *query,
>         enum rte_flow_query_update_mode mode,
>         struct rte_flow_error *error);
> 
> int
> rte_flow_async_action_handle_query_update
>        (uint16_t port_id, uint32_t queue_id,
>         const struct rte_flow_op_attr *op_attr,
>         struct rte_flow_action_handle *action_handle,
>         const void *update, void *query,
>         enum rte_flow_query_update_mode mode,
>         void *user_data, struct rte_flow_error *error);
> 
> Application can control query and update order, if that is supported
> by port hardware, by setting qu_mode parameter to
> RTE_FLOW_QU_QUERY_FIRST or RTE_FLOW_QU_UPDATE_FIRST.
> 
> RTE_FLOW_QU_QUERY and RTE_FLOW_QU_UPDATE parameter values
> provide
> query only and update only functionality for backward compatibility
> with existing API.
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> ---
>  lib/ethdev/rte_flow.c        |  39 +++++++++++++
>  lib/ethdev/rte_flow.h        | 105 +++++++++++++++++++++++++++++++++++
>  lib/ethdev/rte_flow_driver.h |  15 +++++
>  lib/ethdev/version.map       |   5 ++
>  4 files changed, 164 insertions(+)
> 
> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index 7d0c24366c..8b8aa940be 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -1883,3 +1883,42 @@ rte_flow_async_action_handle_query(uint16_t
> port_id,
>  					  action_handle, data, user_data,
> error);
>  	return flow_err(port_id, ret, error);
>  }
> +
> +int
> +rte_flow_action_handle_query_update(uint16_t port_id,
> +				    struct rte_flow_action_handle *handle,
> +				    const void *update, void *query,
> +				    enum rte_flow_query_update_mode
> mode,
> +				    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);
> +	int ret;
> +
> +	if (!ops || !ops->action_handle_query_update)
> +		return -ENOTSUP;
> +	ret = ops->action_handle_query_update(dev, handle, update, query,
> +					      mode, error);
> +	return flow_err(port_id, ret, error);
> +}
> +
> +int
> +rte_flow_async_action_handle_query_update(uint16_t port_id, uint32_t
> queue_id,
> +					  const struct rte_flow_op_attr *attr,
> +					  struct rte_flow_action_handle
> *handle,
> +					  const void *update, void *query,
> +					  enum
> rte_flow_query_update_mode mode,
> +					  void *user_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);
> +	int ret;
> +
> +	if (!ops || !ops->async_action_handle_query_update)
> +		return -ENOTSUP;
> +	ret = ops->async_action_handle_query_update(dev, queue_id, attr,
> +						    handle, update, query,
> mode,
> +						    user_data, error);
> +	return flow_err(port_id, ret, error);
> +}
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index b60987db4b..f9e919bb80 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -5622,6 +5622,111 @@ rte_flow_async_action_handle_query(uint16_t
> port_id,
>  		void *user_data,
>  		struct rte_flow_error *error);
> 
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Query_update operational mode.
> + *
> + * RTE_FLOW_QU_DEFAULT
> + *   Default query_update operational mode.
> + *   If both `update` and `query` parameters are not NULL the call updates
> and

I don't think the above line is relevant.

> + *   queries action in default port order.
> + *   If `update` parameter is NULL the call queries action.
> + *   If `query` parameter is NULL the call updates action.
> + * RTE_FLOW_QU_QUERY_FIRST
> + *   Force port to query action before update.
> + * RTE_FLOW_QU_UPDATE_FIRST
> + *   Force port to update action before update.
> + *
> + * @see rte_flow_action_handle_query_update()
> + * @see rte_flow_async_action_handle_query_update()
> + */
> +enum rte_flow_query_update_mode {
> +	RTE_FLOW_QU_DEFAULT,      /* HW default mode  */

I don't think this is valid. Since if the application is not aware of what is the default
it can't assume anything about the query value.
If we keep it means that each PMD must document what is the default.

> +	RTE_FLOW_QU_QUERY_FIRST,  /* query before update */
> +	RTE_FLOW_QU_UPDATE_FIRST, /* query after  update */
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Query and/or update indirect flow action.

I think better wording will be:
Atomic query and update.
This function can also be used just for query/update when the not needed
field is NULL.

@see rte_flow_action_handle_update.
@see rte_flow_action_handle_query

> + *
> + * @param[in] port_id
> + *   Port identifier of Ethernet device.
> + * @param[in] handle
> + *   Handle for the indirect action object to be updated.
> + * @param update[in]
> + *   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] query
> + *   Pointer to storage for the associated query data type.
> + * @param[in] mode
> + *   Operational mode.
> + * @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_action_handle_query_update(uint16_t port_id,
> +				    struct rte_flow_action_handle *handle,
> +				    const void *update, void *query,
> +				    enum rte_flow_query_update_mode
> mode,
> +				    struct rte_flow_error *error);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Enqueue async indirect flow action query and/or update
> + *

Same comments as above.

> + * @param[in] port_id
> + *   Port identifier of Ethernet device.
> + * @param[in] queue_id
> + *   Flow queue which is used to update the rule.
> + * @param[in] attr
> + *   Indirect action update operation attributes.
> + * @param[in] handle
> + *   Handle for the indirect action object to be updated.
> + * @param[in] update
> + *   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[in] query
> + *   Pointer to storage for the associated query data type.
> + *   Query result returned on async completion event.
> + * @param[in] mode
> + *   Operational mode.
> + * @param[in] user_data
> + *   The user data that will be returned on async completion event.
> + * @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_async_action_handle_query_update(uint16_t port_id, uint32_t
> queue_id,
> +					  const struct rte_flow_op_attr *attr,
> +					  struct rte_flow_action_handle
> *handle,
> +					  const void *update, void *query,
> +					  enum
> rte_flow_query_update_mode mode,
> +					  void *user_data,
> +					  struct rte_flow_error *error);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/ethdev/rte_flow_driver.h b/lib/ethdev/rte_flow_driver.h
> index c7d0699c91..7358c10a7a 100644
> --- a/lib/ethdev/rte_flow_driver.h
> +++ b/lib/ethdev/rte_flow_driver.h
> @@ -114,6 +114,13 @@ struct rte_flow_ops {
>  		 const struct rte_flow_action_handle *handle,
>  		 void *data,
>  		 struct rte_flow_error *error);
> +	/** See rte_flow_action_handle_query_update() */
> +	int (*action_handle_query_update)
> +		(struct rte_eth_dev *dev,
> +		 struct rte_flow_action_handle *handle,
> +		 const void *update, void *query,
> +		 enum rte_flow_query_update_mode qu_mode,
> +		 struct rte_flow_error *error);
>  	/** See rte_flow_tunnel_decap_set() */
>  	int (*tunnel_decap_set)
>  		(struct rte_eth_dev *dev,
> @@ -276,6 +283,14 @@ struct rte_flow_ops {
>  		 void *data,
>  		 void *user_data,
>  		 struct rte_flow_error *error);
> +	/** See rte_flow_async_action_handle_query_update */
> +	int (*async_action_handle_query_update)
> +		(struct rte_eth_dev *dev, uint32_t queue_id,
> +		 const struct rte_flow_op_attr *op_attr,
> +		 struct rte_flow_action_handle *action_handle,
> +		 const void *update, void *query,
> +		 enum rte_flow_query_update_mode qu_mode,
> +		 void *user_data, struct rte_flow_error *error);
>  };
> 
>  /**
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 17201fbe0f..42f0d7b30c 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -298,6 +298,11 @@ EXPERIMENTAL {
>  	rte_flow_get_q_aged_flows;
>  	rte_mtr_meter_policy_get;
>  	rte_mtr_meter_profile_get;
> +
> +	# future
> +	rte_flow_action_handle_query_update;
> +	rte_flow_async_action_handle_query_update;
> +
>  };
> 
>  INTERNAL {
> --
> 2.34.1

Best,
Ori