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
