diff mbox

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

Message ID CD7C01071941AC429549C17338DB8A5288F9799B@IRSMSX101.ger.corp.intel.com
State Not Applicable
Headers show

Commit Message

Stokes, Ian Aug. 9, 2016, 5:23 p.m. UTC 
From: Daniele Di Proietto [mailto:diproiettod@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.stokes@intel.com<mailto: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<mailto:ian.stokes@intel.com>>

---
 lib/netdev-dpdk.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

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
diff mbox

Patch

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: