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

[Jerin Jacob <jerinjacobk@gmail.com>]

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