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

[Li Zhang <lizh@nvidia.com>]

Thanks Jasvinder.
Will change it in V7 patch.

Regards,
Li Zhang

> -----Original Message-----
> From: Singh, Jasvinder <jasvinder.singh@intel.com>
> Sent: Thursday, April 15, 2021 6:22 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>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.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>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ray Kinsella <mdr@ashroe.eu>; Neil
> Horman <nhorman@tuxdriver.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
> 
> 
> <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");
> 
> Add check for "policy",  could be null, before dereferencing this.
> 
> 
> <snip>
> >
> > -/* MTR object policer action update */
> > +/* MTR object policy update */
> >  static int
> > -pmd_mtr_policer_actions_update(struct rte_eth_dev *dev,
> > +pmd_mtr_meter_policy_update(struct rte_eth_dev *dev,
> >       uint32_t mtr_id,
> > -     uint32_t action_mask,
> > -     enum rte_mtr_policer_action *actions,
> > +     uint32_t meter_policy_id,
> >       struct rte_mtr_error *error)
> >  {
> >       struct pmd_internals *p = dev->data->dev_private;
> >       struct softnic_mtr *m;
> >       uint32_t i;
> >       int status;
> > +     struct softnic_mtr_meter_policy *mp_new, *mp_old;
> >
> >       /* MTR object id must be valid */
> >       m = softnic_mtr_find(p, mtr_id); @@ -527,29 +652,14 @@
> > pmd_mtr_policer_actions_update(struct
> > rte_eth_dev *dev,
> >                       RTE_MTR_ERROR_TYPE_MTR_ID,
> >                       NULL,
> >                       "MTR object id not valid");
> > -
> > -     /* Valid policer actions */
> > -     if (actions == NULL)
> > +     /* Meter policy must exist */
> > +     mp_new = softnic_mtr_meter_policy_find(p, meter_policy_id);
> > +     if (mp_new == NULL)
> >               return -rte_mtr_error_set(error,
> >                       EINVAL,
> > -                     RTE_MTR_ERROR_TYPE_UNSPECIFIED,
> > +                     RTE_MTR_ERROR_TYPE_METER_POLICY_ID,
> >                       NULL,
> > -                     "Invalid actions");
> > -
> > -     for (i = 0; i < RTE_COLORS; i++) {
> > -             if (action_mask & (1 << i)) {
> > -                     if (actions[i] !=
> > MTR_POLICER_ACTION_COLOR_GREEN  &&
> > -                             actions[i] !=
> > MTR_POLICER_ACTION_COLOR_YELLOW &&
> > -                             actions[i] !=
> > MTR_POLICER_ACTION_COLOR_RED &&
> > -                             actions[i] != MTR_POLICER_ACTION_DROP) {
> > -                             return -rte_mtr_error_set(error,
> > -                                     EINVAL,
> > -
> >       RTE_MTR_ERROR_TYPE_UNSPECIFIED,
> > -                                     NULL,
> > -                                     " Invalid action value");
> > -                     }
> > -             }
> > -     }
> > +                     "Meter policy id invalid");
> 
> Please add check whether MTR object is already set to meter policy  id,
> return success if true, no need to continue.
> if (m->params.meter_policy_id == meter_policy_id)
>                 return 0;
> 
> >       /* MTR object owner valid? */
> >       if (m->flow) {
> 
> 
> 
> Regards,
> Jasvinder