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

[Thomas Monjalon <thomas@monjalon.net>]

18/01/2023 18:34, Gregory Etelson:
> > > 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.
> > 
> > What is the benefit?
> > Is it a performance optimization? How much?
> >
> 
> rte_flow_action_handle_query_update() can query data and conditionally update it in a single HW operation.
> That provides accuracy that cannot be achieved with existing API.
> For example, quota flow action must be updated when it in the PASS state only. 
> If application use existing query, by the time it gets query data and analyzes it, HW object state can change.
> As the result application update action will not reflect HW configuration.

An explanation of the atomicity benefit should be in the commit log.

> The function can provide general optimization, but that was not tested.


> > > 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.
> > 
> > Why do we need such compatibility?
> > The existing functions will stay, isn't it?
> > 
> 
> rte_flow_action_handle_query_update() has extended functionality with better HW support.

You cannot argue HW support without knowing what would be the support
of other vendors.

> The plan is to deprecate and replace existing query and update functions.

I disagree with such deprecation.
Anyway it is not the topic of this patch, so I suggest to remove the sentence
about compatibility.