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

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

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