Hi Jerin and folks, any update on this one? Thanks, Cristian
> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitre...@intel.com>
> Sent: Tuesday, March 1, 2022 5:48 PM
> To: sk...@marvell.com; Thomas Monjalon <tho...@monjalon.net>; Yigit,
> Ferruh <ferruh.yi...@intel.com>; Andrew Rybchenko
> <andrew.rybche...@oktetlabs.ru>; Ray Kinsella <m...@ashroe.eu>
> Cc: dev@dpdk.org; Jerin Jacob <jer...@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
