Re: [PATCH v9-S2 05/13] net/dpaa2: support meter and policing

[Stephen Hemminger <stephen@networkplumber.org>]

There is a race here.

> static int
> +dpaa2_mtr_meter_create(struct rte_eth_dev *dev,
> +	uint32_t mtr_id, struct rte_mtr_params *params,
> +	int shared, struct rte_mtr_error *error)
> +{
> +	struct dpaa2_dev_priv *priv = dev->data->dev_private;
> +	struct dpaa2_dev_meter_profile *profile;
> +	struct dpaa2_dev_meter_policy *policy;
> +	struct dpaa2_dev_meter *meter, *curr;
> +	uint32_t profile_id, policy_id;
> +	enum rte_mtr_error_type err_type = RTE_MTR_ERROR_TYPE_NONE;
> +	bool found = false;
> +	int ret = 0;
> +

...

> +	rte_spinlock_lock(&priv->meter_lock);
> +	profile = LIST_FIRST(&priv->profiles);
> +	while (profile != NULL) {
> +		if (profile->profile_id == profile_id) {
> +			found = true;
> +			break;
> +		}
> +		profile = LIST_NEXT(profile, next);
> +	}
> +	if (!found) {
> +		snprintf(s_err_msg, sizeof(s_err_msg),
> +			"Meter profile ID(%d) not exist!", profile_id);
> +		ret = ENOENT;
> +		err_type = RTE_MTR_ERROR_TYPE_METER_PROFILE_ID;
> +		goto quit;
> +	}
> +
> +	found = false;
> +	policy = LIST_FIRST(&priv->policies);
> +	while (policy != NULL) {
> +		if (policy->policy_id == policy_id) {
> +			found = true;
> +			break;
> +		}
> +		policy = LIST_NEXT(policy, next);
> +	}
> +	if (!found) {
> +		snprintf(s_err_msg, sizeof(s_err_msg),
> +			"Meter policy ID(%d) not exist!", policy_id);
> +		ret = ENOENT;
> +		err_type = RTE_MTR_ERROR_TYPE_METER_POLICY_ID;
> +		goto quit;
> +	}
> +	meter = LIST_FIRST(&priv->meters);
> +	while (meter != NULL) {
> +		if (meter->meter_id == mtr_id) {
> +			snprintf(s_err_msg, sizeof(s_err_msg),
> +				"Meter ID(%d) exist!", mtr_id);
> +			ret = EEXIST;
> +			err_type = RTE_MTR_ERROR_TYPE_MTR_ID;
> +			goto quit;
> +		}
> +		meter = LIST_NEXT(meter, next);
> +	}
> +	meter = rte_zmalloc(NULL, sizeof(struct dpaa2_dev_meter), 0);
> +	if (meter == NULL) {
> +		snprintf(s_err_msg, sizeof(s_err_msg),
> +			"Meter memory alloc failed!");
> +		ret = ENOMEM;
> +		err_type = RTE_MTR_ERROR_TYPE_UNSPECIFIED;
> +		goto quit;
> +	}
> +	meter->meter_id = mtr_id;
> +	meter->profile_id = profile_id;
> +	meter->policy_id = policy_id;
> +
> +	rte_spinlock_unlock(&priv->meter_lock);

At this point meter is created but not put into any list.

> +
> +	/* Program the hardware policing configuration for this TC. */
> +	ret = dpaa2_mtr_hw_program(dev, mtr_id, profile, policy);
> +	if (ret) {
> +		rte_free(meter);
> +		snprintf(s_err_msg, sizeof(s_err_msg),
> +			"TC[%u] HW policing set failed (%d)!", mtr_id, ret);
> +		return rte_mtr_error_set(error, -ret,
> +			RTE_MTR_ERROR_TYPE_UNSPECIFIED, NULL, s_err_msg);
> +	}
> +
> +	rte_spinlock_lock(&priv->meter_lock);

While HW was being programmed, in theory the policy and mtr_id could have
gone away. Either refcount the mtr_id or do a quick rescan to make sure
it hasn't disappeared.

> +	curr = LIST_FIRST(&priv->meters);
> +	if (curr == NULL) {
> +		LIST_INSERT_HEAD(&priv->meters, meter, next);
> +	} else {
> +		while (LIST_NEXT(curr, next) != NULL)
> +			curr = LIST_NEXT(curr, next);
> +		LIST_INSERT_AFTER(curr, meter, next);
> +	}
> +
> +quit:
> +	rte_spinlock_unlock(&priv->meter_lock);
> +	if (ret != 0) {
> +		if (meter != NULL && ret == ENOMEM)
> +			rte_free(meter);
> +		return rte_mtr_error_set(error, ret, err_type, NULL, s_err_msg);
> +	}
> +
> +	return 0;
> +}

The code would also be easier to read with helper functions that
looked up in list rather lots of open coded list walks.