Re: [dpdk-dev] [RFC PATCH] ethdev: mtr: enhance input color table features

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

Hi Jerin,

> -----Original Message-----
> From: jerinj@marvell.com <jerinj@marvell.com>
> Sent: Friday, August 20, 2021 9:24 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: dev@dpdk.org; arybchenko@solarflare.com; lizh@nvidia.com;
> ajit.khaparde@broadcom.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>; matan@nvidia.com;
> ndabilpuram@marvell.com; skori@marvell.com; rkudurumalla@marvell.com;
> Jerin Jacob <jerinj@marvell.com>
> Subject: [dpdk-dev] [RFC PATCH] ethdev: mtr: enhance input color table
> features
> 
> From: Jerin Jacob <jerinj@marvell.com>
> 
> Currently, meter object supports only DSCP based on input color table,
> The patch enhance that to support VLAN based input color table,
> color table based on inner field for the tunnel use case,  and support
> for fallback color per meter if packet based on a different field.
> 
> All of the above features are exposed through capability and added
> additional capability to specify the implementation supports
> more than one input color table per ethdev port.
> 
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> ---
>  lib/ethdev/rte_mtr.h | 130
> ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 116 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/ethdev/rte_mtr.h b/lib/ethdev/rte_mtr.h
> index dc246dd7af..311e8754de 100644
> --- a/lib/ethdev/rte_mtr.h
> +++ b/lib/ethdev/rte_mtr.h
> @@ -213,6 +213,16 @@ struct rte_mtr_meter_policy_params {
>  	const struct rte_flow_action *actions[RTE_COLORS];
>  };
> 
> +/**
> + * Input color table
> + */
> +enum rte_mtr_input_color_tbl {
> +	/** DSCP based input color table */
> +	RTE_MTR_INPUT_COLOR_TBL_DSCP,
> +	/** VLAN based input color table */
> +	RTE_MTR_INPUT_COLOR_TBL_VLAN,
> +};
> +
>  /**
>   * Parameters for each traffic metering & policing object
>   *
> @@ -233,20 +243,44 @@ struct rte_mtr_params {
>  	 */
>  	int use_prev_mtr_color;
> 
> -	/** Meter input color. When non-NULL: it points to a pre-allocated
> and
> -	 * pre-populated table with exactly 64 elements providing the input
> -	 * color for each value of the IPv4/IPv6 Differentiated Services Code
> -	 * Point (DSCP) input packet field. When NULL: it is equivalent to
> -	 * setting this parameter to an all-green populated table (i.e. table
> -	 * with all the 64 elements set to green color). The color blind mode
> -	 * is configured by setting *use_prev_mtr_color* to 0 and
> *dscp_table*
> -	 * to either NULL or to an all-green populated table. When
> -	 * *use_prev_mtr_color* is non-zero value or when *dscp_table*
> contains
> -	 * at least one yellow or red color element, then the color aware
> mode
> -	 * is configured.
> -	 */
> -	enum rte_color *dscp_table;
> -
> +	RTE_STD_C11
> +	union {
> +		/** Meter input color based on DSCP.
> +		 * Valid when rte_mtr_input_color_tbl::tbl_selector is
> +		 * set to RTE_MTR_INPUT_COLOR_TBL_DSCP.
> +		 * When non-NULL: it points to a pre-allocated and pre-
> populated
> +		 * table with exactly 64 elements providing the input color for
> +		 * each value of the IPv4/IPv6 Differentiated Services Code
> +		 * Point (DSCP) input packet field. When NULL:
> +		 * it is equivalent to setting this parameter to an all-green
> +		 * populated table (i.e. table with
> +		 * all the 64 elements set to green color). The color blind
> mode
> +		 * is configured by setting *use_prev_mtr_color* to 0 and
> +		 * *dscp_table* to either NULL or to an all-green
> +		 * populated table. When *use_prev_mtr_color* is non-zero
> value
> +		 * or when *dscp_table* contains at least one yellow or
> +		 * red color element, then the color aware mode is
> configured.
> +		 * @see struct
> rte_mtr_capabilities::input_color_dscp_supported
> +		 */
> +		enum rte_color *dscp_table;
> +		/** Meter input color based on VLAN.
> +		 * Valid when rte_mtr_input_color_tbl::tbl_selector is
> +		 * set to RTE_MTR_INPUT_COLOR_TBL_VLAN.
> +		 * When non-NULL: it points to a pre-allocated and pre-
> populated
> +		 * table with exactly 16 elements providing the input color for
> +		 * each value of the DEI(1bit), PCP(3 bits) input packet field.
> +		 * When NULL: it is equivalent to setting this parameter to an
> +		 * all-green populated table (i.e. table with
> +		 * all the 16 elements set to green color). The color blind
> mode
> +		 * is configured by setting *use_prev_mtr_color* to 0 and
> +		 * *vlan_table* to either NULL or to an all-green
> +		 * populated table. When *use_prev_mtr_color* is non-zero
> value
> +		 * or when *vlan_table* contains at least one yellow or
> +		 * red color element, then the color aware mode is
> configured.
> +		 * @see struct
> rte_mtr_capabilities::input_color_vlan_supported
> +		 */
> +		enum rte_color *vlan_table;
> +	};
>  	/** Non-zero to enable the meter, zero to disable the meter at the
> time
>  	 * of MTR object creation. Ignored when the meter profile indicated
> by
>  	 * *meter_profile_id* is set to NONE.
> @@ -261,6 +295,25 @@ struct rte_mtr_params {
> 
>  	/** Meter policy ID. */
>  	uint32_t meter_policy_id;
> +
> +	/** Select the input color table
> +	 * @see struct rte_mtr_params::dscp_table
> +	 * @see struct rte_mtr_capabilities::input_color_dscp_supported
> +	 * @see struct rte_mtr_params::vlan_table
> +	 * @see struct rte_mtr_capabilities::input_color_vlan_supported
> +	 */
> +	enum rte_mtr_input_color_tbl tbl_selector;
> +	/** Fallback input color for the meter,
> +	 *  when *use_prev_mtr_color* set to zero value and
> +	 *  when packet is not based on selected *tbl_selector*.
> +	 *  @see struct rte_mtr_capabilities::input_color_fallback_supported
> +	 */
> +	enum rte_color fallback_input_color;
> +	/** Input color table based on inner field of selected
> +	 *  of *tbl_selector*.
> +	 *  @see struct rte_mtr_capabilities::input_color_inner_supported
> +	 */
> +	int input_color_inner_enable;
>  };
> 
>  /**
> @@ -417,6 +470,31 @@ struct rte_mtr_capabilities {
>  	 * @see enum rte_mtr_stats_type
>  	 */
>  	uint64_t stats_mask;
> +
> +	/** Input color based on DSCP.
> +	 * When non-zero, it indicates that driver supports input color table
> +	 * based on DSCP.
> +	 */
> +	int input_color_dscp_supported;
> +	/** Input color based on VLAN.
> +	 * When non-zero, it indicates that driver supports input color table
> +	 * based on VLAN.
> +	 */
> +	int input_color_vlan_supported;
> +	/** Input color fallback support.
> +	 * When non-zero, it indicates that driver supports input color
> +	 * fallback.
> +	 */
> +	int input_color_fallback_supported;
> +	/** Input color based on inner packet field.
> +	 * When non-zero, it indicates that driver supports input color
> +	 * based on inner field.
> +	 */
> +	int input_color_inner_supported;
> +	 /** When non-zero, it indicates that driver supports separate
> +	  * input color table for given ethdev port.
> +	  */
> +	int seperate_input_color_table_per_port;
>  };
> 
>  /**
> @@ -832,6 +910,30 @@ rte_mtr_meter_dscp_table_update(uint16_t
> port_id,
>  	enum rte_color *dscp_table,
>  	struct rte_mtr_error *error);
> 
> +/**
> + * MTR object VLAN table update
> + *
> + * @param[in] port_id
> + *   The port identifier of the Ethernet device.
> + * @param[in] mtr_id
> + *   MTR object ID. Needs to be valid.
> + * @param[in] vlan_table
> + *   When non-NULL: it points to a pre-allocated and pre-populated table
> with
> + *   exactly 16 elements providing the input color for each value of the
> + *   each value of the DEI(1bit), PCP(3 bits) input packet field.
> + *   When NULL: it is equivalent to setting this parameter to an "all-green"
> + *   populated table (i.e. table with all the 16 elements set to green color).
> + * @param[out] error
> + *   Error details. Filled in only on error, when not NULL.
> + * @return
> + *   0 on success, non-zero error code otherwise.
> + */
> +__rte_experimental
> +int
> +rte_mtr_meter_vlan_table_update(uint16_t port_id,
> +	uint32_t mtr_id,
> +	enum rte_color *vlan_table,
> +	struct rte_mtr_error *error);
>  /**
>   * MTR object enabled statistics counters update
>   *
> --
> 2.33.0

Allowing the configuration of the packet input color based on multiple protocols as opposed to just IP DSCP field makes sense to me.

A few questions/suggestions:

1. How do we decide which field/protocol takes precedence to define the packet input color? You are enabling 2 possible options so far, i.e. VLAN tag PCP field and IP DSCP field, which one is to be set for the current meter object? Using the capabilities to  decide is confusing, as a specific meter object might be able to support multiple or even all the possible options (e.g. the meter object is fine with either IP DSCP or VLAN PCP). Therefore, we need  a clear mechanism to unequivocally pick one; I think the user must explicitly define which input color methodology is to be used by explicitly setting a field (to be added) in the meter object parameter structure.

2. What if the defined input color methodology is not applicable to the current input packet? For example, the user selects VLAN PCP, but some or all of the input packets do not contain any VLAN labels?

3. How do we manage the many packet fields that could be used as the key for the input color: outer IP DSCP, inner IP DSCP, VLAN 1st label, VLAN 2nd label, MPLS QoS, etc?
- Approach A: Use an enumeration, like you propose, and we can add more in the future. Not ideal.
- Approach B: Point to a protocol-agnostic packet field by defining the offset and mask of a 64-bit field. Advantage: we don't need to change the API every time we add a new option.

4. I suggest we use a unified input color table as opposed to one per protocol, i.e. rename the struct rte_mtr_params::dscp_table to color_in_table. The size of this table could be implicitly defined by the packet field type (in case of enum) or the field mask (in case of protocol-agnostic field offset and mask), as described above.

Regards,
Cristian