RE: [RFC PATCH v3] ethdev: add indirect list flow action

[Gregory Etelson <getelson@nvidia.com>]

Hello Ori,

> First I'm assuming this is just RFC, since there is no code implementation.
>

I'll add code implementation in v4.
 
> > -----Original Message-----
> > From: Gregory Etelson <getelson@nvidia.com>
> > Sent: Sunday, May 7, 2023 12:51 PM
> >
> > Indirect API creates a shared flow action with unique action handle.
> > Flow rules can access the shared flow action and resources related to
> > that action through the indirect action handle.
> > In addition, the API allows to update existing shared flow action
> > configuration.  After the update completes, new action configuration
> > is available to all flows that reference that shared action.
> >
> > Indirect actions list expands the indirect action API:
> > • Indirect action list creates a handle for one or several
> >   flow actions, while legacy indirect action handle references
> >   single action only.
> >   Input flow actions arranged in END terminated list.
> > • Flow rule can provide rule specific configuration parameters to
> >   existing shared handle.
> >   Updates of flow rule specific configuration will not change the base
> >   action configuration.
> >   Base action configuration was set during the action creation.
> >
> > Indirect action list handle defines 2 types of resources:
> > • Mutable handle resource can be changed during handle lifespan.
> > • Immutable handle resource value is set during handle creation
> >   and cannot be changed.
> >
> > There are 2 types of mutable indirect handle contexts:
> > • Action mutable context is always shared between all flows
> >   that referenced indirect actions list handle.
> >   Action mutable context can be changed by explicit invocation
> >   of indirect handle update function.
> > • Flow mutable context is private to a flow.
> >   Flow mutable context can be updated by indirect list handle
> >   flow rule configuration.
> >
> > flow 1:
> >  / indirect handle H conf C1 /
> >                    |       |
> >                    |       |
> >                    |       |         flow 2:
> >                    |       |         / indirect handle H conf C2 /
> >                    |       |                           |      |
> >                    |       |                           |      |
> >                    |       |                           |      |
> >             =========================================================
> >             ^      |       |                           |      |
> >             |      |       V                           |      V
> >             |    ~~~~~~~~~~~~~~                      ~~~~~~~~~~~~~~~
> >             |     flow mutable                        flow mutable
> >             |     context 1                           context 2
> >             |    ~~~~~~~~~~~~~~                      ~~~~~~~~~~~~~~~
> >   indirect  |      |                                   |
> >   action    |      |                                   |
> >   context   |      V                                   V
> >             |   -----------------------------------------------------
> >             |                 action mutable context
> >             |   -----------------------------------------------------
> >             v                action immutable context
> >             =========================================================
> >
> > Indirect action types - immutable, action / flow mutable, are mutually
> > exclusive and depend on the action definition.
> > For example:
> > • Indirect METER_MARK policy is immutable action member and profile is
> >   action mutable action member.
> > • Indirect METER_MARK flow action defines init_color as flow mutable
> >   member.
> > • Indirect QUOTA flow action does not define flow mutable members.
> >
> > Template API:
> >
> > Action template format:
> >
> >     template .. indirect_list handle Htmpl conf Ctmpl ..
> >     mask     .. indirect_list handle Hmask conf Cmask ..
> >
> > 1 If Htmpl was masked (Hmask != 0), PMD compiles base action
> >   configuration during action template, table template and flow rule
> >   phases - depending on PMD action implementation.
> >   Otherwise, action is compiled from scratch during flow rule
> >   processing.
> >
> Why not just say if Hmask != NULL this action is fixed for this template,
> else action will be set per each flow?
> 
> Also since this is a list of action it is possible that one action will have null
> while other will have a fixed value.
> This means that the Ctmpl should also be checked per each entery:
> Conf[0] != null, conf[1] != null.
> This will also mean that if application wants to use fixed confs he also
> must supply a valid example action_list, (can be different action_list object
> per flow but must be composed of the same actions.)
> 

Will update indirect_list action template description in v4

> > 2 If Htmpl and Ctmpl were masked (Hmask !=0 and Cmask != 0), table
> >   template processing overwrites base action configuration with Ctmpl
> >   parameters.
> >
> What do you mean based action?
> If the Htmpl and Cmask is non zero it means that the configuration and
> action are based on the one set in the template create and not per flow.
> 
> > Flow rule format:
> >
> >     actions .. indirect_list handle Hflow conf Cflow ..
> >
> > 3 If Htmpl was masked, Hflow can reference a different action of the
> >   same type as Htmpl.
> >
> > 4 If Cflow was specified, it overwrites action configuration.
> >
> > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > ---
> > v3: do not deprecate indirect flow action.
> > ---
> >  lib/ethdev/rte_flow.h        | 266 +++++++++++++++++++++++++++++++++++
> >  lib/ethdev/rte_flow_driver.h |  41 ++++++
> >  2 files changed, 307 insertions(+)
> >
> > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > index 713ba8b65c..ac1f51e564 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -2912,6 +2912,13 @@ enum rte_flow_action_type {
> >  	 * applied to the given ethdev Rx queue.
> >  	 */
> >  	RTE_FLOW_ACTION_TYPE_SKIP_CMAN,
> > +
> > +	/**
> > +	 * Action handle to reference flow actions list.
> > +	 *
> > +	 * @see struct rte_flow_action_indirect_list
> > +	 */
> > +	RTE_FLOW_ACTION_TYPE_INDIRECT_LIST,
> >  };
> >
> >  /**
> > @@ -6118,6 +6125,265 @@
> > rte_flow_async_action_handle_query_update(uint16_t port_id, uint32_t
> > queue_id,
> >  					  void *user_data,
> >  					  struct rte_flow_error *error);
> >
> > +struct rte_flow_action_list_handle;
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Configure INDIRECT_LIST flow action.
> > + *
> > + * @see RTE_FLOW_ACTION_TYPE_INDIRECT_LIST
> > + */
> > +struct rte_flow_action_indirect_list {
> > +	struct rte_flow_action_list_handle; /**< Indirect action list handle */
> Since the comments above the member the comment should start with /** only
> 

Will update the comment in v4.

> > +	/**
> > +	 * Flow mutable configuration array.
> > +	 * NULL if the handle has no flow mutable configuration update.
> > +	 * Otherwise, if the handle was created with list A1 / A2 .. An / END
> > +	 * size of conf is n.
> > +	 * conf[i] points to flow mutable update of Ai in the handle
> > +	 * actions list or NULL if Ai has no update.
> > +	 */
> > +	const void **conf;
> > +};
> > +
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Create an indirect flow action object from flow actions list.
> > + * The object is identified by a unique handle.
> > + * The handle has single state and configuration
> > + * across all the flow rules using it.
> > + *
> > + * @param[in] port_id
> > + *    The port identifier of the Ethernet device.
> > + * @param[in] conf
> > + *   Action configuration for the indirect action list creation.
> > + * @param[in] actions
> > + *   Specific configuration of the indirect action lists.
> > + * @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:
> > + *   - (-ENODEV) if *port_id* invalid.
> > + *   - (-ENOSYS) if underlying device does not support this functionality.
> > + *   - (-EIO) if underlying device is removed.
> > + *   - (-EINVAL) if *actions* list invalid.
> > + *   - (-ENOTSUP) if *action* list element valid but unsupported.
> > + *   - (-E2BIG) to many elements in *actions*
> How can you get this error?
> 

Will remove E2BIG in v4.

> > + */
> > +__rte_experimental
> > +struct rte_flow_action_list_handle *
> > +rte_flow_action_list_handle_create(uint16_t port_id,
> > +				   const
> > +				   struct rte_flow_indir_action_conf *conf,
> > +				   const struct rte_flow_action *actions,
> > +				   struct rte_flow_error *error);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Async function call to create an indirect flow action object
> > + * from flow actions list.
> > + * The object is identified by a unique handle.
> > + * The handle has single state and configuration
> > + * across all the flow rules using it.
> > + *
> > + * @param[in] port_id
> > + *    The port identifier of the Ethernet device.
> > + * @param[in] queue_id
> > + *   Flow queue which is used to update the rule.
> > + * @param[in] attr
> > + *   Indirect action update operation attributes.
> > + * @param[in] conf
> > + *   Action configuration for the indirect action list creation.
> > + * @param[in] actions
> > + *   Specific configuration of the indirect action list.
> > + * @param[in] user_data
> > + *   The user data that will be returned on async completion event.
> > + * @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:
> > + *   - (-ENODEV) if *port_id* invalid.
> > + *   - (-ENOSYS) if underlying device does not support this functionality.
> > + *   - (-EIO) if underlying device is removed.
> > + *   - (-EINVAL) if *actions* list invalid.
> > + *   - (-ENOTSUP) if *action* list element valid but unsupported.
> > + *   - (-E2BIG) to many elements in *actions*
> Same as above question
> 

Will remove E2BIG in v4.

> > + */
> > +__rte_experimental
> > +struct rte_flow_action_list_handle *
> > +rte_flow_async_action_list_handle_create(uint16_t port_id, uint32_t
> > queue_id,
> > +					 const struct rte_flow_op_attr *attr,
> > +					 const struct
> > +					 rte_flow_indir_action_conf *conf,
> > +					 const struct rte_flow_action
> > *actions,
> > +					 void *user_data,
> > +					 struct rte_flow_error *error);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Destroy indirect actions list by handle.
> > + *
> > + * @param[in] port_id
> > + *    The port identifier of the Ethernet device.
> > + * @param[in] handle
> > + *   Handle for the indirect actions list to be destroyed.
> > + * @param[out] error
> > + *   Perform verbose error reporting if not NULL. PMDs initialize this
> > + *   structure in case of error only.
> > + * @return
> > + *   - (0) if success.
> > + *   - (-ENODEV) if *port_id* invalid.
> > + *   - (-ENOSYS) if underlying device does not support this functionality.
> > + *   - (-EIO) if underlying device is removed.
> > + *   - (-ENOENT) if actions list pointed by *action* handle was not found.
> > + *   - (-EBUSY) if actions list pointed by *action* handle still used
> > + *   rte_errno is also set.
> Why not keep the same return description as above?
> 

Will update the format in v4.

> > + */
> > +__rte_experimental
> > +int
> > +rte_flow_action_list_handle_destroy(uint16_t port_id,
> > +				    struct rte_flow_action_list_handle
> > *handle,
> > +				    struct rte_flow_error *error);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Enqueue indirect action list destruction operation.
> > + * The destroy queue must be the same
> > + * as the queue on which the action was created.
> > + *
> > + * @param[in] port_id
> > + *   Port identifier of Ethernet device.
> > + * @param[in] queue_id
> > + *   Flow queue which is used to destroy the rule.
> > + * @param[in] op_attr
> > + *   Indirect action destruction operation attributes.
> > + * @param[in] handle
> > + *   Handle for the indirect action object to be destroyed.
> > + * @param[in] user_data
> > + *   The user data that will be returned on the completion events.
> > + * @param[out] 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.
> Same errors as above?
> 

Will update the format in v4.

> > + */
> > +__rte_experimental
> > +int
> > +rte_flow_async_action_list_handle_destroy
> > +		(uint16_t port_id, uint32_t queue_id,
> > +		 const struct rte_flow_op_attr *op_attr,
> > +		 struct rte_flow_action_list_handle *handle,
> > +		 void *user_data, struct rte_flow_error *error);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Query and/or update indirect flow actions list.
> > + * 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.
> > + * If ether query or update is NULL, the function executes
> > + * complementing operation.
> > + *
> > + * @param port_id
> > + *   Port identifier of Ethernet device.
> > + * @param handle
> > + *   Handle for the indirect actions list object to be updated.
> > + * @param update
> > + *   If not NULL, update profile specification used to modify the action
> > + *   pointed by handle.
> > + *   @see struct rte_flow_action_indirect_list
> It should be clear that the update/query is in the order the action was
> created and if the action i doesn't have configuration it should be s set to NULL.
> And goes without saying that the list size must be equal to the number of actions,
> that this list is build from.
> 

Will update in v4.

> > + * @param query
> > + *   If not NULL pointer to storage for the associated query data type.
> > + *   @see struct rte_flow_action_indirect_list
> > + * @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.
> > + * - (-ENODEV) if *port_id* invalid.
> > + * - (-ENOTSUP) if underlying device does not support this functionality.
> > + * - (-EINVAL) if *handle* or *mode* invalid or
> > + *             both *query* and *update* are NULL.
> > + */
> > +__rte_experimental
> > +int
> > +rte_flow_action_list_handle_query_update(uint16_t port_id,
> > +					 const struct
> > +					 rte_flow_action_list_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 actions list 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 actions list object to be updated.
> > + * @param update
> > + *   If not NULL, update profile specification used to modify the action
> > + *   pointed by handle.
> > + *   @see struct rte_flow_action_indirect_list
> > + * @param query
> > + *   If not NULL, pointer to storage for the associated query data type.
> > + *   Query result returned on async completion event.
> > + *   @see struct rte_flow_action_indirect_list
> > + * @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.
> > + * - (-ENODEV) if *port_id* invalid.
> > + * - (-ENOTSUP) if underlying device does not support this functionality.
> > + * - (-EINVAL) if *handle* or *mode* invalid or
> > + *             both *update* and *query* are NULL.
> > + */
> > +__rte_experimental
> > +int
> > +rte_flow_async_action_list_handle_query_update(uint16_t port_id,
> > uint32_t queue_id,
> > +					  const struct rte_flow_op_attr *attr,
> > +					  const struct
> > +					  rte_flow_action_list_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 a129a4605d..8dc803023c 100644
> > --- a/lib/ethdev/rte_flow_driver.h
> > +++ b/lib/ethdev/rte_flow_driver.h
> > @@ -121,6 +121,17 @@ struct rte_flow_ops {
> >  		 const void *update, void *query,
> >  		 enum rte_flow_query_update_mode qu_mode,
> >  		 struct rte_flow_error *error);
> > +	/** @see rte_flow_action_list_handle_create() */
> > +	struct rte_flow_action_list_handle *(*action_list_handle_create)
> > +		(struct rte_eth_dev *dev,
> > +		 const struct rte_flow_indir_action_conf *conf,
> > +		 const struct rte_flow_action actions[],
> > +		 struct rte_flow_error *error);
> > +	/** @see rte_flow_action_list_handle_destroy() */
> > +	int (*action_list_handle_destroy)
> > +		(struct rte_eth_dev *dev,
> > +		 struct rte_flow_action_list_handle *handle,
> > +		 struct rte_flow_error *error);
> >  	/** See rte_flow_tunnel_decap_set() */
> >  	int (*tunnel_decap_set)
> >  		(struct rte_eth_dev *dev,
> > @@ -302,6 +313,36 @@ struct rte_flow_ops {
> >  		 const void *update, void *query,
> >  		 enum rte_flow_query_update_mode qu_mode,
> >  		 void *user_data, struct rte_flow_error *error);
> > +	/** @see rte_flow_async_action_list_handle_create() */
> > +	struct rte_flow_action_list_handle *
> > +	(*async_action_list_handle_create)
> > +		(struct rte_eth_dev *dev, uint32_t queue_id,
> > +		 const struct rte_flow_op_attr *attr,
> > +		 const struct rte_flow_indir_action_conf *conf,
> > +		 const struct rte_flow_action *actions,
> > +		 void *user_data, struct rte_flow_error *error);
> > +	/** @see rte_flow_async_action_list_handle_destroy() */
> > +	int (*async_action_list_handle_destroy)
> > +		(struct rte_eth_dev *dev, uint32_t queue_id,
> > +		 const struct rte_flow_op_attr *op_attr,
> > +		 struct rte_flow_action_list_handle *action_handle,
> > +		 void *user_data, struct rte_flow_error *error);
> > +	/** @see rte_flow_action_list_handle_query_update() */
> > +	int (*action_list_handle_query_update)
> > +		(uint16_t port_id,
> > +		 const struct rte_flow_action_list_handle *handle,
> > +		 const void **update, void **query,
> > +		 enum rte_flow_query_update_mode mode,
> > +		 struct rte_flow_error *error);
> > +	/** @see rte_flow_async_action_list_handle_query_update() */
> > +	int (*async_action_list_handle_query_update)
> > +		(uint16_t port_id, uint32_t queue_id,
> > +		 const struct rte_flow_op_attr *attr,
> > +		 const struct rte_flow_action_list_handle *handle,
> > +		 const void **update, void **query,
> > +		 enum rte_flow_query_update_mode mode,
> > +		 void *user_data, struct rte_flow_error *error);
> > +
> >  };
> >
> >  /**
> > --
> > 2.34.1
> 
> Best,
> Ori