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

Dumitrescu, Cristian cristian.dumitrescu at intel.com
Wed Apr 14 18:16:19 CEST 2021


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


More information about the dev mailing list