dpif: Don't pass in '*meter_id' to meter_set commands.

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>
Acked-by: Ben Pfaff <blp@ovn.org>

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;
 

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;
 }

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

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;
 }

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);

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 */