Re: [dpdk-dev] [PATCH v7 1/2] ethdev: add flow shared action API

[Thomas Monjalon <thomas@monjalon.net>]

14/10/2020 08:49, Andrew Rybchenko:
> On 10/13/20 11:06 PM, Andrey Vesnovaty wrote:
> > From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> >>> Shared action create/use/destroy
> >>> ===
> >>> Shared action may be reused by some or none flow rules at any given
> >>> moment, i.e. shared action resides outside of the context of any flow.
> >>> Shared action represent HW resources/objects used for action offloading
> >>> implementation.
> >>> API for shared action create (see `rte_flow_shared_action_create()`):
> >>> - should allocate HW resources and make related initializations required
> >>>   for shared action implementation.
> >>> - make necessary preparations to maintain shared access to
> >>>   the action resources, configuration and state.
> >>> API for shared action destroy (see `rte_flow_shared_action_destroy()`)
> >>> should release HW resources and make related cleanups required for shared
> >>> action implementation.
> >>>
> >>> In order to share some flow action reuse the handle of type
> >>> `struct rte_flow_shared_action` returned by
> >>> rte_flow_shared_action_create() as a `conf` field of
> >>> `struct rte_flow_action` (see "example" section).
> >>>
> >>> If some shared action not used by any flow rule all resources allocated
> >>> by the shared action can be released by rte_flow_shared_action_destroy()
> >>> (see "example" section). The shared action handle passed as argument to
> >>> destroy API should not be used any further i.e. result of the usage is
> >>> undefined.
> >>>
> >>> Shared action re-configuration
> >>> ===
> >>> Shared action behavior defined by its configuration can be updated via
> >>> rte_flow_shared_action_update() (see "example" section). The shared
> >>> action update operation modifies HW related resources/objects allocated
> >>> on the action creation. The number of operations performed by the update
> >>> operation should not depend on the number of flows sharing the related
> >>> action. On return of shared action update API action behavior should be
> >>> according to updated configuration for all flows sharing the action.
> >>>
> >>> Shared action query
> >>> ===
> >>> Provide separate API to query shared action state (see
> >>> rte_flow_shared_action_update()). Taking a counter as an example: query
> >>> returns value aggregating all counter increments across all flow rules
> >>> sharing the counter. This API doesn't query shared action configuration
> >>> since it is controlled by rte_flow_shared_action_create() and
> >>> rte_flow_shared_action_update() APIs and no supposed to change by other
> >>> means.
> >>>
> >>> PMD support
> >>> ===
> >>> The support of introduced API is pure PMD specific design and
> >>> responsibility for each action type (see struct rte_flow_ops).

This sentence is strange.
Of course PMD implementation is a PMD specific design.
I think you can remove it.

> >>> testpmd
> >>> ===
> >>> In order to utilize introduced API testpmd cli may implement following
> >>> extension
> >>> create/update/destroy/query shared action accordingly
> >>>
> >>> flow shared_action (port) create {action_id (id)} (action) / end
> >>> flow shared_action (port) update (id) (action) / end
> >>> flow shared_action (port) destroy action_id (id) {action_id (id) [...]}
> >>> flow shared_action (port) query (id)
> >>>
> >>> testpmd example
> >>> ===
> >>>
> >>> configure rss to queues 1 & 2
> >>>
> >>>> flow shared_action 0 create action_id 100 rss queues 1 2 end / end
> >>>
> >>> create flow rule utilizing shared action
> >>>
> >>>> flow create 0 ingress \
> >>>     pattern eth dst is 0c:42:a1:15:fd:ac / ipv6 / tcp / end \
> >>>   actions shared 100 / end
> >>>
> >>> add 2 more queues
> >>>
> >>>> flow shared_action 0 modify 100 rss queues 1 2 3 4 end / end
> >>
> >> testpmd is out-of-scope of the patch and it is better to
> >> remove the description to avoid unsync if testpmd
> >> commands are changed.
> > 
> > There is still added value is testpmd example demonstrating usage of
> > shared action with rte flows. I will update the example to reflect the current
> > testpmd syntax for shared action.
> 
> Yes and no. IMHO It would be OK for series description etc,
> but not OK for the changeset description which will be
> kept in the history and will become misleading when
> commands are changed. I think that no information is better
> than potentially wrong information.

I agree with Andrew, the API explanation should be enough.
Talking about testpmd commands in the ethdev patch is not appropriate.