[RFC] 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(-)

--
2.33.0
  

Ping


On Fri, Aug 20, 2021 at 1:56 PM <jerinj@marvell.com> wrote:
>
> 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
>
  

On 8/30/2021 10:23 AM, Jerin Jacob wrote:
> Ping
> 
> 
> On Fri, Aug 20, 2021 at 1:56 PM <jerinj@marvell.com> wrote:
>>
>> 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>


Hi Cristian,

Reminder of this set, can you please review it?
  

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
  

On Mon, Oct 11, 2021 at 8:53 PM Dumitrescu, Cristian
<cristian.dumitrescu@intel.com> wrote:
>
> Hi Jerin,

Hi Cristian,


>
> > -----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>

> > 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.
>

Apologies for the delay in response. This was missed the 21.11
timeframe so I don't bother replying. Reviving back.

> 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.

Currently, in our HW, Each meter object needs to tell which pre-color
scheme it is going to use(IP DSCP or VLAN PCP). This is OK in the
overall scheme of things as the meter object is connected to rte_flow.
So,
rte_flow pattern can decide the steering to meter. Having said that,
it is possible to have array for input color as you suggested. If
choose that, path, We need to add capability tell implementation
supports only one input color per meter object. Let me know if this is
OK.


>
> 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?

it picks the rte_mtr_params::fallback_input_color

>
> 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.

No strong opinion on doing Approach B. I think, it may be overkill for
application and implementation to express. No strong opinion, If you
have a strong opinion on that, I will change that to v1. Let me know.


>
> 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.

Will do that. I thought not to do that just because, I don't want to
remove the existing dscp_table symbol.
Make sense to enable your suggestion.

Let me know your views on Questions 1 and 3. I will send the next
version based on that.



>
> Regards,
> Cristian
  

On Wed, Nov 17, 2021 at 5:30 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Mon, Oct 11, 2021 at 8:53 PM Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com> wrote:
> >
> > Hi Jerin,
>
Hi Cristian,

Could you share your feedback so that I can send the v1?


>
>
> >
> > > -----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>
>
> > > 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.
> >
>
> Apologies for the delay in response. This was missed the 21.11
> timeframe so I don't bother replying. Reviving back.
>
> > 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.
>
> Currently, in our HW, Each meter object needs to tell which pre-color
> scheme it is going to use(IP DSCP or VLAN PCP). This is OK in the
> overall scheme of things as the meter object is connected to rte_flow.
> So,
> rte_flow pattern can decide the steering to meter. Having said that,
> it is possible to have array for input color as you suggested. If
> choose that, path, We need to add capability tell implementation
> supports only one input color per meter object. Let me know if this is
> OK.
>
>
> >
> > 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?
>
> it picks the rte_mtr_params::fallback_input_color
>
> >
> > 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.
>
> No strong opinion on doing Approach B. I think, it may be overkill for
> application and implementation to express. No strong opinion, If you
> have a strong opinion on that, I will change that to v1. Let me know.
>
>
> >
> > 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.
>
> Will do that. I thought not to do that just because, I don't want to
> remove the existing dscp_table symbol.
> Make sense to enable your suggestion.
>
> Let me know your views on Questions 1 and 3. I will send the next
> version based on that.
>
>
>
> >
> > Regards,
> > Cristian
  

HI Jerin,

Sorry for my delay. I am currently in vacation until the beginning on January 2022, so my response is slower than usual.

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, December 7, 2021 9:55 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: jerinj@marvell.com; Thomas Monjalon <thomas@monjalon.net>; Yigit,
> Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; 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
> Subject: Re: [dpdk-dev] [RFC PATCH] ethdev: mtr: enhance input color table
> features
> 
> On Wed, Nov 17, 2021 at 5:30 PM Jerin Jacob <jerinjacobk@gmail.com>
> wrote:
> >
> > On Mon, Oct 11, 2021 at 8:53 PM Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com> wrote:
> > >
> > > Hi Jerin,
> >
> Hi Cristian,
> 
> Could you share your feedback so that I can send the v1?
> 
> 
> >
> >
> > >
> > > > -----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>
> >
> > > > 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.
> > >
> >
> > Apologies for the delay in response. This was missed the 21.11
> > timeframe so I don't bother replying. Reviving back.
> >
> > > 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.
> >
> > Currently, in our HW, Each meter object needs to tell which pre-color
> > scheme it is going to use(IP DSCP or VLAN PCP). This is OK in the
> > overall scheme of things as the meter object is connected to rte_flow.
> > So,
> > rte_flow pattern can decide the steering to meter. Having said that,
> > it is possible to have array for input color as you suggested. If
> > choose that, path, We need to add capability tell implementation
> > supports only one input color per meter object. Let me know if this is
> > OK.
> >

Thanks for the explanation of the capabilities you are trying to enable in the API. Based on this, I am reconsidering some of my previous input. Here are my current thoughts:

a) Each meter object can be configured with multiple methods to determine the input color: IP DSCP and VLAN PCP are just two of them, others are possible (as also listed by you). I think the problem we are trying to solve here is: in case multiple such methods are enabled for a given meter object AND more than one enabled method is applicable for a particular input packet at run-time (e.g. the packet is an IP packet, but it also contains at least one VLAN label, and both IP DSCP and the VLAN PCP methods are enabled for the meter object), how do we decide which method is to be applied? So, IMO we need to define a priority scheme that always picks a single method:

	- a unique priority for each method;

	- a default method to be used when none of the enabled methods is applicable to the input packet.

b) The default method must be usable even when the input packet type is unknown to the HW (unsupported), e.g. we should still be able to decide the input color of a packet that is not an IP packet, such as ARP, and it does not contain any VLAN labels. For this, I suggest we add a field ins struct rte_mtr_params called default_input_color of type enum rte_color:

	struct rte_mtr_params {
		enum rte_color default_input_color; /* Input color to be set for the input packet when none of the enabled input color methods is applicable to the current input packet. Ignored when this method is set to the color blind method. */
	};

c) In terms of methods and their priority, I propose to start with the following options, each of which referring to a set of methods, with a clear way to select a unique method. This is the minimal set needed to support the HW capabilities that you mention, this set can be extended as needed in the future:

	enum rte_mtr_input_color_method {
		RTE_MTR_INPUT_COLOR_METHOD_COLOR_BLIND, /* The input color is always green. The default_input_color is ignored for this method. */
		RTE_ MTR_INPUT_COLOR_METHOD_IP_DSCP, /* If the input packet is IPv4 or IPv6, its input color is detected  by the DSCP field indexing into the dscp_table. Otherwise, the default_input_color is applied. */
		RTE_ MTR_INPUT_COLOR_METHOD_OUTER_VLAN_PCP, /* If the input packet has at least one VLAN label, its input color is detected by the outermost VLAN PCP indexing into the vlan_table. Otherwise, the default_input_color is applied. */
		RTE_ MTR_INPUT_COLOR_METHOD_OUTER_VLAN_PCP_IP_DSCP, /* If the input packet has at least one VLAN label, its input color is detected by the outermost VLAN PCP indexing into the vlan_table. Otherwise, if the packet is IPv4 or IPv6, its input color is detected by the DSCP field indexing into the dscp_table. Otherwise, the default_input_color is applied. */
		RTE_ MTR_INPUT_COLOR_METHOD_INNER_VLAN_PCP, /* If the input packet has at least one VLAN label, its input color is detected by the innermost VLAN PCP indexing into the vlan_table. Otherwise, the default_input_color is applied. */
		RTE_ MTR_INPUT_COLOR_METHOD_INNER_VLAN_PCP_IP_DSCP, /* If the input packet has at least one VLAN label, its input color is detected by the innermost VLAN PCP indexing into the vlan_table. Otherwise, if the packet is IPv4 or IPv6, its input color is detected by the DSCP field indexing into the dscp_table. Otherwise, the default_input_color is applied. */
	};

	struct rte_mtr_params {
		enum rte_mtr_input_color_method input_color_method;
	};

d) The above point means that all the protocol dependent tables are independent of each other, so they can all exist at the same time, so we cannot put all of them into a union or a single unified input color translation table. In case any of these tables is enabled/required by the input color scheme, but it is set to NULL, it must automatically resolve to an "all-green" table (as it is already the case for the existing dscp_table).

	struct rte_mtr_params {
		enum rte_color *ip_dscp_table; /* Used when the IP DSCP input color method is selected for the current input packet. If set to NULL, it defaults to 64 elements of RTE_COLOR_GREEN. */
		enum rte_color *vlan_table; /* Used when the outermost/innermost VLAN PCP input color method is selected for the current input packet. If set to NULL, it defaults to all elements being RTE_COLOR_GREEN. */
	};

So many details to address in order to avoid any loopholes in this API, but I think this scheme is implementable and robust enough. What do you think?

> >
> > >
> > > 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?
> >
> > it picks the rte_mtr_params::fallback_input_color
> >

We likely need a more complex scheme, see above ;)

> > >
> > > 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.
> >
> > No strong opinion on doing Approach B. I think, it may be overkill for
> > application and implementation to express. No strong opinion, If you
> > have a strong opinion on that, I will change that to v1. Let me know.
> >

I would love to have a protocol agnostic scheme, but I am OK to start with a simple scheme for now (if you can call the above scheme as being simple ;) )and revisit later on if needed.

> >
> > >
> > > 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.
> >
> > Will do that. I thought not to do that just because, I don't want to
> > remove the existing dscp_table symbol.
> > Make sense to enable your suggestion.
> >
> > Let me know your views on Questions 1 and 3. I will send the next
> > version based on that.
> >

Based on the above comments, I no longer thing a unified such table would work.

> >
> >
> > >
> > > Regards,
> > > Cristian

Regards,
Cristian
  

On Tue, Dec 7, 2021 at 11:52 PM Dumitrescu, Cristian
<cristian.dumitrescu@intel.com> wrote:
>
> HI Jerin,

Hi Chistian,


>
> Sorry for my delay. I am currently in vacation until the beginning on January 2022, so my response is slower than usual.

I was too on vacation.

>
> Thanks for the explanation of the capabilities you are trying to enable in the API. Based on this, I am reconsidering some of my previous input. Here are my current thoughts:
>
> a) Each meter object can be configured with multiple methods to determine the input color: IP DSCP and VLAN PCP are just two of them, others are possible (as also listed by you). I think the problem we are trying to solve here is: in case multiple such methods are enabled for a given meter object AND more than one enabled method is applicable for a particular input packet at run-time (e.g. the packet is an IP packet, but it also contains at least one VLAN label, and both IP DSCP and the VLAN PCP methods are enabled for the meter object), how do we decide which method is to be applied? So, IMO we need to define a priority scheme that always picks a single method:
>
>         - a unique priority for each method;
>
>         - a default method to be used when none of the enabled methods is applicable to the input packet.
>
> b) The default method must be usable even when the input packet type is unknown to the HW (unsupported), e.g. we should still be able to decide the input color of a packet that is not an IP packet, such as ARP, and it does not contain any VLAN labels. For this, I suggest we add a field ins struct rte_mtr_params called default_input_color of type enum rte_color:
>
>         struct rte_mtr_params {
>                 enum rte_color default_input_color; /* Input color to be set for the input packet when none of the enabled input color methods is applicable to the current input packet. Ignored when this method is set to the color blind method. */
>         };
>
> c) In terms of methods and their priority, I propose to start with the following options, each of which referring to a set of methods, with a clear way to select a unique method. This is the minimal set needed to support the HW capabilities that you mention, this set can be extended as needed in the future:
>
>         enum rte_mtr_input_color_method {
>                 RTE_MTR_INPUT_COLOR_METHOD_COLOR_BLIND, /* The input color is always green. The default_input_color is ignored for this method. */
>                 RTE_ MTR_INPUT_COLOR_METHOD_IP_DSCP, /* If the input packet is IPv4 or IPv6, its input color is detected  by the DSCP field indexing into the dscp_table. Otherwise, the default_input_color is applied. */
>                 RTE_ MTR_INPUT_COLOR_METHOD_OUTER_VLAN_PCP, /* If the input packet has at least one VLAN label, its input color is detected by the outermost VLAN PCP indexing into the vlan_table. Otherwise, the default_input_color is applied. */
>                 RTE_ MTR_INPUT_COLOR_METHOD_OUTER_VLAN_PCP_IP_DSCP, /* If the input packet has at least one VLAN label, its input color is detected by the outermost VLAN PCP indexing into the vlan_table. Otherwise, if the packet is IPv4 or IPv6, its input color is detected by the DSCP field indexing into the dscp_table. Otherwise, the default_input_color is applied. */
>                 RTE_ MTR_INPUT_COLOR_METHOD_INNER_VLAN_PCP, /* If the input packet has at least one VLAN label, its input color is detected by the innermost VLAN PCP indexing into the vlan_table. Otherwise, the default_input_color is applied. */
>                 RTE_ MTR_INPUT_COLOR_METHOD_INNER_VLAN_PCP_IP_DSCP, /* If the input packet has at least one VLAN label, its input color is detected by the innermost VLAN PCP indexing into the vlan_table. Otherwise, if the packet is IPv4 or IPv6, its input color is detected by the DSCP field indexing into the dscp_table. Otherwise, the default_input_color is applied. */
>         };
>
>         struct rte_mtr_params {
>                 enum rte_mtr_input_color_method input_color_method;
>         };
>
> d) The above point means that all the protocol dependent tables are independent of each other, so they can all exist at the same time, so we cannot put all of them into a union or a single unified input color translation table. In case any of these tables is enabled/required by the input color scheme, but it is set to NULL, it must automatically resolve to an "all-green" table (as it is already the case for the existing dscp_table).
>
>         struct rte_mtr_params {
>                 enum rte_color *ip_dscp_table; /* Used when the IP DSCP input color method is selected for the current input packet. If set to NULL, it defaults to 64 elements of RTE_COLOR_GREEN. */
>                 enum rte_color *vlan_table; /* Used when the outermost/innermost VLAN PCP input color method is selected for the current input packet. If set to NULL, it defaults to all elements being RTE_COLOR_GREEN. */
>         };
>
> So many details to address in order to avoid any loopholes in this API, but I think this scheme is implementable and robust enough. What do you think?


Sounds good. I will next version based on your suggestion.




>
> > >
> > > >
> > > > 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?
> > >
> > > it picks the rte_mtr_params::fallback_input_color
> > >
>
> We likely need a more complex scheme, see above ;)
>
> > > >
> > > > 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.
> > >
> > > No strong opinion on doing Approach B. I think, it may be overkill for
> > > application and implementation to express. No strong opinion, If you
> > > have a strong opinion on that, I will change that to v1. Let me know.
> > >
>
> I would love to have a protocol agnostic scheme, but I am OK to start with a simple scheme for now (if you can call the above scheme as being simple ;) )and revisit later on if needed.
>
> > >
> > > >
> > > > 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.
> > >
> > > Will do that. I thought not to do that just because, I don't want to
> > > remove the existing dscp_table symbol.
> > > Make sense to enable your suggestion.
> > >
> > > Let me know your views on Questions 1 and 3. I will send the next
> > > version based on that.
> > >
>
> Based on the above comments, I no longer thing a unified such table would work.
>
> > >
> > >
> > > >
> > > > Regards,
> > > > Cristian
>
> Regards,
> Cristian