[ovs-dev] [PATCH 1/2] dpif: Don't pass in '*meter_id' to meter_set commands.

Ben Pfaff blp at ovn.org
Tue Aug 14 20:55:21 UTC 2018


On Wed, Aug 08, 2018 at 05:35:21PM -0700, Justin Pettit wrote:
> The original intent of the API appears to be that the underlying DPIF
> implementaion would choose a local meter id.  However, neither of the
> existing datapath meter implementations (userspace or Linux) implemented
> that; they expected a valid meter id to be passed in, otherwise they
> returned an error.  This commit follows the existing implementations and
> makes the API somewhat cleaner.
> 
> Signed-off-by: Justin Pettit <jpettit at ovn.org>

My suggestions are below in patch form.  The change to probe_meters() is
because I'm concerned about the thread-safety guarantee that dpif.h
promises.  It's easy to make this thread safe so we might as well.

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 60ce1a6d22a3..d9b404c2bb92 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -53,6 +53,7 @@
 #include "openvswitch/ofpbuf.h"
 #include "openvswitch/poll-loop.h"
 #include "openvswitch/shash.h"
+#include "openvswitch/thread.h"
 #include "openvswitch/vlog.h"
 #include "packets.h"
 #include "random.h"
@@ -2983,7 +2984,7 @@ static void
 dpif_netlink_meter_get_features(const struct dpif *dpif_,
                                 struct ofputil_meter_features *features)
 {
-    if (probe_broken_meters((struct dpif *)dpif_)) {
+    if (probe_broken_meters(CONST_CAST(struct dpif *, dpif_))) {
         features = NULL;
         return;
     }
@@ -3222,16 +3223,8 @@ dpif_netlink_meter_del(struct dpif *dpif, ofproto_meter_id meter_id,
 }
 
 static bool
-probe_broken_meters(struct dpif *dpif)
+probe_broken_meters__(struct dpif *dpif)
 {
-    static bool checked = false;
-    static bool broken_meters = false;
-
-    if (checked) {
-        return broken_meters;
-    }
-    checked = true;
-
     /* This test is destructive if a probe occurs while ovs-vswitchd is
      * running (e.g., an ovs-dpctl meter command is called), so choose a
      * random high meter id to make this less likely to occur. */
@@ -3249,17 +3242,29 @@ probe_broken_meters(struct dpif *dpif)
     if (dpif_netlink_meter_get(dpif, id1, NULL, 0)
         || dpif_netlink_meter_get(dpif, id2, NULL, 0)) {
         VLOG_INFO("The kernel module has a broken meter implementation.");
-        broken_meters = true;
-        goto done;
+        return true;
     }
 
     dpif_netlink_meter_del(dpif, id1, NULL, 0);
     dpif_netlink_meter_del(dpif, id2, NULL, 0);
 
-done:
-    return broken_meters;
+    return false;
 }
 
+static bool
+probe_broken_meters(struct dpif *dpif)
+{
+    /* This is a once-only test because currently OVS only has at most a single
+     * Netlink capable datapath on any given platform. */
+    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+
+    static bool broken_meters = false;
+    if (ovsthread_once_start(&once)) {
+        broken_meters = probe_broken_meters__(dpif);
+        ovsthread_once_done(&once);
+    }
+    return broken_meters;
+}
 
 const struct dpif_class dpif_netlink_class = {
     "system",


More information about the dev mailing list