[ovs-dev] [PATCH v1 1/1] netdev-dpdk: Fix egress policer error detection bug.

Daniele Di Proietto diproiettod at ovn.org
Fri Aug 5 01:15:28 UTC 2016


Thanks for the patch, comments inline

2016-08-02 9:37 GMT-07:00 Ian Stokes <ian.stokes at intel.com>:

> When egress policer is set as a QoS type for a port, an error may occur
> during
> setup if incorrect parameters are used for the rte_meter. If this occurs
> the egress policer construct and set functions should free any allocated
> memory relevant to the policer and set the QoS configuration pointer to
> null. The netdev_dpdk_set_qos function should check the error value
> returned
> for any QoS construct/set calls with an assertion to avoid segfault.
>
> Signed-off-by: Ian Stokes <ian.stokes at intel.com>
> ---
>  lib/netdev-dpdk.c |   29 ++++++++++++++++++++++++++++-
>  1 files changed, 28 insertions(+), 1 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index c208f32..c382270 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2679,12 +2679,19 @@ netdev_dpdk_set_qos(struct netdev *netdev,
>
>              /* Install new QoS configuration. */
>              error = new_ops->qos_construct(netdev, details);
> -            ovs_assert((error == 0) == (dev->qos_conf != NULL));
>          }
>      } else {
>          error = new_ops->qos_construct(netdev, details);
> +    }
> +
> +    if (!error) {
>          ovs_assert((error == 0) == (dev->qos_conf != NULL));
>      }
> +    else {
> +        VLOG_ERR("Failed to set QoS type %s on port %s, returned error
> %d",
> +                type, netdev->name, error);
> +        ovs_assert(dev->qos_conf == NULL);
> +    }
>

I think we can replace this with:

ovs_assert((error == 0) == (dev->qos_conf != NULL));
if (!error) {
   VLOG(...)
}

type should be aligned with " on the above line

Can we use rte_strerror to print a textual representation?


>
>      ovs_mutex_unlock(&dev->mutex);
>      return error;
> @@ -2726,6 +2733,15 @@ egress_policer_qos_construct(struct netdev *netdev,
>      policer->app_srtcm_params.ebs = 0;
>      err = rte_meter_srtcm_config(&policer->egress_meter,
>                                      &policer->app_srtcm_params);
> +
> +    if (err < 0) {
> +        /* Error occurred during rte_meter creation, destroy the policer
> +         * and set the qos configuration for the netdev dpdk to NULL
> +         */
> +        free(policer);
> +        dev->qos_conf = NULL;
> +    }
> +
>

Can we return a positive error number instead of a negative one? This is
more inline with the rest of OVS

     rte_spinlock_unlock(&dev->qos_lock);
>
>      return err;
> @@ -2756,11 +2772,13 @@ static int
>  egress_policer_qos_set(struct netdev *netdev, const struct smap *details)
>  {
>      struct egress_policer *policer;
> +    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>      const char *cir_s;
>      const char *cbs_s;
>      int err = 0;
>
>      policer = egress_policer_get__(netdev);
> +    rte_spinlock_lock(&dev->qos_lock);
>      cir_s = smap_get(details, "cir");
>      cbs_s = smap_get(details, "cbs");
>      policer->app_srtcm_params.cir = cir_s ? strtoull(cir_s, NULL, 10) : 0;
> @@ -2769,6 +2787,15 @@ egress_policer_qos_set(struct netdev *netdev, const
> struct smap *details)
>      err = rte_meter_srtcm_config(&policer->egress_meter,
>                                      &policer->app_srtcm_params);
>
> +    if (err < 0) {
> +        /* Error occurred during rte_meter creation, destroy the policer
> +         * and set the qos configuration for the netdev dpdk to NULL
> +         */
> +        free(policer);
> +        dev->qos_conf = NULL;
> +    }
> +    rte_spinlock_unlock(&dev->qos_lock);
> +
>

Can we return a positive error number instead of a negative one? This is
more inline with the rest of OVS

I guess we forgot to lock the spinlock here on the original patch and this
commit fixes it. Can you document this in the commit message?

In the long term I'd like this to use RCU, as we wouldn't need so many
critical sections, but it's fine to avoid it for now



>      return err;
>  }
>
>

Thanks,

Daniele



More information about the dev mailing list