[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