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

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

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