On Tue, Dec 7, 2021 at 11:52 PM Dumitrescu, Cristian
<cristian.dumitre...@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
