Re: [dpdk-dev] [RFC PATCH] ethdev: mtr: enhance input color table features

[Jerin Jacob <jerinjacobk@gmail.com>]

On Tue, Dec 7, 2021 at 11:52 PM Dumitrescu, Cristian
<cristian.dumitrescu@intel.com> wrote:
>
> HI Jerin,

Hi Chistian,


>
> Sorry for my delay. I am currently in vacation until the beginning on January 2022, so my response is slower than usual.

I was too on vacation.

>
> Thanks for the explanation of the capabilities you are trying to enable in the API. Based on this, I am reconsidering some of my previous input. Here are my current thoughts:
>
> a) Each meter object can be configured with multiple methods to determine the input color: IP DSCP and VLAN PCP are just two of them, others are possible (as also listed by you). I think the problem we are trying to solve here is: in case multiple such methods are enabled for a given meter object AND more than one enabled method is applicable for a particular input packet at run-time (e.g. the packet is an IP packet, but it also contains at least one VLAN label, and both IP DSCP and the VLAN PCP methods are enabled for the meter object), how do we decide which method is to be applied? So, IMO we need to define a priority scheme that always picks a single method:
>
>         - a unique priority for each method;
>
>         - a default method to be used when none of the enabled methods is applicable to the input packet.
>
> b) The default method must be usable even when the input packet type is unknown to the HW (unsupported), e.g. we should still be able to decide the input color of a packet that is not an IP packet, such as ARP, and it does not contain any VLAN labels. For this, I suggest we add a field ins struct rte_mtr_params called default_input_color of type enum rte_color:
>
>         struct rte_mtr_params {
>                 enum rte_color default_input_color; /* Input color to be set for the input packet when none of the enabled input color methods is applicable to the current input packet. Ignored when this method is set to the color blind method. */
>         };
>
> c) In terms of methods and their priority, I propose to start with the following options, each of which referring to a set of methods, with a clear way to select a unique method. This is the minimal set needed to support the HW capabilities that you mention, this set can be extended as needed in the future:
>
>         enum rte_mtr_input_color_method {
>                 RTE_MTR_INPUT_COLOR_METHOD_COLOR_BLIND, /* The input color is always green. The default_input_color is ignored for this method. */
>                 RTE_ MTR_INPUT_COLOR_METHOD_IP_DSCP, /* If the input packet is IPv4 or IPv6, its input color is detected  by the DSCP field indexing into the dscp_table. Otherwise, the default_input_color is applied. */
>                 RTE_ MTR_INPUT_COLOR_METHOD_OUTER_VLAN_PCP, /* If the input packet has at least one VLAN label, its input color is detected by the outermost VLAN PCP indexing into the vlan_table. Otherwise, the default_input_color is applied. */
>                 RTE_ MTR_INPUT_COLOR_METHOD_OUTER_VLAN_PCP_IP_DSCP, /* If the input packet has at least one VLAN label, its input color is detected by the outermost VLAN PCP indexing into the vlan_table. Otherwise, if the packet is IPv4 or IPv6, its input color is detected by the DSCP field indexing into the dscp_table. Otherwise, the default_input_color is applied. */
>                 RTE_ MTR_INPUT_COLOR_METHOD_INNER_VLAN_PCP, /* If the input packet has at least one VLAN label, its input color is detected by the innermost VLAN PCP indexing into the vlan_table. Otherwise, the default_input_color is applied. */
>                 RTE_ MTR_INPUT_COLOR_METHOD_INNER_VLAN_PCP_IP_DSCP, /* If the input packet has at least one VLAN label, its input color is detected by the innermost VLAN PCP indexing into the vlan_table. Otherwise, if the packet is IPv4 or IPv6, its input color is detected by the DSCP field indexing into the dscp_table. Otherwise, the default_input_color is applied. */
>         };
>
>         struct rte_mtr_params {
>                 enum rte_mtr_input_color_method input_color_method;
>         };
>
> d) The above point means that all the protocol dependent tables are independent of each other, so they can all exist at the same time, so we cannot put all of them into a union or a single unified input color translation table. In case any of these tables is enabled/required by the input color scheme, but it is set to NULL, it must automatically resolve to an "all-green" table (as it is already the case for the existing dscp_table).
>
>         struct rte_mtr_params {
>                 enum rte_color *ip_dscp_table; /* Used when the IP DSCP input color method is selected for the current input packet. If set to NULL, it defaults to 64 elements of RTE_COLOR_GREEN. */
>                 enum rte_color *vlan_table; /* Used when the outermost/innermost VLAN PCP input color method is selected for the current input packet. If set to NULL, it defaults to all elements being RTE_COLOR_GREEN. */
>         };
>
> So many details to address in order to avoid any loopholes in this API, but I think this scheme is implementable and robust enough. What do you think?


Sounds good. I will next version based on your suggestion.




>
> > >
> > > >
> > > > 2. What if the defined input color methodology is not applicable to the
> > current input packet? For example, the user selects VLAN PCP, but some or
> > all of the input packets do not contain any VLAN labels?
> > >
> > > it picks the rte_mtr_params::fallback_input_color
> > >
>
> We likely need a more complex scheme, see above ;)
>
> > > >
> > > > 3. How do we manage the many packet fields that could be used as the
> > key for the input color: outer IP DSCP, inner IP DSCP, VLAN 1st label, VLAN
> > 2nd label, MPLS QoS, etc?
> > > > - Approach A: Use an enumeration, like you propose, and we can add
> > more in the future. Not ideal.
> > > > - Approach B: Point to a protocol-agnostic packet field by defining the
> > offset and mask of a 64-bit field. Advantage: we don't need to change the
> > API every time we add a new option.
> > >
> > > No strong opinion on doing Approach B. I think, it may be overkill for
> > > application and implementation to express. No strong opinion, If you
> > > have a strong opinion on that, I will change that to v1. Let me know.
> > >
>
> I would love to have a protocol agnostic scheme, but I am OK to start with a simple scheme for now (if you can call the above scheme as being simple ;) )and revisit later on if needed.
>
> > >
> > > >
> > > > 4. I suggest we use a unified input color table as opposed to one per
> > protocol, i.e. rename the struct rte_mtr_params::dscp_table to
> > color_in_table. The size of this table could be implicitly defined by the packet
> > field type (in case of enum) or the field mask (in case of protocol-agnostic
> > field offset and mask), as described above.
> > >
> > > Will do that. I thought not to do that just because, I don't want to
> > > remove the existing dscp_table symbol.
> > > Make sense to enable your suggestion.
> > >
> > > Let me know your views on Questions 1 and 3. I will send the next
> > > version based on that.
> > >
>
> Based on the above comments, I no longer thing a unified such table would work.
>
> > >
> > >
> > > >
> > > > Regards,
> > > > Cristian
>
> Regards,
> Cristian