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

[Jiawei(Jonny) Wang]

Thanks Jasvinder's comments!
Li is on vacation so I help to update the code changes based on your comments, V9 is sent with your ack.

> -----Original Message-----
> From: Singh, Jasvinder <jasvinder.singh at intel.com>
> Sent: Monday, April 19, 2021 8:35 PM
> To: Li Zhang <lizh at nvidia.com>; dekelp at nvidia.com; Ori Kam
> <orika at nvidia.com>; Slava Ovsiienko <viacheslavo at nvidia.com>; Matan
> Azrad <matan at nvidia.com>; Shahaf Shuler <shahafs at nvidia.com>;
> Dumitrescu, Cristian <cristian.dumitrescu at intel.com>; lironh at marvell.com;
> jerinj at marvell.com; Yigit, Ferruh <ferruh.yigit at intel.com>;
> ajit.khaparde at broadcom.com; Wisam Monther <wisamm at nvidia.com>; Li,
> Xiaoyun <xiaoyun.li at intel.com>; NBU-Contact-Thomas Monjalon
> <thomas at monjalon.net>; Andrew Rybchenko
> <andrew.rybchenko at oktetlabs.ru>; Ray Kinsella <mdr at ashroe.eu>; Neil
> Horman <nhorman at tuxdriver.com>
> Cc: dev at dpdk.org; Raslan Darawsheh <rasland at nvidia.com>; Roni Bar Yanai
> <roniba at nvidia.com>; Haifei Luo <haifeil at nvidia.com>; Jiawei(Jonny) Wang
> <jiaweiw at nvidia.com>
> Subject: RE: [PATCH v8 1/2] ethdev: add pre-defined meter policy API
> 
> <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;
> > +	bool valid_act_found;
> > +
> > +	if (policy == NULL)
> > +		return -rte_mtr_error_set(error,
> > +			EINVAL,
> > +			RTE_MTR_ERROR_TYPE_METER_POLICY,
> > +			NULL,
> > +			"Null meter policy invalid");
> > +
> > +	/* Meter policy must not exist. */
> > +	mp = softnic_mtr_meter_policy_find(p, meter_policy_id);
> > +	if (mp != NULL)
> > +		return -rte_mtr_error_set(error,
> > +			EINVAL,
> > +			RTE_MTR_ERROR_TYPE_METER_POLICY_ID,
> > +			NULL,
> > +			"Meter policy already exists");
> > +
> > +	for (i = 0; i < RTE_COLORS; i++) {
> > +		if (policy->actions[i] == NULL)
> > +			return -rte_mtr_error_set(error,
> > +				EINVAL,
> > +				RTE_MTR_ERROR_TYPE_METER_POLICY,
> > +				NULL,
> > +				"Null action list");
> > +		for (act = policy->actions[i], valid_act_found = false;
> > +			act && act->type != RTE_FLOW_ACTION_TYPE_END;
> > +			act++) {
> 
> Hi Li,
> No need to check "act" in for loop instruction as it is validated before the for
> loop.  Also, would be good to skip all the steps inside this loop for the actions
> like RTE_FLOW_ACTION_TYPE_VOID. After for loop, will be good to check
> "valid_act_found" is true else return an error for that color action.
> 
> Rest seems good to me
> 
> With above fix for softnic-
> Acked-by: Jasvinder Singh <jasvinder.singh at intel.com>
> 
>