[v3,1/1] ethdev: mtr: support input color selection

  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

Suggested-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Signed-off-by: Jerin Jacob <jerinj@marvell.com>
---
v3..v2:
- Fix input color flags as a bitmask
- Add definitions for newly added API

v2..v1:

- Fix seperate typo

v1..RFC:

Address the review comments by Cristian at
https://patches.dpdk.org/project/dpdk/patch/20210820082401.3778736-1-jerinj@marvell.com/

- Moved to v22.07 release
- Updated rte_mtr_input_color_method to support all VLAN, DSCP, Inner cases
- Added input_color_method
- Removed union between vlan_table and dscp_table
- Kept VLAN instead of PCP as HW coloring based on DEI(1bit), PCP(3 bits)

What is missing:
- 22.07 release notes update

 .../traffic_metering_and_policing.rst         |  28 +++
 lib/ethdev/rte_mtr.c                          |  12 ++
 lib/ethdev/rte_mtr.h                          | 164 +++++++++++++++++-
 lib/ethdev/rte_mtr_driver.h                   |   9 +
 lib/ethdev/version.map                        |   3 +
 5 files changed, 214 insertions(+), 2 deletions(-)
  

Corresponding driver implementation is available at http://patches.dpdk.org/project/dpdk/patch/20220301090056.1042866-1-skori@marvell.com/.
Also CLI is added to testpmd for below API and available at http://patches.dpdk.org/project/dpdk/patch/20220301090056.1042866-3-skori@marvell.com/ 

> -----Original Message-----
> From: skori@marvell.com <skori@marvell.com>
> Sent: Tuesday, March 1, 2022 2:28 PM
> To: Cristian Dumitrescu <cristian.dumitrescu@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Ray Kinsella
> <mdr@ashroe.eu>
> Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Subject: [EXT] [PATCH v3 1/1] ethdev: mtr: support input color selection
> 
> External Email
> 
> ----------------------------------------------------------------------
> 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
> 
> Suggested-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> ---
> v3..v2:
> - Fix input color flags as a bitmask
> - Add definitions for newly added API
> 
> v2..v1:
> 
> - Fix seperate typo
> 
> v1..RFC:
> 
> Address the review comments by Cristian at
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__patches.dpdk.org_project_dpdk_patch_20210820082401.3778736-2D1-
> 2Djerinj-
> 40marvell.com_&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5
> COgn1zxHMyaF1_d9IIuq6vHQO6NrIPjaE&m=bvYbz6aP4xuLPRt2TIk1o99kWXj
> u-rRDvz3qXnCVbz_TER0zcRDqjQxJSm3SnLXM&s=mVeBbQWjsvJLRm-dcB4h-
> KmAj3tGi4WT0E9IvsuNT2g&e=
> 
> - Moved to v22.07 release
> - Updated rte_mtr_input_color_method to support all VLAN, DSCP, Inner
> cases
> - Added input_color_method
> - Removed union between vlan_table and dscp_table
> - Kept VLAN instead of PCP as HW coloring based on DEI(1bit), PCP(3 bits)
> 
> What is missing:
> - 22.07 release notes update
> 
>  .../traffic_metering_and_policing.rst         |  28 +++
>  lib/ethdev/rte_mtr.c                          |  12 ++
>  lib/ethdev/rte_mtr.h                          | 164 +++++++++++++++++-
>  lib/ethdev/rte_mtr_driver.h                   |   9 +
>  lib/ethdev/version.map                        |   3 +
>  5 files changed, 214 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/traffic_metering_and_policing.rst
> b/doc/guides/prog_guide/traffic_metering_and_policing.rst
> index ceb5a96488..59ebd361ba 100644
> --- a/doc/guides/prog_guide/traffic_metering_and_policing.rst
> +++ b/doc/guides/prog_guide/traffic_metering_and_policing.rst
> @@ -21,6 +21,7 @@ The main features are:
>  * Policer actions (per meter output color): recolor, drop
>  * Statistics (per policer output color)
>  * Chaining multiple meter objects
> +* Packet content based input color selection
> 
>  Configuration steps
>  -------------------
> @@ -105,3 +106,30 @@ traffic meter and policing library.
>     * Adding one (or multiple) actions of the type
> ``RTE_FLOW_ACTION_TYPE_METER``
>       to the list of meter actions (``struct
> rte_mtr_meter_policy_params::actions``)
>       specified per color as show in :numref:`figure_rte_mtr_chaining`.
> +
> +Packet content based input color selection
> +------------------------------------------
> +
> +The API supports selecting the input color based on the packet content.
> +Following is the API usage model for the same.
> +
> +#. Probe the input color selection device capabilities using following
> +   parameter using ``rte_mtr_capabilities_get()`` API
> +   ``struct rte_mtr_capabilities::methods_mask`` and
> +   ``struct rte_mtr_capabilitie::separate_input_color_table_per_port``
> +
> +#. When creating the meter object using ``rte_mtr_create()``, configure
> +   relevant input color selection parameters such as
> +
> +   * Select the input color method ``struct
> + rte_mtr_params::input_color_method``
> +
> +   * Fill the tables ``struct rte_mtr_params::dscp_table``,
> +     ``struct rte_mtr_params::dscp_table`` based on input color
> + selected
> +
> +   * Update the ``struct rte_mtr_params::default_input_color`` to determine
> +     the default input color in case the input packet does not match
> +     the input color method
> +
> +   * If needed, update the input color table at runtime using
> +     ``rte_mtr_meter_vlan_table_update()`` and
> ``rte_mtr_meter_dscp_table_update()``
> +     APIs.
> diff --git a/lib/ethdev/rte_mtr.c b/lib/ethdev/rte_mtr.c index
> e49fcf271c..a0cb91e0b1 100644
> --- a/lib/ethdev/rte_mtr.c
> +++ b/lib/ethdev/rte_mtr.c
> @@ -207,6 +207,18 @@ rte_mtr_meter_dscp_table_update(uint16_t
> port_id,
>  		mtr_id, dscp_table, error);
>  }
> 
> +/** MTR object meter VLAN table update */ 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)
> +{
> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> +	return RTE_MTR_FUNC(port_id, meter_vlan_table_update)(dev,
> +		mtr_id, vlan_table, error);
> +}
> +
>  /** MTR object enabled stats update */
>  int
>  rte_mtr_stats_update(uint16_t port_id,
> diff --git a/lib/ethdev/rte_mtr.h b/lib/ethdev/rte_mtr.h index
> 40df0888c8..7f9abebb41 100644
> --- a/lib/ethdev/rte_mtr.h
> +++ b/lib/ethdev/rte_mtr.h
> @@ -213,6 +213,80 @@ struct rte_mtr_meter_policy_params {
>  	const struct rte_flow_action *actions[RTE_COLORS];  };
> 
> +/**
> + * Input color method
> + */
> +enum rte_mtr_input_color_method {
> +	/**
> +	 * The input color is always green.
> +	 * The default_input_color is ignored for this method.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_COLOR_BLIND  = RTE_BIT64(0),
> +	/**
> +	 * If the input packet has at least one VLAN label, its input color is
> +	 * detected by the outermost VLAN DEI(1bit), PCP(3 bits)
> +	 * indexing into the struct rte_mtr_params::vlan_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::vlan_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_VLAN = RTE_BIT64(1),
> +	/**
> +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> +	 * the outermost DSCP field indexing into the
> +	 * struct rte_mtr_params::dscp_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::dscp_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_DSCP = RTE_BIT64(2),
> +	/**
> +	 * If the input packet has at least one VLAN label, its input color is
> +	 * detected by the outermost VLAN DEI(1bit), PCP(3 bits)
> +	 * indexing into the struct rte_mtr_params::vlan_table.
> +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> +	 * the outermost DSCP field indexing into the
> +	 * struct rte_mtr_params::dscp_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::vlan_table
> +	 * @see struct rte_mtr_params::dscp_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_VLAN_DSCP = RTE_BIT64(3),
> +	/**
> +	 * If the input packet has at least one VLAN label, its input color is
> +	 * detected by the innermost VLAN DEI(1bit), PCP(3 bits)
> +	 * indexing into the struct rte_mtr_params::vlan_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::vlan_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN = RTE_BIT64(4),
> +	/**
> +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> +	 * the innermost DSCP field indexing into the
> +	 * struct rte_mtr_params::dscp_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::dscp_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_INNER_DSCP = RTE_BIT64(5),
> +	/**
> +	 * If the input packet has at least one VLAN label, its input color is
> +	 * detected by the innermost VLAN DEI(1bit), PCP(3 bits)
> +	 * indexing into the struct rte_mtr_params::vlan_table.
> +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> +	 * the innermost DSCP field indexing into the
> +	 * struct rte_mtr_params::dscp_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::vlan_table
> +	 * @see struct rte_mtr_params::dscp_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_DSCP =
> RTE_BIT64(6), };
> +
>  /**
>   * Parameters for each traffic metering & policing object
>   *
> @@ -233,7 +307,15 @@ struct rte_mtr_params {
>  	 */
>  	int use_prev_mtr_color;
> 
> -	/** Meter input color. When non-NULL: it points to a pre-allocated
> and
> +	/** Meter input color based on IP DSCP field.
> +	 *
> +	 * Valid when input_color_method set to any of the following
> +	 * RTE_MTR_INPUT_COLOR_METHOD_DSCP,
> +	 * RTE_MTR_INPUT_COLOR_METHOD_VLAN_DSCP,
> +	 * RTE_MTR_INPUT_COLOR_METHOD_INNER_DSCP,
> +	 * RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_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 @@
> -244,9 +326,39 @@ struct rte_mtr_params {
>  	 * *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 enum
> rte_mtr_input_color_method::RTE_MTR_INPUT_COLOR_METHOD_DSCP
> +	 * @see enum
> rte_mtr_input_color_method::RTE_MTR_INPUT_COLOR_METHOD_VLAN_DS
> CP
> +	 * @see enum
> rte_mtr_input_color_method::RTE_MTR_INPUT_COLOR_METHOD_INNER_D
> SCP
> +	 * @see enum
> rte_mtr_input_color_method::RTE_MTR_INPUT_COLOR_METHOD_INNER_VL
> AN_DSCP
> +	 * @see struct rte_mtr_params::input_color_method
>  	 */
>  	enum rte_color *dscp_table;
> -
> +	/** Meter input color based on VLAN DEI(1bit), PCP(3 bits) fields.
> +	 *
> +	 * Valid when input_color_method set to any of the following
> +	 * RTE_MTR_INPUT_COLOR_METHOD_VLAN,
> +	 * RTE_MTR_INPUT_COLOR_METHOD_VLAN_DSCP,
> +	 * RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN,
> +	 * RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_DSCP
> +	 *
> +	 * 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 enum
> rte_mtr_input_color_method::RTE_MTR_INPUT_COLOR_METHOD_VLAN
> +	 * @see enum
> rte_mtr_input_color_method::RTE_MTR_INPUT_COLOR_METHOD_VLAN_DS
> CP
> +	 * @see enum
> rte_mtr_input_color_method::RTE_MTR_INPUT_COLOR_METHOD_INNER_VL
> AN
> +	 * @see enum
> rte_mtr_input_color_method::RTE_MTR_INPUT_COLOR_METHOD_INNER_VL
> AN_DSCP
> +	 * @see struct rte_mtr_params::input_color_method
> +	 */
> +	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 +373,20 @@ struct rte_mtr_params {
> 
>  	/** Meter policy ID. @see rte_mtr_meter_policy_add() */
>  	uint32_t meter_policy_id;
> +
> +	/** Input color method to select.
> +	 * @see struct rte_mtr_params::dscp_table
> +	 * @see struct rte_mtr_params::vlan_table
> +	 */
> +	enum rte_mtr_input_color_method input_color_method;
> +
> +	/** Input color to be set for the input packet when none of the
> +	 * enabled input color methods is applicable to the input packet.
> +	 * Ignored when this method is set to the
> +	 * enum
> rte_mtr_input_color_method::RTE_MTR_INPUT_COLOR_METHOD_COLOR_B
> LIND
> +	 * method.
> +	 */
> +	enum rte_color default_input_color;
>  };
> 
>  /**
> @@ -417,6 +543,16 @@ struct rte_mtr_capabilities {
>  	 * @see enum rte_mtr_stats_type
>  	 */
>  	uint64_t stats_mask;
> +
> +	/** Set of supported input color methods.
> +	 * @see enum rte_mtr_input_color_method
> +	 */
> +	uint64_t methods_mask;
> +
> +	/** When non-zero, it indicates that driver supports separate
> +	 * input color table for given ethdev port.
> +	 */
> +	int separate_input_color_table_per_port;
>  };
> 
>  /**
> @@ -832,6 +968,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
>   *
> diff --git a/lib/ethdev/rte_mtr_driver.h b/lib/ethdev/rte_mtr_driver.h index
> ee8c6ef7ad..37ab79666b 100644
> --- a/lib/ethdev/rte_mtr_driver.h
> +++ b/lib/ethdev/rte_mtr_driver.h
> @@ -97,6 +97,12 @@ typedef int
> (*rte_mtr_meter_dscp_table_update_t)(struct rte_eth_dev *dev,
>  	enum rte_color *dscp_table,
>  	struct rte_mtr_error *error);
> 
> +/** @internal MTR object meter VLAN table update. */ typedef int
> +(*rte_mtr_meter_vlan_table_update_t)(struct rte_eth_dev *dev,
> +	uint32_t mtr_id,
> +	enum rte_color *vlan_table,
> +	struct rte_mtr_error *error);
> +
>  /** @internal MTR object enabled stats update. */  typedef int
> (*rte_mtr_stats_update_t)(struct rte_eth_dev *dev,
>  	uint32_t mtr_id,
> @@ -139,6 +145,9 @@ struct rte_mtr_ops {
>  	/** MTR object meter DSCP table update */
>  	rte_mtr_meter_dscp_table_update_t meter_dscp_table_update;
> 
> +	/** MTR object meter VLAN table update */
> +	rte_mtr_meter_vlan_table_update_t meter_vlan_table_update;
> +
>  	/** MTR object enabled stats update */
>  	rte_mtr_stats_update_t stats_update;
> 
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map index
> 20391ab29e..77915c0ddc 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -279,6 +279,9 @@ EXPERIMENTAL {
>  	rte_flow_async_action_handle_create;
>  	rte_flow_async_action_handle_destroy;
>  	rte_flow_async_action_handle_update;
> +
> +	# added in 22.07
> +	rte_mtr_meter_vlan_table_update;
>  };
> 
>  INTERNAL {
> --
> 2.25.1
  

HI Jerin,

Thanks for your patch! I think we are making great progress, here are a few more comments: 

<snip>

> +/**
> + * Input color method
> + */
> +enum rte_mtr_input_color_method {

We should clean up the names of these methods a bit: we should not mix header names (VLAN, IP) with header field names (DSCP, PCP), in the sense that to me METHOD_VLAN_DSCP should be replaced with either:
* METHOD_OUTER_VLAN_IP :shorter name, as only the headers are mentioned (my preference, but I am OK with both)
* METHOD_OUTER_VLAN_PCP_IP_DSCP: longer name, as both the headers and the header fields are mentioned

Please put a blank line in between these methods to better readability.

I see some issues in the list of methods below, I am trying to do my best to catch them all:

> +	/**
> +	 * The input color is always green.
> +	 * The default_input_color is ignored for this method.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_COLOR_BLIND  = RTE_BIT64(0),

OK.

> +	/**
> +	 * If the input packet has at least one VLAN label, its input color is
> +	 * detected by the outermost VLAN DEI(1bit), PCP(3 bits)
> +	 * indexing into the struct rte_mtr_params::vlan_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::vlan_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_VLAN = RTE_BIT64(1),

OK.
Does your HW use PCP+DEI , or just PCP?

> +	/**
> +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> +	 * the outermost DSCP field indexing into the
> +	 * struct rte_mtr_params::dscp_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::dscp_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_DSCP = RTE_BIT64(2),

OK.
Please change name to METHOD_IP.
Description: Change the "outermost DSCP" to "the DSCP field of the outermost IP header".
I would move this up on the second position (to follow immediately after the color blind method).

> +	/**
> +	 * If the input packet has at least one VLAN label, its input color is
> +	 * detected by the outermost VLAN DEI(1bit), PCP(3 bits)
> +	 * indexing into the struct rte_mtr_params::vlan_table.
> +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> +	 * the outermost DSCP field indexing into the
> +	 * struct rte_mtr_params::dscp_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::vlan_table
> +	 * @see struct rte_mtr_params::dscp_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_VLAN_DSCP = RTE_BIT64(3),

OK.
Please change name to METHOD_VLAN_IP.
This should follow immediately after the METHOD_VLAN.
Description: please use "Otherwise" before "if the input packet is IP"; please replace "outermost DSCP" as above.
Is your HW using DEI + PCP or just PCP?

> +	/**
> +	 * If the input packet has at least one VLAN label, its input color is
> +	 * detected by the innermost VLAN DEI(1bit), PCP(3 bits)
> +	 * indexing into the struct rte_mtr_params::vlan_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::vlan_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN = RTE_BIT64(4),

OK.
Is your HW using DEI + PCP or just PCP?

> +	/**
> +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> +	 * the innermost DSCP field indexing into the
> +	 * struct rte_mtr_params::dscp_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::dscp_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_INNER_DSCP = RTE_BIT64(5),

This is very confusing to me, I don't get what this one is about: The "inner" word in the name suggests that inner VLAN is attempted first, then IP DSCP (if no VLAN is present), but the description only talks about IP.

> +	/**
> +	 * If the input packet has at least one VLAN label, its input color is
> +	 * detected by the innermost VLAN DEI(1bit), PCP(3 bits)
> +	 * indexing into the struct rte_mtr_params::vlan_table.
> +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> +	 * the innermost DSCP field indexing into the
> +	 * struct rte_mtr_params::dscp_table.
> +	 * Otherwise, the default_input_color is applied.
> +	 * @see struct rte_mtr_params::default_input_color
> +	 * @see struct rte_mtr_params::vlan_table
> +	 * @see struct rte_mtr_params::dscp_table
> +	 */
> +	RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_DSCP =
> RTE_BIT64(6),

OK.
Description fixes: Use "otherwise" before "if IP"; replace innermost DSCP with "DSCP field of the outermost IP header".

<snip>

Regards,
Cristian
  

Hi Jerin and folks, any update on this one? Thanks, Cristian

> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Sent: Tuesday, March 1, 2022 5:48 PM
> To: skori@marvell.com; Thomas Monjalon <thomas@monjalon.net>; Yigit,
> Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ray Kinsella <mdr@ashroe.eu>
> Cc: dev@dpdk.org; Jerin Jacob <jerinj@marvell.com>
> Subject: RE: [PATCH v3 1/1] ethdev: mtr: support input color selection
> 
> HI Jerin,
> 
> Thanks for your patch! I think we are making great progress, here are a few
> more comments:
> 
> <snip>
> 
> > +/**
> > + * Input color method
> > + */
> > +enum rte_mtr_input_color_method {
> 
> We should clean up the names of these methods a bit: we should not mix
> header names (VLAN, IP) with header field names (DSCP, PCP), in the sense
> that to me METHOD_VLAN_DSCP should be replaced with either:
> * METHOD_OUTER_VLAN_IP :shorter name, as only the headers are
> mentioned (my preference, but I am OK with both)
> * METHOD_OUTER_VLAN_PCP_IP_DSCP: longer name, as both the headers
> and the header fields are mentioned
> 
> Please put a blank line in between these methods to better readability.
> 
> I see some issues in the list of methods below, I am trying to do my best to
> catch them all:
> 
> > +	/**
> > +	 * The input color is always green.
> > +	 * The default_input_color is ignored for this method.
> > +	 * @see struct rte_mtr_params::default_input_color
> > +	 */
> > +	RTE_MTR_INPUT_COLOR_METHOD_COLOR_BLIND  = RTE_BIT64(0),
> 
> OK.
> 
> > +	/**
> > +	 * If the input packet has at least one VLAN label, its input color is
> > +	 * detected by the outermost VLAN DEI(1bit), PCP(3 bits)
> > +	 * indexing into the struct rte_mtr_params::vlan_table.
> > +	 * Otherwise, the default_input_color is applied.
> > +	 * @see struct rte_mtr_params::default_input_color
> > +	 * @see struct rte_mtr_params::vlan_table
> > +	 */
> > +	RTE_MTR_INPUT_COLOR_METHOD_VLAN = RTE_BIT64(1),
> 
> OK.
> Does your HW use PCP+DEI , or just PCP?
> 
> > +	/**
> > +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> > +	 * the outermost DSCP field indexing into the
> > +	 * struct rte_mtr_params::dscp_table.
> > +	 * Otherwise, the default_input_color is applied.
> > +	 * @see struct rte_mtr_params::default_input_color
> > +	 * @see struct rte_mtr_params::dscp_table
> > +	 */
> > +	RTE_MTR_INPUT_COLOR_METHOD_DSCP = RTE_BIT64(2),
> 
> OK.
> Please change name to METHOD_IP.
> Description: Change the "outermost DSCP" to "the DSCP field of the outermost
> IP header".
> I would move this up on the second position (to follow immediately after the
> color blind method).
> 
> > +	/**
> > +	 * If the input packet has at least one VLAN label, its input color is
> > +	 * detected by the outermost VLAN DEI(1bit), PCP(3 bits)
> > +	 * indexing into the struct rte_mtr_params::vlan_table.
> > +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> > +	 * the outermost DSCP field indexing into the
> > +	 * struct rte_mtr_params::dscp_table.
> > +	 * Otherwise, the default_input_color is applied.
> > +	 * @see struct rte_mtr_params::default_input_color
> > +	 * @see struct rte_mtr_params::vlan_table
> > +	 * @see struct rte_mtr_params::dscp_table
> > +	 */
> > +	RTE_MTR_INPUT_COLOR_METHOD_VLAN_DSCP = RTE_BIT64(3),
> 
> OK.
> Please change name to METHOD_VLAN_IP.
> This should follow immediately after the METHOD_VLAN.
> Description: please use "Otherwise" before "if the input packet is IP"; please
> replace "outermost DSCP" as above.
> Is your HW using DEI + PCP or just PCP?
> 
> > +	/**
> > +	 * If the input packet has at least one VLAN label, its input color is
> > +	 * detected by the innermost VLAN DEI(1bit), PCP(3 bits)
> > +	 * indexing into the struct rte_mtr_params::vlan_table.
> > +	 * Otherwise, the default_input_color is applied.
> > +	 * @see struct rte_mtr_params::default_input_color
> > +	 * @see struct rte_mtr_params::vlan_table
> > +	 */
> > +	RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN = RTE_BIT64(4),
> 
> OK.
> Is your HW using DEI + PCP or just PCP?
> 
> > +	/**
> > +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> > +	 * the innermost DSCP field indexing into the
> > +	 * struct rte_mtr_params::dscp_table.
> > +	 * Otherwise, the default_input_color is applied.
> > +	 * @see struct rte_mtr_params::default_input_color
> > +	 * @see struct rte_mtr_params::dscp_table
> > +	 */
> > +	RTE_MTR_INPUT_COLOR_METHOD_INNER_DSCP = RTE_BIT64(5),
> 
> This is very confusing to me, I don't get what this one is about: The "inner" word
> in the name suggests that inner VLAN is attempted first, then IP DSCP (if no
> VLAN is present), but the description only talks about IP.
> 
> > +	/**
> > +	 * If the input packet has at least one VLAN label, its input color is
> > +	 * detected by the innermost VLAN DEI(1bit), PCP(3 bits)
> > +	 * indexing into the struct rte_mtr_params::vlan_table.
> > +	 * If the input packet is IPv4 or IPv6, its input color is detected by
> > +	 * the innermost DSCP field indexing into the
> > +	 * struct rte_mtr_params::dscp_table.
> > +	 * Otherwise, the default_input_color is applied.
> > +	 * @see struct rte_mtr_params::default_input_color
> > +	 * @see struct rte_mtr_params::vlan_table
> > +	 * @see struct rte_mtr_params::dscp_table
> > +	 */
> > +	RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_DSCP =
> > RTE_BIT64(6),
> 
> OK.
> Description fixes: Use "otherwise" before "if IP"; replace innermost DSCP with
> "DSCP field of the outermost IP header".
> 
> <snip>
> 
> Regards,
> Cristian
  

0                    0             0                  0

0                    0             0                  0


On Tue, Mar 1, 2022 at 11:18 PM Dumitrescu, Cristian
<cristian.dumitrescu@intel.com> wrote:
>
> HI Jerin,

Hi Cristian,

>
> Thanks for your patch! I think we are making great progress, here are a few more comments:
>
> <snip>
>
> > +/**
> > + * Input color method
> > + */
> > +enum rte_mtr_input_color_method {
>
> We should clean up the names of these methods a bit: we should not mix header names (VLAN, IP) with header field names (DSCP, PCP), in the sense that to me METHOD_VLAN_DSCP should be replaced with either:
> * METHOD_OUTER_VLAN_IP :shorter name, as only the headers are mentioned (my preference, but I am OK with both)

OK, We will keep VLAN and IP. By default OUTER is implicit in other
DPDK API spec,i.e if not mentioned, it is outer. Hence I removed the
outer. I can add outer explicit if you think in that way. See last
comment.

> * METHOD_OUTER_VLAN_PCP_IP_DSCP: longer name, as both the headers and the header fields are mentioned
>
> Please put a blank line in between these methods to better readability.
>
> I see some issues in the list of methods below, I am trying to do my best to catch them all:

Thanks. Sorry for the delay in reply.


>
> > +     /**
> > +      * The input color is always green.
> > +      * The default_input_color is ignored for this method.
> > +      * @see struct rte_mtr_params::default_input_color
> > +      */
> > +     RTE_MTR_INPUT_COLOR_METHOD_COLOR_BLIND  = RTE_BIT64(0),
>
> OK.
>
> > +     /**
> > +      * If the input packet has at least one VLAN label, its input color is
> > +      * detected by the outermost VLAN DEI(1bit), PCP(3 bits)
> > +      * indexing into the struct rte_mtr_params::vlan_table.
> > +      * Otherwise, the default_input_color is applied.
> > +      * @see struct rte_mtr_params::default_input_color
> > +      * @see struct rte_mtr_params::vlan_table
> > +      */
> > +     RTE_MTR_INPUT_COLOR_METHOD_VLAN = RTE_BIT64(1),
>
> OK.
> Does your HW use PCP+DEI , or just PCP?

PCP + DEI

>
> > +     /**
> > +      * If the input packet is IPv4 or IPv6, its input color is detected by
> > +      * the outermost DSCP field indexing into the
> > +      * struct rte_mtr_params::dscp_table.
> > +      * Otherwise, the default_input_color is applied.
> > +      * @see struct rte_mtr_params::default_input_color
> > +      * @see struct rte_mtr_params::dscp_table
> > +      */
> > +     RTE_MTR_INPUT_COLOR_METHOD_DSCP = RTE_BIT64(2),
>
> OK.
> Please change name to METHOD_IP.
> Description: Change the "outermost DSCP" to "the DSCP field of the outermost IP header".

OK

> I would move this up on the second position (to follow immediately after the color blind method).

Please check the summary below.

>
> > +     /**
> > +      * If the input packet has at least one VLAN label, its input color is
> > +      * detected by the outermost VLAN DEI(1bit), PCP(3 bits)
> > +      * indexing into the struct rte_mtr_params::vlan_table.
> > +      * If the input packet is IPv4 or IPv6, its input color is detected by
> > +      * the outermost DSCP field indexing into the
> > +      * struct rte_mtr_params::dscp_table.
> > +      * Otherwise, the default_input_color is applied.
> > +      * @see struct rte_mtr_params::default_input_color
> > +      * @see struct rte_mtr_params::vlan_table
> > +      * @see struct rte_mtr_params::dscp_table
> > +      */
> > +     RTE_MTR_INPUT_COLOR_METHOD_VLAN_DSCP = RTE_BIT64(3),
>
> OK.
> Please change name to METHOD_VLAN_IP.

OK

> This should follow immediately after the METHOD_VLAN.

OK

> Description: please use "Otherwise" before "if the input packet is IP"; please replace "outermost DSCP" as above.

OK

> Is your HW using DEI + PCP or just PCP?

OK

>
> > +     /**
> > +      * If the input packet has at least one VLAN label, its input color is
> > +      * detected by the innermost VLAN DEI(1bit), PCP(3 bits)
> > +      * indexing into the struct rte_mtr_params::vlan_table.
> > +      * Otherwise, the default_input_color is applied.
> > +      * @see struct rte_mtr_params::default_input_color
> > +      * @see struct rte_mtr_params::vlan_table
> > +      */
> > +     RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN = RTE_BIT64(4),
>
> OK.
> Is your HW using DEI + PCP or just PCP?

DEI + PCP

>
> > +     /**
> > +      * If the input packet is IPv4 or IPv6, its input color is detected by
> > +      * the innermost DSCP field indexing into the
> > +      * struct rte_mtr_params::dscp_table.
> > +      * Otherwise, the default_input_color is applied.
> > +      * @see struct rte_mtr_params::default_input_color
> > +      * @see struct rte_mtr_params::dscp_table
> > +      */
> > +     RTE_MTR_INPUT_COLOR_METHOD_INNER_DSCP = RTE_BIT64(5),
>
> This is very confusing to me, I don't get what this one is about: The "inner" word in the name suggests that inner VLAN is attempted first, then IP DSCP (if no VLAN is present), but the description only talks about IP.

This case attempts only inner IP DSCP. VLAN does not matter.


>
> > +     /**
> > +      * If the input packet has at least one VLAN label, its input color is
> > +      * detected by the innermost VLAN DEI(1bit), PCP(3 bits)
> > +      * indexing into the struct rte_mtr_params::vlan_table.
> > +      * If the input packet is IPv4 or IPv6, its input color is detected by
> > +      * the innermost DSCP field indexing into the
> > +      * struct rte_mtr_params::dscp_table.
> > +      * Otherwise, the default_input_color is applied.
> > +      * @see struct rte_mtr_params::default_input_color
> > +      * @see struct rte_mtr_params::vlan_table
> > +      * @see struct rte_mtr_params::dscp_table
> > +      */
> > +     RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_DSCP =
> > RTE_BIT64(6),
>
> OK.
> Description fixes: Use "otherwise" before "if IP"; replace innermost DSCP with "DSCP field of the outermost IP header".

OK.

To summarize we have 4 attributes, Please find below the truth table
1) Outer VLAN
2) Outer IP
3) Inner VLAN
4) Inner IP


Inner IP -Inner VLAN- Outer IP-Outer VLAN
0                    0             0                  0
- Not valid case
0                    0             0                  1
- RTE_MTR_INPUT_COLOR_METHOD_OUTER_VLAN
0                    0             1                  0
- RTE_MTR_INPUT_COLOR_METHOD_OUTER_IP
0                    0             1                  1
- RTE_MTR_INPUT_COLOR_METHOD_OUTER_VLAN_OUTER_IP - If found outer VLAN
then vlan else outer IP
0                    1             0                  0
- RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN
0                    1             0                  1
- RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_VLAN - If found inner
VLAN else outer VLAN
0                    1             1                  0              -
 RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_IP
0                    1             1                  1              -
 RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_IP_OUTER_VLAN - If found
inner vlan then inner vlan else outer IP else outer VLAN
1                    0             0                  0              -
 RTE_MTR_INPUT_COLOR_METHOD_INNER_IP
1                    0             0                  1              -
 RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_VLAN
1                    0             1                  0              -
 RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_IP
1                    0             1                  1              -
 RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_IP_OUTER_VLAN
1                    1             0                  0              -
 RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN
1                    1             0                  1              -
 RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_VLAN
1                    1             1                  0              -
 RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_IP
1                    1             1                  1              -
 RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_IP_OUTER_VLAN

Is this above enumeration fine, If not, Please suggest.

In Interms of name,
a) We could omit explicit OUTER to reduce the length as suggestion.
b) or change IIP, OIP, IVLAN, OVLAN kind of scheme to reduce the name.

Let me know the names and enumeration you prefer, I will change
accordingly in the next version?



>
> <snip>
>
> Regards,
> Cristian
  

> 
> To summarize we have 4 attributes, Please find below the truth table
> 1) Outer VLAN
> 2) Outer IP
> 3) Inner VLAN
> 4) Inner IP
> 
> 
> Inner IP -Inner VLAN- Outer IP-Outer VLAN
> 0                    0             0                  0
> - Not valid case
> 0                    0             0                  1
> - RTE_MTR_INPUT_COLOR_METHOD_OUTER_VLAN
> 0                    0             1                  0
> - RTE_MTR_INPUT_COLOR_METHOD_OUTER_IP
> 0                    0             1                  1
> - RTE_MTR_INPUT_COLOR_METHOD_OUTER_VLAN_OUTER_IP - If found outer
> VLAN
> then vlan else outer IP
> 0                    1             0                  0
> - RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN
> 0                    1             0                  1
> - RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_VLAN - If found
> inner
> VLAN else outer VLAN
> 0                    1             1                  0              -
>  RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_IP
> 0                    1             1                  1              -
>  RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_IP_OUTER_VLAN -
> If found
> inner vlan then inner vlan else outer IP else outer VLAN
> 1                    0             0                  0              -
>  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP
> 1                    0             0                  1              -
>  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_VLAN
> 1                    0             1                  0              -
>  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_IP
> 1                    0             1                  1              -
>  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_IP_OUTER_VLAN
> 1                    1             0                  0              -
>  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN
> 1                    1             0                  1              -
>  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_VLAN
> 1                    1             1                  0              -
>  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_IP
> 1                    1             1                  1              -
> 
> RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_IP_OUTE
> R_VLAN
> 
> Is this above enumeration fine, If not, Please suggest.
> 
> In Interms of name,
> a) We could omit explicit OUTER to reduce the length as suggestion.
> b) or change IIP, OIP, IVLAN, OVLAN kind of scheme to reduce the name.
> 
> Let me know the names and enumeration you prefer, I will change
> accordingly in the next version?
> 

Hi Jerin,

The above table looks confusing to me, I suggest we have a meeting next week to go over it and then report back to the list?

Regards,
Cristian
  

On Thu, Apr 7, 2022 at 6:55 PM Dumitrescu, Cristian
<cristian.dumitrescu@intel.com> wrote:
>
> >
> > To summarize we have 4 attributes, Please find below the truth table
> > 1) Outer VLAN
> > 2) Outer IP
> > 3) Inner VLAN
> > 4) Inner IP
> >
> >
> > Inner IP -Inner VLAN- Outer IP-Outer VLAN
> > 0                    0             0                  0
> > - Not valid case
> > 0                    0             0                  1
> > - RTE_MTR_INPUT_COLOR_METHOD_OUTER_VLAN
> > 0                    0             1                  0
> > - RTE_MTR_INPUT_COLOR_METHOD_OUTER_IP
> > 0                    0             1                  1
> > - RTE_MTR_INPUT_COLOR_METHOD_OUTER_VLAN_OUTER_IP - If found outer
> > VLAN
> > then vlan else outer IP
> > 0                    1             0                  0
> > - RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN
> > 0                    1             0                  1
> > - RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_VLAN - If found
> > inner
> > VLAN else outer VLAN
> > 0                    1             1                  0              -
> >  RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_IP
> > 0                    1             1                  1              -
> >  RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_IP_OUTER_VLAN -
> > If found
> > inner vlan then inner vlan else outer IP else outer VLAN
> > 1                    0             0                  0              -
> >  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP
> > 1                    0             0                  1              -
> >  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_VLAN
> > 1                    0             1                  0              -
> >  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_IP
> > 1                    0             1                  1              -
> >  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_IP_OUTER_VLAN
> > 1                    1             0                  0              -
> >  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN
> > 1                    1             0                  1              -
> >  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_VLAN
> > 1                    1             1                  0              -
> >  RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_IP
> > 1                    1             1                  1              -
> >
> > RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_IP_OUTE
> > R_VLAN
> >
> > Is this above enumeration fine, If not, Please suggest.
> >
> > In Interms of name,
> > a) We could omit explicit OUTER to reduce the length as suggestion.
> > b) or change IIP, OIP, IVLAN, OVLAN kind of scheme to reduce the name.
> >
> > Let me know the names and enumeration you prefer, I will change
> > accordingly in the next version?
> >
>
> Hi Jerin,
>
> The above table looks confusing to me, I suggest we have a meeting next week to go over it and then report back to the list?

Scheduled a meeting at 11th April - 6:30 IST


Agenda:
To discuss https://patches.dpdk.org/project/dpdk/patch/20220301085824.1041009-1-skori@marvell.com/
patch


Let me know, If time needs to change to include any interested participants.


Hi there,

Jerin Jacob is inviting you to a scheduled Zoom meeting.

Topic: Jerin Jacob Kollanukkaran's Personal Meeting Room


Join Zoom Meeting:
https://marvell.zoom.us/j/9901077677?pwd=T2lTTGMwYlc1YTQzMnR4eGRWQXR6QT09
    Password: 339888


Or Telephone：
    Dial(for higher quality, dial a number based on your current location)：
        US: +1 301 715 8592  or +1 312 626 6799  or +1 346 248 7799
or +1 646 558 8656  or +1 669 900 6833  or +1 253 215 8782  or 888 788
0099 (Toll Free) or 833 548 0276 (Toll Free) or 833 548 0282 (Toll
Free) or 877 853 5247 (Toll Free)
    Meeting ID: 990 107 7677
    Password: 358309
    International numbers available: https://marvell.zoom.us/u/adpcCpMHYt

Or a Video Conference Room:
From Touchpad: Tap Join Zoom button. When prompted, enter 990 107 7677
Password: 358309

For China locations, from Touchpad: Dial* then 990 107 7677
    Password: 358309

>
> Regards,
> Cristian
  

Hi folks,

Thanks to the community colleagues that participated to the call earlier today. Apologies for not CC-ing all the 9 attendants, as I don't have the email address for all of them.

We had a good meeting, I think we agreed on a good solution that will also simplify the API proposal.

Recap on the problem statement:
1. How do we decide on the input color for the current packet in a deterministic (implementation independent) way?
2. Multiple choices possible for a given packet: The same packet might contain multiple headers that can provide the input color. Examples: VLAN (the PCP and DEI fields), IP (DSCP field), others. Some headers of same type may show up in the same packet (e.g. outer/inner header).
3. Some of the possible choices might not be available for a given packet: Different packets may contain different headers.

Proposed solution:
1. Configure which protocols to enable. Example: we might want to consider Outer VLAN and Outer IP, but disable Inner VLAN and inner IP.
2. Configure on the priority of each of the enabled protocols (0 = highest priority). Example: If the current packet has an Outer VLAN header, but not an Outer IP header, then we get the input color from the Outer VLAN header; same, if there is an Outer IP header, but no Outer VLAN header, we get the priority from the Outer IP header. But what if the packet has both an Outer VLAN and an Outer IP header? In this case we need to consider the priority and pick the input color from the highest priority header.
3. Configure the default input color. What happens if the packet does not have an Outer VLAN header, nor an Outer IP header? Then the default input color is picked.

API guideline:
enum rte_mtr_color_in_protocol {
	RTE_MTR_COLOR_IN_OUTER_VLAN,
	RTE_MTR_COLOR_IN_INNER_VLAN,
	RTE_MTR_COLOR_IN_OUTER_IP,
	RTE_MTR_COLOR_IN_INNER_IP,
	//more to add as needed. Per Ori's comment, we can add a reunion of the two protocols as well when needed, e.g. RTE_MTR_COLOR_IN_OUTER_VLAN_OUTER_IP that has a (4+6)-bit index (PCI, DEI, DSCP).
}

int
rte_mtr_color_in_protocol_priority_set(enum rte_mtr_color_in_protocol proto, uint32_t priority); //0 is highest priority

Regards,
Cristian
  

Adding some missing guys, that I know about.

Ori



> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Sent: Monday, April 11, 2022 5:46 PM
> To: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>; Ray Kinsella <mdr@ashroe.eu>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; Jerin Jacob <jerinj@marvell.com>; skori@marvell.com; Ori
> Kam <orika@nvidia.com>
> Subject: RE: [PATCH v3 1/1] ethdev: mtr: support input color selection
> 
> Hi folks,
> 
> Thanks to the community colleagues that participated to the call earlier today. Apologies for not CC-ing
> all the 9 attendants, as I don't have the email address for all of them.
> 
> We had a good meeting, I think we agreed on a good solution that will also simplify the API proposal.
> 
> Recap on the problem statement:
> 1. How do we decide on the input color for the current packet in a deterministic (implementation
> independent) way?
> 2. Multiple choices possible for a given packet: The same packet might contain multiple headers that
> can provide the input color. Examples: VLAN (the PCP and DEI fields), IP (DSCP field), others. Some
> headers of same type may show up in the same packet (e.g. outer/inner header).
> 3. Some of the possible choices might not be available for a given packet: Different packets may
> contain different headers.
> 
> Proposed solution:
> 1. Configure which protocols to enable. Example: we might want to consider Outer VLAN and Outer IP,
> but disable Inner VLAN and inner IP.
> 2. Configure on the priority of each of the enabled protocols (0 = highest priority). Example: If the
> current packet has an Outer VLAN header, but not an Outer IP header, then we get the input color
> from the Outer VLAN header; same, if there is an Outer IP header, but no Outer VLAN header, we get
> the priority from the Outer IP header. But what if the packet has both an Outer VLAN and an Outer IP
> header? In this case we need to consider the priority and pick the input color from the highest priority
> header.
> 3. Configure the default input color. What happens if the packet does not have an Outer VLAN header,
> nor an Outer IP header? Then the default input color is picked.
> 
> API guideline:
> enum rte_mtr_color_in_protocol {
> 	RTE_MTR_COLOR_IN_OUTER_VLAN,
> 	RTE_MTR_COLOR_IN_INNER_VLAN,
> 	RTE_MTR_COLOR_IN_OUTER_IP,
> 	RTE_MTR_COLOR_IN_INNER_IP,
> 	//more to add as needed. Per Ori's comment, we can add a reunion of the two protocols as
> well when needed, e.g. RTE_MTR_COLOR_IN_OUTER_VLAN_OUTER_IP that has a (4+6)-bit index
> (PCI, DEI, DSCP).
> }
> 
> int
> rte_mtr_color_in_protocol_priority_set(enum rte_mtr_color_in_protocol proto, uint32_t priority);
> //0 is highest priority
> 
> Regards,
> Cristian