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

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

HI Jerin,

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

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, December 7, 2021 9:55 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: jerinj@marvell.com; Thomas Monjalon <thomas@monjalon.net>; Yigit,
> Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org;
> arybchenko@solarflare.com; lizh@nvidia.com;
> ajit.khaparde@broadcom.com; Singh, Jasvinder
> <jasvinder.singh@intel.com>; matan@nvidia.com;
> ndabilpuram@marvell.com; skori@marvell.com; rkudurumalla@marvell.com
> Subject: Re: [dpdk-dev] [RFC PATCH] ethdev: mtr: enhance input color table
> features
> 
> On Wed, Nov 17, 2021 at 5:30 PM Jerin Jacob <jerinjacobk@gmail.com>
> wrote:
> >
> > On Mon, Oct 11, 2021 at 8:53 PM Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com> wrote:
> > >
> > > Hi Jerin,
> >
> Hi Cristian,
> 
> Could you share your feedback so that I can send the v1?
> 
> 
> >
> >
> > >
> > > > -----Original Message-----
> > > > From: jerinj@marvell.com <jerinj@marvell.com>
> > > > Sent: Friday, August 20, 2021 9:24 AM
> > > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Thomas
> Monjalon
> > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Andrew
> > > > Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > > > Cc: dev@dpdk.org; arybchenko@solarflare.com; lizh@nvidia.com;
> > > > ajit.khaparde@broadcom.com; Singh, Jasvinder
> > > > <jasvinder.singh@intel.com>; matan@nvidia.com;
> > > > ndabilpuram@marvell.com; skori@marvell.com;
> rkudurumalla@marvell.com;
> > > > Jerin Jacob <jerinj@marvell.com>
> > > > Subject: [dpdk-dev] [RFC PATCH] ethdev: mtr: enhance input color
> table
> > > > features
> > > >
> > > > From: Jerin Jacob <jerinj@marvell.com>
> > > >
> > > > Currently, meter object supports only DSCP based on input color table,
> > > > The patch enhance that to support VLAN based input color table,
> > > > color table based on inner field for the tunnel use case,  and support
> > > > for fallback color per meter if packet based on a different field.
> > > >
> > > > All of the above features are exposed through capability and added
> > > > additional capability to specify the implementation supports
> > > > more than one input color table per ethdev port.
> > > >
> > > > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> >
> > > > 2.33.0
> > >
> > > Allowing the configuration of the packet input color based on multiple
> protocols as opposed to just IP DSCP field makes sense to me.
> > >
> >
> > Apologies for the delay in response. This was missed the 21.11
> > timeframe so I don't bother replying. Reviving back.
> >
> > > A few questions/suggestions:
> > >
> > > 1. How do we decide which field/protocol takes precedence to define the
> packet input color? You are enabling 2 possible options so far, i.e. VLAN tag
> PCP field and IP DSCP field, which one is to be set for the current meter
> object? Using the capabilities to  decide is confusing, as a specific meter
> object might be able to support multiple or even all the possible options (e.g.
> the meter object is fine with either IP DSCP or VLAN PCP). Therefore, we
> need  a clear mechanism to unequivocally pick one; I think the user must
> explicitly define which input color methodology is to be used by explicitly
> setting a field (to be added) in the meter object parameter structure.
> >
> > Currently, in our HW, Each meter object needs to tell which pre-color
> > scheme it is going to use(IP DSCP or VLAN PCP). This is OK in the
> > overall scheme of things as the meter object is connected to rte_flow.
> > So,
> > rte_flow pattern can decide the steering to meter. Having said that,
> > it is possible to have array for input color as you suggested. If
> > choose that, path, We need to add capability tell implementation
> > supports only one input color per meter object. Let me know if this is
> > OK.
> >

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?

> >
> > >
> > > 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