From: Daniele Di Proietto [mailto:diproiet...@ovn.org] Sent: Friday, August 05, 2016 2:15 AM To: Stokes, Ian Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH v1 1/1] netdev-dpdk: Fix egress policer error detection bug.
Thanks for the patch, comments inline 2016-08-02 9:37 GMT-07:00 Ian Stokes <ian.sto...@intel.com<mailto:ian.sto...@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.sto...@intel.com<mailto:ian.sto...@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 for the review Daniele, I agree with the comments above and have sent a v2 patch http://openvswitch.org/pipermail/dev/2016-August/077592.html As regards the RCU, I had started work on this but didn’t have a chance to finish it off; it’s something I’ll look at again in future as it would be better than what we have now. Thanks Ian Thanks, Daniele _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
