[ovs-dev] [PATCH 1/2] dpif: Don't pass in '*meter_id' to meter_set commands.
Justin Pettit
jpettit at ovn.org
Thu Aug 9 00:35:21 UTC 2018
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>
---
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(-)
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 */
--
2.17.1
More information about the dev
mailing list