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

[Andrey Vesnovaty <andreyv@nvidia.com>]

Hi Andrew

All your suggestions applied in v8 series.

Thanks,
Andrey

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Wednesday, October 14, 2020 9:50 AM
> To: Andrey Vesnovaty <andreyv@nvidia.com>; dev@dpdk.org
> Cc: jer@marvell.com; jerinjacobk@gmail.com; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; 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; mdr@ashroe.eu; nhorman@tuxdriver.com;
> ajit.khaparde@broadcom.com; samik.gupta@broadcom.com
> Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: add flow shared action API
> 
> Hi Andrey,
> 
> On 10/13/20 11:06 PM, Andrey Vesnovaty wrote:
> > Hi Andrew.
> >
> > Thanks for the input.
> > All spelling & rephrases will be applied  PSB for the rest.
> > Will publish v8 very soon.
> >
> > Thanks,
> > Andrey
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>
> >> Sent: Monday, October 12, 2020 5:19 PM
> >> To: Andrey Vesnovaty <andreyv@nvidia.com>; dev@dpdk.org
> >> Cc: jer@marvell.com; jerinjacobk@gmail.com; NBU-Contact-Thomas
> Monjalon
> >> <thomas@monjalon.net>; 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; mdr@ashroe.eu; nhorman@tuxdriver.com;
> >> ajit.khaparde@broadcom.com; samik.gupta@broadcom.com
> >> Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: add flow shared action API
> >>
> >> "add flow shared action API"
> >>
> >> Is flow shared? May be "add shared actions to flow API".
> >
> > Accepted, will update commit message.
> >>
> >> On 10/8/20 2:51 PM, Andrey Vesnovaty wrote:
> >>> This commit introduces extension of DPDK flow action API enabling
> >>
> >> "This commit" and "DPDK" are not necessary in description.
> >> It is a commit description and the patch is to DPDK tree.
> >> Consider just:
> >> "Introduce extension of flow action API enabling..."
> >
> > Accepted, will update commit message.
> >>
> >>> sharing of single rte_flow_action in multiple flows. The API intended for
> >>> PMDs, where multiple HW offloaded flows can reuse the same HW
> >>> essence/object representing flow action and modification of such an
> >>> essence/object affects all the rules using it.
> >>>
> >>> Motivation and example
> >>> ===
> >>> Adding or removing one or more queues to RSS used by multiple flow rules
> >>> imposes per rule toll for current DPDK flow API; the scenario requires
> >>> for each flow sharing cloned RSS action:
> >>> - call `rte_flow_destroy()`
> >>> - call `rte_flow_create()` with modified RSS action
> >>>
> >>> API for sharing action and its in-place update benefits:
> >>> - reduce the overhead of multiple RSS flow rules reconfiguration
> >>> - optimize resource utilization by sharing action across multiple
> >>>   flows
> >>>
> >>> Change description
> >>> ===
> >>>
> >>> Shared action
> >>> ===
> >>> In order to represent flow action shared by multiple flows new action
> >>> type RTE_FLOW_ACTION_TYPE_SHARED is introduced (see `enum
> >>> rte_flow_action_type`).
> >>> Actually the introduced API decouples action from any specific flow and
> >>> enables sharing of single action by its handle across multiple flows.
> >>>
> >>> 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).
> >>>
> >>> 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.
> 
> [snip]
> 
> >>> diff --git a/lib/librte_ethdev/rte_ethdev_version.map
> >> b/lib/librte_ethdev/rte_ethdev_version.map
> >>> index c95ef5157a..a8a4821dbb 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev_version.map
> >>> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> >>> @@ -229,6 +229,10 @@ EXPERIMENTAL {
> >>>  	# added in 20.11
> >>>  	rte_eth_link_speed_to_str;
> >>>  	rte_eth_link_to_str;
> >>> +	rte_flow_shared_action_create;
> >>> +	rte_flow_shared_action_destroy;
> >>> +	rte_flow_shared_action_update;
> >>> +	rte_flow_shared_action_query;
> >>
> >> Shouldn't it be alphabetically sorted?
> >
> > Query before update?
> 
> yes
> 
> [snip]
> 
> >>> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> >>> index da8bfa5489..9050adec23 100644
> >>> --- a/lib/librte_ethdev/rte_flow.h
> >>> +++ b/lib/librte_ethdev/rte_flow.h
> 
> [snip]
> 
> >>> @@ -3357,6 +3380,150 @@ int
> >>>  rte_flow_get_aged_flows(uint16_t port_id, void **contexts,
> >>>  			uint32_t nb_contexts, struct rte_flow_error *error);
> >>>
> >>> +/**
> >>> + * Specify shared action configuration
> >>> + */
> >>> +struct rte_flow_shared_action_conf {
> >>> +	/**
> >>> +	 * Flow direction for shared action configuration.
> >>> +	 *
> >>> +	 * Shred action should be valid at least for one flow direction,
> >>> +	 * otherwise it is invalid for both ingress and egress rules.
> >>> +	 */
> >>> +	uint32_t ingress:1;
> >>> +	/**< Action valid for rules applied to ingress traffic. */
> >>> +	uint32_t egress:1;
> >>> +	/**< Action valid for rules applied to egress traffic. */
> >>> +};
> >>> +
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Create shared action for reuse in multiple flow rules.
> >>> + * The created shared action has single state and configuration
> >>> + * across all flow rules using it.
> >>> + *
> >>> + * @param[in] port_id
> >>> + *    The port identifier of the Ethernet device.
> >>> + * @param[in] conf
> >>> + *   Shared action configuration.
> >>> + * @param[in] action
> >>> + *   Action configuration for shared action creation.
> >>> + * @param[out] error
> >>> + *   Perform verbose error reporting if not NULL. PMDs initialize this
> >>> + *   structure in case of error only.
> >>> + * @return
> >>> + *   A valid handle in case of success, NULL otherwise and rte_errno is set
> >>> + *   to one of the error codes defined:
> >>> + *   - (ENOSYS) if underlying device does not support this functionality.
> >>> + *   - (EIO) if underlying device is removed.
> >>> + *   - (EINVAL) if *action* invalid.
> >>> + *   - (ENOTSUP) if *action* valid but unsupported.
> >>
> >> ENODEV ?
> >
> > Can you elaborate?
> 
> 
> ENODEV is returned if port_id is invalid and it should be
> documented here if you try to list all return values.