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

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

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