[v8,1/2] ethdev: add query_update sync and async function calls

Message ID 20230201151646.3500-1-getelson@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v8,1/2] ethdev: add query_update sync and async function calls |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Gregory Etelson Feb. 1, 2023, 3:16 p.m. UTC
  Current API allows either query or update indirect flow action.
If indirect action must be conditionally updated according to it's
present state application must first issue action query then
analyze returned data and if needed issue update request.
When the update will be processed, action state can change and
the update can invalidate the action.

Add `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.

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.

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
v2: Remove RTE_FLOW_QU_DEFAULT query-update mode.
v3: Update release release notes.
    Fix doxygen errors.
v4: Add returned errno codes.
v5: Update the patch description.
    Fix typos.
v6: Resolve merge conflict with the main branch.	
v7: Update documentation.
v8: Style fixes.
---
 doc/guides/rel_notes/release_23_03.rst |   8 ++
 lib/ethdev/rte_flow.c                  |  45 +++++++++++
 lib/ethdev/rte_flow.h                  | 103 +++++++++++++++++++++++++
 lib/ethdev/rte_flow_driver.h           |  15 ++++
 lib/ethdev/version.map                 |   5 ++
 5 files changed, 176 insertions(+)
  

Comments

Ori Kam Feb. 1, 2023, 5:30 p.m. UTC | #1
Hi Gregory,.


> -----Original Message-----
> From: Gregory Etelson <getelson@nvidia.com>
> Sent: Wednesday, 1 February 2023 17:17
> To: dev@dpdk.org
> Cc: Gregory Etelson <getelson@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>; Ori Kam
> <orika@nvidia.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@amd.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Subject: [PATCH v8 1/2] ethdev: add query_update sync and async function
> calls
> 
> Current API allows either query or update indirect flow action.
> If indirect action must be conditionally updated according to it's
> present state application must first issue action query then
> analyze returned data and if needed issue update request.
> When the update will be processed, action state can change and
> the update can invalidate the action.
> 
> Add `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.
> 
> 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.
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> ---
> v2: Remove RTE_FLOW_QU_DEFAULT query-update mode.
> v3: Update release release notes.
>     Fix doxygen errors.
> v4: Add returned errno codes.
> v5: Update the patch description.
>     Fix typos.
> v6: Resolve merge conflict with the main branch.
> v7: Update documentation.
> v8: Style fixes.
> ---

Acked-by: Ori Kam <orika@nvidia.com>
Best,
Ori
  
Andrew Rybchenko Feb. 2, 2023, 9:15 a.m. UTC | #2
query_update is bad for summary, consider something like
ethdev: add query and update sync and async API

On 2/1/23 18:16, Gregory Etelson wrote:
> Current API allows either query or update indirect flow action.
> If indirect action must be conditionally updated according to it's
> present state application must first issue action query then
> analyze returned data and if needed issue update request.
> When the update will be processed, action state can change and
> the update can invalidate the action.
> 
> Add `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.
> 
> 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.
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>

[snip]

> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> index 7d0c24366c..ece40e7877 100644
> --- a/lib/ethdev/rte_flow.c
> +++ b/lib/ethdev/rte_flow.c
> @@ -1883,3 +1883,48 @@ 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)
> +{
> +	int ret;
> +	struct rte_eth_dev *dev;
> +	const struct rte_flow_ops *ops;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];

(-EINVAL) if *handle* invalid or both *query* and *update* are NULL.

must be handled here. It is generic checks and we should do it
in a generic place to avoid duplication in PMD callbacks.

> +	ops = rte_flow_ops_get(port_id, error);
> +	if (!ops || !ops->action_handle_query_update)
> +		return -ENOTSUP;

May be it makes sense to use single-operation callbacks if
another operation input is NULL. I guess it could be
convenient in some cases. Just an idea.

> +	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)
> +{
> +	int ret;
> +	struct rte_eth_dev *dev;
> +	const struct rte_flow_ops *ops;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];

same here

> +	ops = rte_flow_ops_get(port_id, error);
> +	if (!ops || !ops->async_action_handle_query_update)
> +		return -ENOTSUP;

same here

> +	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..d3fbba5b99 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -5622,6 +5622,109 @@ 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 and update operational mode.
> + *
> + * RTE_FLOW_QU_QUERY_FIRST
> + *   Force port to query action before update.
> + * RTE_FLOW_QU_UPDATE_FIRST
> + *   Force port to update action before query.

I'm sorry, but I strongly dislike enum members duplication
here. I don't understand why we need to duplicate it and why
we can't document it in a right way below.

> + *
> + * @see rte_flow_action_handle_query_update()
> + * @see rte_flow_async_action_handle_query_update()
> + */
> +enum rte_flow_query_update_mode {
> +	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.
> + * If both query and update not NULL, the function atomically
> + * queries and updates indirect action. Query and update are carried in order
> + * specified in the mode parameter.
> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param handle
> + *   Handle for the indirect action object to be updated.
> + * @param update
> + *   If not NULL, update profile specification used to modify the action
> + *   pointed by handle.
> + * @param query
> + *   If not NULL pointer to storage for the associated query data type.
> + * @param mode
> + *   Operational mode.
> + *   Required if both *update* and *query* are not NULL.

It should be a part of generic checks in
rte_flow_action_handle_query_update() implementation.

> + * @param 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.
> + * - (-ENODEV) if *port_id* invalid.
> + * - (-ENOTSUP) if underlying device does not support this functionality.
> + * - (-EINVAL) if *handle* invalid or both *query* and *update* are NULL.
> + */
> +__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
> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param queue_id
> + *   Flow queue which is used to update the rule.
> + * @param attr
> + *   Indirect action update operation attributes.
> + * @param handle
> + *   Handle for the indirect action object to be updated.
> + * @param update
> + *   If not NULL, update profile specification used to modify the action
> + *   pointed by handle.
> + * @param query
> + *   If not NULL, pointer to storage for the associated query data type.
> + *   Query result returned on async completion event.
> + * @param mode
> + *   Operational mode.
> + *   Required if both *update* and *query* are not NULL.

It should be a part of generic checks in
rte_flow_action_handle_query_update() implementation.

> + * @param user_data
> + *   The user data that will be returned on async completion event.
> + * @param 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.
> + * - (-ENODEV) if *port_id* invalid.
> + * - (-ENOTSUP) if underlying device does not support this functionality.
> + * - (-EINVAL) if *handle* invalid or both *update* and *query* are NULL.
> + */
> +__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

[snip]
  
Gregory Etelson Feb. 2, 2023, 10:24 a.m. UTC | #3
Hello Andrew,

Please check out my comments below.

Regards,
Gregory

> 
> 
> query_update is bad for summary, consider something like
> ethdev: add query and update sync and async API
>

Will fix in v9.
 
> On 2/1/23 18:16, Gregory Etelson wrote:
> > Current API allows either query or update indirect flow action.
> > If indirect action must be conditionally updated according to it's
> > present state application must first issue action query then
> > analyze returned data and if needed issue update request.
> > When the update will be processed, action state can change and
> > the update can invalidate the action.
> >
> > Add `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.
> >
> > 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.
> >
> > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> 
> [snip]
> 
> > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> > index 7d0c24366c..ece40e7877 100644
> > --- a/lib/ethdev/rte_flow.c
> > +++ b/lib/ethdev/rte_flow.c
> > @@ -1883,3 +1883,48 @@ 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)
> > +{
> > +     int ret;
> > +     struct rte_eth_dev *dev;
> > +     const struct rte_flow_ops *ops;
> > +
> > +     RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +     dev = &rte_eth_devices[port_id];
> 
> (-EINVAL) if *handle* invalid or both *query* and *update* are NULL.
> 
> must be handled here. It is generic checks and we should do it
> in a generic place to avoid duplication in PMD callbacks.
> 

Will fix in v9.

> > +     ops = rte_flow_ops_get(port_id, error);
> > +     if (!ops || !ops->action_handle_query_update)
> > +             return -ENOTSUP;
> 
> May be it makes sense to use single-operation callbacks if
> another operation input is NULL. I guess it could be
> convenient in some cases. Just an idea.
> 

There is a plan to deprecate indirect query and update functions
and replace them with a single query_update.

> > +     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)
> > +{
> > +     int ret;
> > +     struct rte_eth_dev *dev;
> > +     const struct rte_flow_ops *ops;
> > +
> > +     RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +     dev = &rte_eth_devices[port_id];
> 
> same here
> 

Will fix in v9.

> > +     ops = rte_flow_ops_get(port_id, error);
> > +     if (!ops || !ops->async_action_handle_query_update)
> > +             return -ENOTSUP;
> 
> same here
>

Will fix in v9.
 
> > +     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..d3fbba5b99 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -5622,6 +5622,109 @@
> 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 and update operational mode.
> > + *
> > + * RTE_FLOW_QU_QUERY_FIRST
> > + *   Force port to query action before update.
> > + * RTE_FLOW_QU_UPDATE_FIRST
> > + *   Force port to update action before query.
> 
> I'm sorry, but I strongly dislike enum members duplication
> here. I don't understand why we need to duplicate it and why
> we can't document it in a right way below.
> 

I don't understand where the duplication is.
Query and update operations are atomic for application, 
but in hardware these are 2 separate operations that reference hardware object.
The operations execution order can have different results on object state. 
When application asks both actions it must explicitly specify execution order for hardware.

> > + *
> > + * @see rte_flow_action_handle_query_update()
> > + * @see rte_flow_async_action_handle_query_update()
> > + */
> > +enum rte_flow_query_update_mode {
> > +     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.
> > + * If both query and update not NULL, the function atomically
> > + * queries and updates indirect action. Query and update are carried in
> order
> > + * specified in the mode parameter.
> > + *
> > + * @param port_id
> > + *   Port identifier of Ethernet device.
> > + * @param handle
> > + *   Handle for the indirect action object to be updated.
> > + * @param update
> > + *   If not NULL, update profile specification used to modify the action
> > + *   pointed by handle.
> > + * @param query
> > + *   If not NULL pointer to storage for the associated query data type.
> > + * @param mode
> > + *   Operational mode.
> > + *   Required if both *update* and *query* are not NULL.
> 
> It should be a part of generic checks in
> rte_flow_action_handle_query_update() implementation.
> 

Will fix in v9.

> > + * @param 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.
> > + * - (-ENODEV) if *port_id* invalid.
> > + * - (-ENOTSUP) if underlying device does not support this functionality.
> > + * - (-EINVAL) if *handle* invalid or both *query* and *update* are
> NULL.
> > + */
> > +__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
> > + *
> > + * @param port_id
> > + *   Port identifier of Ethernet device.
> > + * @param queue_id
> > + *   Flow queue which is used to update the rule.
> > + * @param attr
> > + *   Indirect action update operation attributes.
> > + * @param handle
> > + *   Handle for the indirect action object to be updated.
> > + * @param update
> > + *   If not NULL, update profile specification used to modify the action
> > + *   pointed by handle.
> > + * @param query
> > + *   If not NULL, pointer to storage for the associated query data type.
> > + *   Query result returned on async completion event.
> > + * @param mode
> > + *   Operational mode.
> > + *   Required if both *update* and *query* are not NULL.
> 
> It should be a part of generic checks in
> rte_flow_action_handle_query_update() implementation.
>

Will fix in v9.
 
> > + * @param user_data
> > + *   The user data that will be returned on async completion event.
> > + * @param 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.
> > + * - (-ENODEV) if *port_id* invalid.
> > + * - (-ENOTSUP) if underlying device does not support this functionality.
> > + * - (-EINVAL) if *handle* invalid or both *update* and *query* are
> NULL.
> > + */
> > +__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
> 
> [snip]
>
  
Andrew Rybchenko Feb. 2, 2023, 10:30 a.m. UTC | #4
On 2/2/23 13:24, Gregory Etelson wrote:
>> On 2/1/23 18:16, Gregory Etelson wrote:
>>> +     ops = rte_flow_ops_get(port_id, error);
>>> +     if (!ops || !ops->action_handle_query_update)
>>> +             return -ENOTSUP;
>>
>> May be it makes sense to use single-operation callbacks if
>> another operation input is NULL. I guess it could be
>> convenient in some cases. Just an idea.
>>
> 
> There is a plan to deprecate indirect query and update functions
> and replace them with a single query_update.

So, will you add corresponding code? Just want to clarify.

>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Query and update operational mode.
>>> + *
>>> + * RTE_FLOW_QU_QUERY_FIRST
>>> + *   Force port to query action before update.
>>> + * RTE_FLOW_QU_UPDATE_FIRST
>>> + *   Force port to update action before query.
>>
>> I'm sorry, but I strongly dislike enum members duplication
>> here. I don't understand why we need to duplicate it and why
>> we can't document it in a right way below.
>>
> 
> I don't understand where the duplication is.
> Query and update operations are atomic for application,
> but in hardware these are 2 separate operations that reference hardware object.
> The operations execution order can have different results on object state.
> When application asks both actions it must explicitly specify execution order for hardware.

No-no, simpler. Just remove RTE_FLOW_QU_QUERY_FIRST and
RTE_FLOW_QU_UPDATE_FIRST above and update comments below
to use comments from above. That's it.

>>> + *
>>> + * @see rte_flow_action_handle_query_update()
>>> + * @see rte_flow_async_action_handle_query_update()
>>> + */
>>> +enum rte_flow_query_update_mode {
>>> +     RTE_FLOW_QU_QUERY_FIRST,  /**< Query before update. */
>>> +     RTE_FLOW_QU_UPDATE_FIRST, /**< Query after  update. */
>>> +};
  
Gregory Etelson Feb. 2, 2023, 10:44 a.m. UTC | #5
Hello Andrew,

[]

> On 2/2/23 13:24, Gregory Etelson wrote:
> >> On 2/1/23 18:16, Gregory Etelson wrote:
> >>> +     ops = rte_flow_ops_get(port_id, error);
> >>> +     if (!ops || !ops->action_handle_query_update)
> >>> +             return -ENOTSUP;
> >>
> >> May be it makes sense to use single-operation callbacks if
> >> another operation input is NULL. I guess it could be
> >> convenient in some cases. Just an idea.
> >>
> >
> > There is a plan to deprecate indirect query and update functions
> > and replace them with a single query_update.
> 
> So, will you add corresponding code? Just want to clarify.
>

There is ongoing work on next stage of indirect action functions.
The plan is to deprecate existing API when all updates will be introduced. 
 
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Query and update operational mode.
> >>> + *
> >>> + * RTE_FLOW_QU_QUERY_FIRST
> >>> + *   Force port to query action before update.
> >>> + * RTE_FLOW_QU_UPDATE_FIRST
> >>> + *   Force port to update action before query.
> >>
> >> I'm sorry, but I strongly dislike enum members duplication
> >> here. I don't understand why we need to duplicate it and why
> >> we can't document it in a right way below.
> >>
> >
> > I don't understand where the duplication is.
> > Query and update operations are atomic for application,
> > but in hardware these are 2 separate operations that reference hardware
> object.
> > The operations execution order can have different results on object state.
> > When application asks both actions it must explicitly specify execution
> order for hardware.
> 
> No-no, simpler. Just remove RTE_FLOW_QU_QUERY_FIRST and
> RTE_FLOW_QU_UPDATE_FIRST above and update comments below
> to use comments from above. That's it.
> 

I see. Thank you !
Will fix in v9

> >>> + *
> >>> + * @see rte_flow_action_handle_query_update()
> >>> + * @see rte_flow_async_action_handle_query_update()
> >>> + */
> >>> +enum rte_flow_query_update_mode {
> >>> +     RTE_FLOW_QU_QUERY_FIRST,  /**< Query before update. */
> >>> +     RTE_FLOW_QU_UPDATE_FIRST, /**< Query after  update. */
> >>> +};
  
Andrew Rybchenko Feb. 2, 2023, 10:47 a.m. UTC | #6
On 2/2/23 13:44, Gregory Etelson wrote:
> Hello Andrew,
> 
> []
> 
>> On 2/2/23 13:24, Gregory Etelson wrote:
>>>> On 2/1/23 18:16, Gregory Etelson wrote:
>>>>> +     ops = rte_flow_ops_get(port_id, error);
>>>>> +     if (!ops || !ops->action_handle_query_update)
>>>>> +             return -ENOTSUP;
>>>>
>>>> May be it makes sense to use single-operation callbacks if
>>>> another operation input is NULL. I guess it could be
>>>> convenient in some cases. Just an idea.
>>>>
>>>
>>> There is a plan to deprecate indirect query and update functions
>>> and replace them with a single query_update.
>>
>> So, will you add corresponding code? Just want to clarify.
>>
> 
> There is ongoing work on next stage of indirect action functions.
> The plan is to deprecate existing API when all updates will be introduced.

IMHO nothing prevents to add handling right now. It will allow
to use query_update instead of just query or update from the
very beginning. Not a strong requirement. Just nice to have
from my point of view.
  

Patch

diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst
index c15f6fbb9f..8115f840fe 100644
--- a/doc/guides/rel_notes/release_23_03.rst
+++ b/doc/guides/rel_notes/release_23_03.rst
@@ -69,6 +69,14 @@  New Features
     ``rte_event_dev_config::nb_single_link_event_port_queues`` parameter
     required for eth_rx, eth_tx, crypto and timer eventdev adapters.
 
+* **Added functions to atomically query and update indirect flow action.**
+
+  Added synchronous and asynchronous functions to atomically query and update
+  indirect flow action:
+
+  - ``rte_flow_action_handle_query_update``
+  - ``rte_flow_async_action_handle_query_update``
+
 
 Removed Items
 -------------
diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
index 7d0c24366c..ece40e7877 100644
--- a/lib/ethdev/rte_flow.c
+++ b/lib/ethdev/rte_flow.c
@@ -1883,3 +1883,48 @@  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)
+{
+	int ret;
+	struct rte_eth_dev *dev;
+	const struct rte_flow_ops *ops;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+	ops = rte_flow_ops_get(port_id, error);
+	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)
+{
+	int ret;
+	struct rte_eth_dev *dev;
+	const struct rte_flow_ops *ops;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+	ops = rte_flow_ops_get(port_id, error);
+	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..d3fbba5b99 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -5622,6 +5622,109 @@  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 and update operational mode.
+ *
+ * RTE_FLOW_QU_QUERY_FIRST
+ *   Force port to query action before update.
+ * RTE_FLOW_QU_UPDATE_FIRST
+ *   Force port to update action before query.
+ *
+ * @see rte_flow_action_handle_query_update()
+ * @see rte_flow_async_action_handle_query_update()
+ */
+enum rte_flow_query_update_mode {
+	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.
+ * If both query and update not NULL, the function atomically
+ * queries and updates indirect action. Query and update are carried in order
+ * specified in the mode parameter.
+ *
+ * @param port_id
+ *   Port identifier of Ethernet device.
+ * @param handle
+ *   Handle for the indirect action object to be updated.
+ * @param update
+ *   If not NULL, update profile specification used to modify the action
+ *   pointed by handle.
+ * @param query
+ *   If not NULL pointer to storage for the associated query data type.
+ * @param mode
+ *   Operational mode.
+ *   Required if both *update* and *query* are not NULL.
+ * @param 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.
+ * - (-ENODEV) if *port_id* invalid.
+ * - (-ENOTSUP) if underlying device does not support this functionality.
+ * - (-EINVAL) if *handle* invalid or both *query* and *update* are NULL.
+ */
+__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
+ *
+ * @param port_id
+ *   Port identifier of Ethernet device.
+ * @param queue_id
+ *   Flow queue which is used to update the rule.
+ * @param attr
+ *   Indirect action update operation attributes.
+ * @param handle
+ *   Handle for the indirect action object to be updated.
+ * @param update
+ *   If not NULL, update profile specification used to modify the action
+ *   pointed by handle.
+ * @param query
+ *   If not NULL, pointer to storage for the associated query data type.
+ *   Query result returned on async completion event.
+ * @param mode
+ *   Operational mode.
+ *   Required if both *update* and *query* are not NULL.
+ * @param user_data
+ *   The user data that will be returned on async completion event.
+ * @param 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.
+ * - (-ENODEV) if *port_id* invalid.
+ * - (-ENOTSUP) if underlying device does not support this functionality.
+ * - (-EINVAL) if *handle* invalid or both *update* and *query* are NULL.
+ */
+__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..628c486081 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;
+
+	# added in 23.03
+	rte_flow_action_handle_query_update;
+	rte_flow_async_action_handle_query_update;
+
 };
 
 INTERNAL {