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

[Andrey Vesnovaty <andreyv@nvidia.com>]

Hi Thomas

All your suggestions applied in v8 series.

Thanks,
Andrey

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, October 14, 2020 10:23 AM
> To: Andrey Vesnovaty <andreyv@nvidia.com>
> Cc: dev@dpdk.org; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>;
> jerinj@marvell.com; ferruh.yigit@intel.com; stephen@networkplumber.org;
> bruce.richardson@intel.com; Ori Kam <orika@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; andrey.vesnovaty@gmail.com;
> ajit.khaparde@broadcom.com; samik.gupta@broadcom.com
> Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: add flow shared action API
> 
> 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.
> 
>