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.
