RE: [PATCH] ethdev: introduce generic flow item and action

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Qi Zhang [mailto:qi.z.zhang@intel.com]
> Sent: Wednesday, 2 August 2023 19.35
> 
> From: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> 
> For network devices that are programmable through languages such as
> the P4 language, there are no pre-defined flow items and actions.
> 
> The format of the protocol header and metadata fields that are used to
> specify the flow items that make up the flow pattern, as well as the
> flow actions, are all defined by the program, with an infinity of
> possible combinations, as opposed to being selected from a finite
> pre-defined list.
> 
> It is virtually impossible to pre-define all the flow items and the
> flow actions that programs might ever use, as these are only limited
> by the set of HW resources and the program developer's imagination.
> 
> To support the programmable network devices, we are introducing:
> 
> * A generic flow item: The flow item is expressed as an array of bytes
> of a given length, whose meaning is defined by the program loaded by
> the network device.

The flow item is not "generic", it is "opaque": Only the application knows what this flow item does.

I hate the concept for two reasons:
1. The inability for applications to detect which flow items the underlying hardware supports.
2. The risk that vendors will use this instead of introducing new flow item types, available for anyone to implement.

If you proceed anyway, there's a few comments inline below.

> The device is still expected to accept the
> existing pre-defined flow items such as Ethernet, IPv4/IPv6 headers,
> etc, as long as the program currently loaded on the device is defining
> and using flow items with identical format, but the device is not
> limited to the current set of pre-defined RTE_FLOW flow items.
> 
> * A generic flow action: The flow action exact processing is defined
> by the program loaded by the network device, with the user expected to
> know the set of program actions for the purpose of assigning actions
> to flows. The device is still expected to accept the existing
> pre-defined flow actions such as packet drop, packet redirection to
> output queues, packet modifications, etc, as long as the program
> currently loaded on the device is defining and using flow actions that
> perform identical processing, but the device is not limited to the
> current set of pre-defined RTE_FLOW flow actions.
> 
> Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  lib/ethdev/rte_flow.h | 82 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index 3fe57140f9..f7889d7dd0 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -688,6 +688,15 @@ enum rte_flow_item_type {
>  	 * @see struct rte_flow_item_ib_bth.
>  	 */
>  	RTE_FLOW_ITEM_TYPE_IB_BTH,
> +
> +	/**
> +	 * Matches a custom protocol header or metadata field represented
> +	 * as a byte string of a given length, whose meaning is typically
> +	 * defined by the data plane program.
> +	 *
> +	 * @see struct rte_flow_item_generic.
> +	 */
> +	RTE_FLOW_ITEM_TYPE_GENERIC,

Rename: _GENERIC -> _OPAQUE, everywhere.

>  };
> 
>  /**
> @@ -2311,6 +2320,32 @@ static const struct rte_flow_item_tx_queue
> rte_flow_item_tx_queue_mask = {
>  };
>  #endif
> 
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ITEM_TYPE_GENERIC
> + *
> + * Matches a custom protocol header or metadata field represented as a byte
> + * array of a given length, whose meaning is typically defined by the data
> + * plane program.

"byte array" -> "opaque data type"

> + *
> + * The field length must be non-zero. The field value must be non-NULL, with
> the
> + * value bytes specified in network byte order.
> + */
> +struct rte_flow_item_generic {
> +	uint32_t length; /**< Field length. */
> +	const uint8_t *value; /**< Field value. */

Suggestion: Change the value type from "const uint8_t *" to "const void *". It makes it easier for the application to use a hierarchy of structures internally for this.

> +};
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_GENERIC. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_generic rte_flow_item_generic_mask = {
> +	.length = 0xffffffff,
> +	.value = NULL,
> +};
> +#endif
> +
>  /**
>   * Action types.
>   *
> @@ -2989,6 +3024,14 @@ enum rte_flow_action_type {
>  	 * @see struct rte_flow_action_indirect_list
>  	 */
>  	RTE_FLOW_ACTION_TYPE_INDIRECT_LIST,
> +
> +	/**
> +	 * Custom action whose processing is typically defined by the data plane
> +	 * program.
> +	 *
> +	 * @see struct rte_flow_action_generic.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_GENERIC,
>  };
> 
>  /**
> @@ -4064,6 +4107,45 @@ struct rte_flow_indirect_update_flow_meter_mark {
>  	enum rte_color init_color;
>  };
> 
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice.
> + *
> + * Generic action argument configuration parameters.
> + *
> + * The action argument field length must be non-zero. The action argument
> field
> + * value must be non-NULL, with the value bytes specified in network byte
> order.
> + *
> + * @see struct rte_flow_action_generic
> + */
> +struct rte_flow_action_generic_argument {
> +	/** Argument field length. */
> +	uint32_t length;
> +	/** Argument field value. */
> +	const uint8_t *value;
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice.
> + *
> + * RTE_FLOW_ACTION_TYPE_GENERIC
> + *
> + * Generic action configuration parameters.
> + *
> + * Each action can have zero or more arguments.
> + *
> + * @see RTE_FLOW_ACTION_TYPE_GENERIC
> + */
> +struct rte_flow_action_generic {
> +	/** Action ID. */
> +	uint32_t action_id;
> +	/** Number of action arguments. */
> +	uint32_t action_args_num;
> +	/** Action arguments array. */
> +	const struct rte_flow_action_generic_argument *action_args;
> +};

This is a list of arguments, not one argument. You should append "_array" somewhere in the name, and add a normal (non-list) action, e.g.:

struct rte_flow_action_opaque {
//Or: "struct rte_flow_action_generic", if you want to keep that name.
	/** Action ID. */
	uint32_t action_id;
	/** Argument length. */
	uint32_t length;
	/** Argument value. */
	const uint8_t *value;
// Or: "const void *value;" for the same reason as for the flow item.
};


> +
>  /* Mbuf dynamic field offset for metadata. */
>  extern int32_t rte_flow_dynf_metadata_offs;
> 
> --
> 2.31.1