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

[Jerin Jacob <jerinjacobk@gmail.com>]

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.
>
>
> >
> > 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
>
> >
> > 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.
>
>
> >
> > 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.
>
>
>
> >
> > Regards,
> > Cristian