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

Message ID 20180809003522.10837-1-jpettit@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev,1/2] dpif: Don't pass in '*meter_id' to meter_set commands.
Related show

Commit Message

Justin Pettit Aug. 9, 2018, 12:35 a.m.
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@ovn.org>
---
 lib/dpif-netdev.c      |  4 ++--
 lib/dpif-netlink.c     | 13 ++++++++-----
 lib/dpif-provider.h    |  9 ++++-----
 lib/dpif.c             | 14 ++++++--------
 lib/dpif.h             |  2 +-
 ofproto/ofproto-dpif.c |  2 +-
 6 files changed, 22 insertions(+), 22 deletions(-)

Comments

Ben Pfaff Aug. 14, 2018, 8:55 p.m. | #1
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@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",
Ben Pfaff Aug. 14, 2018, 8:55 p.m. | #2
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@ovn.org>

For the series (please consider the comments on patch 2):
Acked-by: Ben Pfaff <blp@ovn.org>
Justin Pettit Aug. 16, 2018, 5:31 p.m. | #3
> On Aug 14, 2018, at 1:55 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> 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@ovn.org>
> 
> For the series (please consider the comments on patch 2):
> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks!  I pushed the changes with your suggestion to master and branch-2.10.

--Justin

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 26d07b39c9af..8c9062485bf7 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -5160,11 +5160,11 @@  dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_,
 
 /* Meter set/get/del processing is still single-threaded. */
 static int
-dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id *meter_id,
+dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id meter_id,
                       struct ofputil_meter_config *config)
 {
     struct dp_netdev *dp = get_dp_netdev(dpif);
-    uint32_t mid = meter_id->uint32;
+    uint32_t mid = meter_id.uint32;
     struct dp_meter *meter;
     int i;
 
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index f669b1108d61..bf94e5413e71 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -3030,7 +3030,7 @@  dpif_netlink_meter_get_features(const struct dpif *dpif_,
 }
 
 static int
-dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id *meter_id,
+dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id meter_id,
                        struct ofputil_meter_config *config)
 {
     struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
@@ -3057,9 +3057,8 @@  dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id *meter_id,
 
     dpif_netlink_meter_init(dpif, &buf, stub, sizeof stub, OVS_METER_CMD_SET);
 
-    if (meter_id->uint32 != UINT32_MAX) {
-        nl_msg_put_u32(&buf, OVS_METER_ATTR_ID, meter_id->uint32);
-    }
+    nl_msg_put_u32(&buf, OVS_METER_ATTR_ID, meter_id.uint32);
+
     if (config->flags & OFPMF13_KBPS) {
         nl_msg_put_flag(&buf, OVS_METER_ATTR_KBPS);
     }
@@ -3098,7 +3097,11 @@  dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id *meter_id,
         return error;
     }
 
-    meter_id->uint32 = nl_attr_get_u32(a[OVS_METER_ATTR_ID]);
+    if (nl_attr_get_u32(a[OVS_METER_ATTR_ID]) != meter_id.uint32) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+        VLOG_INFO_RL(&rl,
+                     "Kernel returned a different meter id than requested");
+    }
     ofpbuf_delete(msg);
     return 0;
 }
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 62b3598acfc5..8906d4e0a1e6 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -451,12 +451,11 @@  struct dpif_class {
     void (*meter_get_features)(const struct dpif *,
                                struct ofputil_meter_features *);
 
-    /* Adds or modifies 'meter' in 'dpif'.   If '*meter_id' is UINT32_MAX,
-     * adds a new meter, otherwise modifies an existing meter.
+    /* Adds or modifies the meter in 'dpif' with the given 'meter_id'
+     * and the configuration in 'config'.
      *
-     * If meter is successfully added, sets '*meter_id' to the new meter's
-     * meter id selected by 'dpif'. */
-    int (*meter_set)(struct dpif *, ofproto_meter_id *meter_id,
+     * The meter id specified through 'config->meter_id' is ignored. */
+    int (*meter_set)(struct dpif *, ofproto_meter_id meter_id,
                      struct ofputil_meter_config *);
 
     /* Queries 'dpif' for meter stats with the given 'meter_id'.  Stores
diff --git a/lib/dpif.c b/lib/dpif.c
index c267bcfb0c55..d799f972cbf6 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1886,13 +1886,12 @@  dpif_meter_get_features(const struct dpif *dpif,
     }
 }
 
-/* Adds or modifies meter identified by 'meter_id' in 'dpif'.  If '*meter_id'
- * is UINT32_MAX, adds a new meter, otherwise modifies an existing meter.
+/* Adds or modifies the meter in 'dpif' with the given 'meter_id' and
+ * the configuration in 'config'.
  *
- * If meter is successfully added, sets '*meter_id' to the new meter's
- * meter number. */
+ * The meter id specified through 'config->meter_id' is ignored. */
 int
-dpif_meter_set(struct dpif *dpif, ofproto_meter_id *meter_id,
+dpif_meter_set(struct dpif *dpif, ofproto_meter_id meter_id,
                struct ofputil_meter_config *config)
 {
     COVERAGE_INC(dpif_meter_set);
@@ -1918,11 +1917,10 @@  dpif_meter_set(struct dpif *dpif, ofproto_meter_id *meter_id,
     int error = dpif->dpif_class->meter_set(dpif, meter_id, config);
     if (!error) {
         VLOG_DBG_RL(&dpmsg_rl, "%s: DPIF meter %"PRIu32" set",
-                    dpif_name(dpif), meter_id->uint32);
+                    dpif_name(dpif), meter_id.uint32);
     } else {
         VLOG_WARN_RL(&error_rl, "%s: failed to set DPIF meter %"PRIu32": %s",
-                     dpif_name(dpif), meter_id->uint32, ovs_strerror(error));
-        meter_id->uint32 = UINT32_MAX;
+                     dpif_name(dpif), meter_id.uint32, ovs_strerror(error));
     }
     return error;
 }
diff --git a/lib/dpif.h b/lib/dpif.h
index 33d2d0bec333..bbdc3eb6cfb6 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -868,7 +868,7 @@  void dpif_print_packet(struct dpif *, struct dpif_upcall *);
 /* Meters. */
 void dpif_meter_get_features(const struct dpif *,
                              struct ofputil_meter_features *);
-int dpif_meter_set(struct dpif *, ofproto_meter_id *meter_id,
+int dpif_meter_set(struct dpif *, ofproto_meter_id meter_id,
                    struct ofputil_meter_config *);
 int dpif_meter_get(const struct dpif *, ofproto_meter_id meter_id,
                    struct ofputil_meter_stats *, uint16_t n_bands);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ad67e300a8be..e3abda571d31 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5959,7 +5959,7 @@  meter_set(struct ofproto *ofproto_, ofproto_meter_id *meter_id,
         }
     }
 
-    switch (dpif_meter_set(ofproto->backer->dpif, meter_id, config)) {
+    switch (dpif_meter_set(ofproto->backer->dpif, *meter_id, config)) {
     case 0:
         return 0;
     case EFBIG: /* meter_id out of range */