| Message ID | 1470155850-1932-1-git-send-email-ian.stokes@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Daniele Di Proietto |
| Headers | show |
Thanks for the patch, comments inline 2016-08-02 9:37 GMT-07:00 Ian Stokes <ian.stokes@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@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
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); + } 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; + } + 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); + return err; }
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@intel.com> --- lib/netdev-dpdk.c | 29 ++++++++++++++++++++++++++++- 1 files changed, 28 insertions(+), 1 deletions(-)