Re: [dpdk-dev] [PATCH v6 1/2] ethdev: add pre-defined meter policy API

[Li Zhang <lizh@nvidia.com>]

Thanks Cristian.
Will change it in V7 patch.

Regards,
Li Zhang

> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Sent: Thursday, April 15, 2021 12:16 AM
> To: Li Zhang <lizh@nvidia.com>; dekelp@nvidia.com; Ori Kam
> <orika@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Matan
> Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
> lironh@marvell.com; jerinj@marvell.com; Yigit, Ferruh
> <ferruh.yigit@intel.com>; ajit.khaparde@broadcom.com; Wisam Monther
> <wisamm@nvidia.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Singh,
> Jasvinder <jasvinder.singh@intel.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ray Kinsella <mdr@ashroe.eu>; Neil
> Horman <nhorman@tuxdriver.com>; Jerin Jacob <jerinjacobk@gmail.com>;
> Hemant Agrawal <hemant.agrawal@nxp.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Roni Bar Yanai
> <roniba@nvidia.com>; Haifei Luo <haifeil@nvidia.com>; Jiawei(Jonny) Wang
> <jiaweiw@nvidia.com>
> Subject: RE: [PATCH v6 1/2] ethdev: add pre-defined meter policy API
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Li,
> 
> Following the API changes, there are lots of changes in the drivers, as
> expected, so we'll have to take the necessary time to review them.
> 
> Here are just a few comments below, please expect more during the next
> few days.
> 
> <snip>
> 
> > +
> > +/* MTR meter policy add */
> > +static int
> > +pmd_mtr_meter_policy_add(struct rte_eth_dev *dev,
> > +     uint32_t meter_policy_id,
> > +     struct rte_mtr_meter_policy_params *policy,
> > +     struct rte_mtr_error *error)
> > +{
> > +     struct pmd_internals *p = dev->data->dev_private;
> > +     struct softnic_mtr_meter_policy_list *mpl = &p-
> > >mtr.meter_policies;
> > +     struct softnic_mtr_meter_policy *mp;
> > +     const struct rte_flow_action *act;
> > +     const struct rte_flow_action_meter_color *recolor;
> > +     uint32_t i;
> > +
> > +     /* Meter policy ID must be valid. */
> > +     if (meter_policy_id == UINT32_MAX)
> > +             return -rte_mtr_error_set(error,
> > +                     EINVAL,
> > +                     RTE_MTR_ERROR_TYPE_METER_POLICY_ID,
> > +                     NULL,
> > +                     "Meter policy id not valid");
> 
> This is obviously not correct, we need to check whether the meter_policy_id
> provided by the user is already in use (by a policy previously added) or not.
> You can do this with the policy find function that you have already
> implemented.
> 
> > +
> > +     for (i = 0; i < RTE_COLORS; i++) {
> > +             act = policy->actions[i];
> > +             if (act && act->type !=
> > RTE_FLOW_ACTION_TYPE_METER_COLOR &&
> > +                     act->type != RTE_FLOW_ACTION_TYPE_DROP)
> > +                     return -rte_mtr_error_set(error,
> > +                             EINVAL,
> > +                             RTE_MTR_ERROR_TYPE_METER_POLICY,
> > +                             NULL,
> > +                             "Action invalid");
> > +     }
> 
> This check does not look right either: obviously we cannot accept a null
> action list for any color, plus the action list should contain only those action
> types we support (RTE_FLOW_ACTION_TYPE_METER_COLOR or
> RTE_FLOW_ACTION_TYPE_DROP).
> 
> I agree, fist you need to check that the linked list of policy actions for each
> color is non-NULL, then you need to traverse it until you meet the END
> action, skip any PAD actions, then check that exactly one (and one only) of
> METER_COLOR or DROP exist, but not both at the same time, and also we
> don't have the same action showing up multiple times. Makes sense?
> 
> Regards,
> Cristian