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

Message ID 20230119164713.7164-1-getelson@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v6,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 Jan. 19, 2023, 4:47 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.

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.

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.	
---
 doc/guides/rel_notes/release_23_03.rst |  7 ++
 lib/ethdev/rte_flow.c                  | 39 ++++++++++
 lib/ethdev/rte_flow.h                  | 99 ++++++++++++++++++++++++++
 lib/ethdev/rte_flow_driver.h           | 15 ++++
 lib/ethdev/version.map                 |  5 ++
 5 files changed, 165 insertions(+)
  

Comments

Andrew Rybchenko Jan. 20, 2023, 8:35 a.m. UTC | #1
On 1/19/23 19:47, 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.
> 
> The patch adds `rte_flow_action_handle_query_update` function call,

"This patch adds ..." -> "Add ..."

> and it's async version `rte_flow_async_action_handle_query_update`
> to atomically query and update flow action.

Sorry, may be I'm missing something, but after reading previous
discussion I have a feeling that update could be queried data
dependent. However, I don't understand how it could be possible
with API below. Just my misunderstanding?

> 
> 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.	
> ---
>   doc/guides/rel_notes/release_23_03.rst |  7 ++
>   lib/ethdev/rte_flow.c                  | 39 ++++++++++
>   lib/ethdev/rte_flow.h                  | 99 ++++++++++++++++++++++++++
>   lib/ethdev/rte_flow_driver.h           | 15 ++++
>   lib/ethdev/version.map                 |  5 ++
>   5 files changed, 165 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst
> index c15f6fbb9f..6941d20abc 100644
> --- a/doc/guides/rel_notes/release_23_03.rst
> +++ b/doc/guides/rel_notes/release_23_03.rst
> @@ -69,6 +69,13 @@ 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``

Please, add one more empty line since two should separate
sections.

>   
>   Removed Items
>   -------------
> 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];

I know that nearby code does the same, but I still dislike it.
If port_id is invalid, it could be out-of-boundary access.
Yes, port_id is checked on ops get, but it would be safer to
do the assignment after ops check below.
Right now there is no bugs, but the future modifications
could break it.

> +	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];

same here

> +	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..280567a3ae 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -5622,6 +5622,105 @@ 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.

May be "Query and update operation 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 update parameter is NULL the function queries indirect action.
> + * If query parameter is NULL the function updates indirect action.

IMHO it is better to highlight in the parameter description
below that parameter may be NULL. See below.

> + * 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.

I'd like to double-check what happens if both ops are NULL.
I guess it is just NOP.
Also it should not be a problem is QUERY_FIRST is specified,
but query is NULL. Same for update.
If yes in both cases, no extra documentation is required.
If it is an error, it should be documented.

> + *
> + * @param port_id
> + *   Port identifier of Ethernet device.
> + * @param handle
> + *   Handle for the indirect action object to be updated.
> + * @param update
> + *   Update profile specification used to modify the action pointed by handle.

If not NULL, update ...

> + * @param query
> + *   Pointer to storage for the associated query data type.

If not NULL, query action data and store in provided location.

> + * @param mode
> + *   Operational mode.
> + * @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.
> + * - (ENOTSUP) if underlying device does not support this functionality.
> + * - (EINVAL) - if *handle* invalid
> + */
> +__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
> + *   Update profile specification used to modify the action pointed by handle.

It should be documented that NULL is allowed

> + * @param query
> + *   Pointer to storage for the associated query data type.
> + *   Query result returned on async completion event.

It should be documented that NULL is allowed

> + * @param mode
> + *   Operational mode.
> + * @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.
> + * - (ENOTSUP) if underlying device does not support this functionality.
> + * - (EINVAL) - if *handle* invalid
> + */
> +__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

[skip]
  
Gregory Etelson Jan. 20, 2023, 10:46 a.m. UTC | #2
Hello Andrew,

[]

> > The patch adds `rte_flow_action_handle_query_update` function call,
> 
> "This patch adds ..." -> "Add ..."
> 

Will fix in v7.

> > and it's async version `rte_flow_async_action_handle_query_update`
> > to atomically query and update flow action.
> 
> Sorry, may be I'm missing something, but after reading previous
> discussion I have a feeling that update could be queried data
> dependent. However, I don't understand how it could be possible
> with API below. Just my misunderstanding?
> 

The goal of `rte_flow_action_handle_query_update()` is to execute query and update actions atomically.
The function guaranties that action state will not change by any event before both update and query complete.
If the function was called with the ` RTE_FLOW_QU_QUERY_FIRST ` `mode` argument, then update execution can depend
on query result. That's up to query format, PMD implementation and hardware capabilities.
I hope that answered your question.
  
> >
> > 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.
> > ---
> >   doc/guides/rel_notes/release_23_03.rst |  7 ++
> >   lib/ethdev/rte_flow.c                  | 39 ++++++++++
> >   lib/ethdev/rte_flow.h                  | 99 ++++++++++++++++++++++++++
> >   lib/ethdev/rte_flow_driver.h           | 15 ++++
> >   lib/ethdev/version.map                 |  5 ++
> >   5 files changed, 165 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/release_23_03.rst
> b/doc/guides/rel_notes/release_23_03.rst
> > index c15f6fbb9f..6941d20abc 100644
> > --- a/doc/guides/rel_notes/release_23_03.rst
> > +++ b/doc/guides/rel_notes/release_23_03.rst
> > @@ -69,6 +69,13 @@ 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``
> 
> Please, add one more empty line since two should separate
> sections.
> 

Will fix in v7.

> >
> >   Removed Items
> >   -------------
> > 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];
> 
> I know that nearby code does the same, but I still dislike it.
> If port_id is invalid, it could be out-of-boundary access.
> Yes, port_id is checked on ops get, but it would be safer to
> do the assignment after ops check below.
> Right now there is no bugs, but the future modifications
> could break it.
> 

Will fix in v7.

> > +     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];
> 
> same here
> 

Will fix in v7.

> > +     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..280567a3ae 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -5622,6 +5622,105 @@
> 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.
> 
> May be "Query and update operation mode."?
> 

Looks good. Will fix in v7.

> > + *
> > + * 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 update parameter is NULL the function queries indirect action.
> > + * If query parameter is NULL the function updates indirect action.
> 
> IMHO it is better to highlight in the parameter description
> below that parameter may be NULL. See below.
> 

Will fix in v7.

> > + * 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.
> 
> I'd like to double-check what happens if both ops are NULL.
> I guess it is just NOP.

If both query and update parameters were NULL, the function will fail with EINVAL error
Will fix in v7.

> Also it should not be a problem is QUERY_FIRST is specified,
> but query is NULL. Same for update.
> If yes in both cases, no extra documentation is required.
> If it is an error, it should be documented.
> 

Both RTE_FLOW_QU_QUERY_FIRST and RTE_FLOW_QU_UPDATE_FIRST require
query and update parameters.
Will fix in v7.

> > + *
> > + * @param port_id
> > + *   Port identifier of Ethernet device.
> > + * @param handle
> > + *   Handle for the indirect action object to be updated.
> > + * @param update
> > + *   Update profile specification used to modify the action pointed by
> handle.
> 
> If not NULL, update ...
> 

Will fix in v7.

> > + * @param query
> > + *   Pointer to storage for the associated query data type.
> 
> If not NULL, query action data and store in provided location.
> 

Will fix in v7.

> > + * @param mode
> > + *   Operational mode.
> > + * @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.
> > + * - (ENOTSUP) if underlying device does not support this functionality.
> > + * - (EINVAL) - if *handle* invalid
> > + */
> > +__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
> > + *   Update profile specification used to modify the action pointed by
> handle.
> 
> It should be documented that NULL is allowed
> 

Will fix in v7.

> > + * @param query
> > + *   Pointer to storage for the associated query data type.
> > + *   Query result returned on async completion event.
> 
> It should be documented that NULL is allowed
> 

Will fix in v7.

> > + * @param mode
> > + *   Operational mode.
> > + * @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.
> > + * - (ENOTSUP) if underlying device does not support this functionality.
> > + * - (EINVAL) - if *handle* invalid
> > + */
> > +__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
> 
> [skip]
  
Andrew Rybchenko Jan. 20, 2023, 11:22 a.m. UTC | #3
On 1/20/23 13:46, Gregory Etelson wrote:
>>> and it's async version `rte_flow_async_action_handle_query_update`
>>> to atomically query and update flow action.
>>
>> Sorry, may be I'm missing something, but after reading previous
>> discussion I have a feeling that update could be queried data
>> dependent. However, I don't understand how it could be possible
>> with API below. Just my misunderstanding?
>>
> 
> The goal of `rte_flow_action_handle_query_update()` is to execute query and update actions atomically.
> The function guaranties that action state will not change by any event before both update and query complete.
> If the function was called with the ` RTE_FLOW_QU_QUERY_FIRST ` `mode` argument, then update execution can depend
> on query result. That's up to query format, PMD implementation and hardware capabilities.
> I hope that answered your question.

Sorry, I'm still confused. Could you explain a bit more,
please. How update could depend on query result?
Caller already specified update structure...
  
Gregory Etelson Jan. 20, 2023, 4:50 p.m. UTC | #4
Hello Andrew,

[]

> On 1/20/23 13:46, Gregory Etelson wrote:
> >>> and it's async version `rte_flow_async_action_handle_query_update`
> >>> to atomically query and update flow action.
> >>
> >> Sorry, may be I'm missing something, but after reading previous
> >> discussion I have a feeling that update could be queried data
> >> dependent. However, I don't understand how it could be possible
> >> with API below. Just my misunderstanding?
> >>
> >
> > The goal of `rte_flow_action_handle_query_update()` is to execute query
> and update actions atomically.
> > The function guaranties that action state will not change by any event
> before both update and query complete.
> > If the function was called with the ` RTE_FLOW_QU_QUERY_FIRST `
> `mode` argument, then update execution can depend
> > on query result. That's up to query format, PMD implementation and
> hardware capabilities.
> > I hope that answered your question.
> 
> Sorry, I'm still confused. Could you explain a bit more,
> please. How update could depend on query result?

Conditional update I described requires special action properties.
Consider an action object with a method that receives query and update as parameters.
The method will activate update only if query result satisfies action state.
If the action was updated, both query and update were atomic for application.
The function returns the action state - updated or not.
Application is aware about the action properties.
Application applies action properties to returned state to discover if action was updated.

> Caller already specified update structure...
 
Regards,
Gregory
  
Andrew Rybchenko Feb. 1, 2023, 11 a.m. UTC | #5
On 1/20/23 19:50, Gregory Etelson wrote:
> Hello Andrew,
> 
> []
> 
>> On 1/20/23 13:46, Gregory Etelson wrote:
>>>>> and it's async version `rte_flow_async_action_handle_query_update`
>>>>> to atomically query and update flow action.
>>>>
>>>> Sorry, may be I'm missing something, but after reading previous
>>>> discussion I have a feeling that update could be queried data
>>>> dependent. However, I don't understand how it could be possible
>>>> with API below. Just my misunderstanding?
>>>>
>>>
>>> The goal of `rte_flow_action_handle_query_update()` is to execute query
>> and update actions atomically.
>>> The function guaranties that action state will not change by any event
>> before both update and query complete.
>>> If the function was called with the ` RTE_FLOW_QU_QUERY_FIRST `
>> `mode` argument, then update execution can depend
>>> on query result. That's up to query format, PMD implementation and
>> hardware capabilities.
>>> I hope that answered your question.
>>
>> Sorry, I'm still confused. Could you explain a bit more,
>> please. How update could depend on query result?
> 
> Conditional update I described requires special action properties.
> Consider an action object with a method that receives query and update as parameters.
> The method will activate update only if query result satisfies action state.
> If the action was updated, both query and update were atomic for application.
> The function returns the action state - updated or not.
> Application is aware about the action properties.
> Application applies action properties to returned state to discover if action was updated.

Is it just a theoretical possibility or do you have an example
right now?

> 
>> Caller already specified update structure...
>   
> Regards,
> Gregory
  
Gregory Etelson Feb. 1, 2023, 2:03 p.m. UTC | #6
Hello Andrew,

> >
> >> On 1/20/23 13:46, Gregory Etelson wrote:
> >>>>> and it's async version `rte_flow_async_action_handle_query_update`
> >>>>> to atomically query and update flow action.
> >>>>
> >>>> Sorry, may be I'm missing something, but after reading previous
> >>>> discussion I have a feeling that update could be queried data
> >>>> dependent. However, I don't understand how it could be possible
> >>>> with API below. Just my misunderstanding?
> >>>>
> >>>
> >>> The goal of `rte_flow_action_handle_query_update()` is to execute
> query
> >> and update actions atomically.
> >>> The function guaranties that action state will not change by any event
> >> before both update and query complete.
> >>> If the function was called with the ` RTE_FLOW_QU_QUERY_FIRST `
> >> `mode` argument, then update execution can depend
> >>> on query result. That's up to query format, PMD implementation and
> >> hardware capabilities.
> >>> I hope that answered your question.
> >>
> >> Sorry, I'm still confused. Could you explain a bit more,
> >> please. How update could depend on query result?
> >
> > Conditional update I described requires special action properties.
> > Consider an action object with a method that receives query and update
> as parameters.
> > The method will activate update only if query result satisfies action state.
> > If the action was updated, both query and update were atomic for
> application.
> > The function returns the action state - updated or not.
> > Application is aware about the action properties.
> > Application applies action properties to returned state to discover if
> action was updated.
> 
> Is it just a theoretical possibility or do you have an example
> right now?
> 

Quota flow action is updated according to these rules.
You validate it with MLX5 hardware and patches from this batch.

Regards,
Gregory
  

Patch

diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst
index c15f6fbb9f..6941d20abc 100644
--- a/doc/guides/rel_notes/release_23_03.rst
+++ b/doc/guides/rel_notes/release_23_03.rst
@@ -69,6 +69,13 @@  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..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..280567a3ae 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -5622,6 +5622,105 @@  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_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 update parameter is NULL the function queries indirect action.
+ * If query parameter is NULL the function updates indirect 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
+ *   Update profile specification used to modify the action pointed by handle.
+ * @param query
+ *   Pointer to storage for the associated query data type.
+ * @param mode
+ *   Operational mode.
+ * @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.
+ * - (ENOTSUP) if underlying device does not support this functionality.
+ * - (EINVAL) - if *handle* invalid
+ */
+__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
+ *   Update profile specification used to modify the action pointed by handle.
+ * @param query
+ *   Pointer to storage for the associated query data type.
+ *   Query result returned on async completion event.
+ * @param mode
+ *   Operational mode.
+ * @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.
+ * - (ENOTSUP) if underlying device does not support this functionality.
+ * - (EINVAL) - if *handle* invalid
+ */
+__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 {