[ovs-dev] [ovs-dev, v23, 1/2] ovn-controller: Persist ovn flow tables
Ben Pfaff
blp at ovn.org
Tue Jul 19 05:48:46 UTC 2016
On Mon, Jul 18, 2016 at 04:21:16PM -0500, Ryan Moats wrote:
> Ensure that ovn flow tables are persisted so that changes to
> them chan be applied incrementally - this is a prereq for
> making lflow_run and physical_run incremental.
>
> As part of this change, add a one-to-many hindex for finding
> desired flows by their parent's UUID. Also extend the mapping
> by match from one-to-one to one-to-many.
>
> Signed-off-by: Ryan Moats <rmoats at us.ibm.com>
I applied this to master. I folded in the following adjustments to
style and comments and the new macro HINDEX_FOR_EACH_WITH_HASH_SAFE.
Thanks,
Ben.
--8<--------------------------cut here-------------------------->8--
diff --git a/lib/hindex.h b/lib/hindex.h
index 416da05..876c5a9 100644
--- a/lib/hindex.h
+++ b/lib/hindex.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2013 Nicira, Inc.
+ * Copyright (c) 2013, 2016 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -132,6 +132,15 @@ void hindex_remove(struct hindex *, struct hindex_node *);
NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER); \
ASSIGN_CONTAINER(NODE, (NODE)->MEMBER.s, MEMBER))
+/* Safe when NODE may be freed (not needed when NODE may be removed from the
+ * hash map but its members remain accessible and intact). */
+#define HINDEX_FOR_EACH_WITH_HASH_SAFE(NODE, NEXT, MEMBER, HASH, HINDEX) \
+ for (INIT_CONTAINER(NODE, hindex_node_with_hash(HINDEX, HASH), MEMBER); \
+ (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER) \
+ ? INIT_CONTAINER(NEXT, (NODE)->MEMBER.s, MEMBER), 1 \
+ : 0); \
+ (NODE) = (NEXT))
+
/* Returns the head node in 'hindex' with the given 'hash', or a null pointer
* if no nodes have that hash value. */
static inline struct hindex_node *
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 40e49a1..d10dcc6 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -54,18 +54,12 @@ struct ovn_flow {
uint16_t priority;
struct match match;
- /* Data. UUID is used for disambiquation. */
+ /* Data. UUID is used for disambiguation. */
struct uuid uuid;
struct ofpact *ofpacts;
size_t ofpacts_len;
};
-static inline struct ovn_flow *
-ovn_flow_from_node(const struct ovs_list *node)
-{
- return CONTAINER_OF(node, struct ovn_flow, list_node);
-}
-
static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
static void ovn_flow_lookup(struct hmap *, const struct ovn_flow *target,
struct ovs_list *answers);
@@ -515,26 +509,32 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
/* Flow table interfaces to the rest of ovn-controller. */
-/* Adds a flow to flow tables with the specified 'match' and 'actions' to
- * the OpenFlow table numbered 'table_id' with the given 'priority'. The
- * caller retains ownership of 'match' and 'actions'.
+/* Adds a flow to the collection associated with 'uuid'. The flow has the
+ * specified 'match' and 'actions' to the OpenFlow table numbered 'table_id'
+ * with the given 'priority'. The caller retains ownership of 'match' and
+ * 'actions'.
+ *
+ * Any number of flows may be associated with a given UUID. The flows with a
+ * given UUID must have a unique (table_id, priority, match) tuple. A
+ * duplicate within a generally indicates a bug in the ovn-controller code that
+ * generated it, so this functions logs a warning.
*
- * Because it is possible for both actions and matches to change on a rule,
- * and because the hmap struct only supports a single hash, this method
- * uses a hash on the defined key of (table_id, priority, match criteria).
- * Actions from different parents is supported, with the parent's UUID
- * information stored for disambiguation.
+ * (table_id, priority, match) tuples should also be unique for flows with
+ * different UUIDs, but it doesn't necessarily indicate a bug in
+ * ovn-controller, for two reasons. First, these duplicates could be caused by
+ * logical flows generated by ovn-northd, which aren't ovn-controller's fault;
+ * perhaps something should warn about these but the root cause is different.
+ * Second, these duplicates might be transient, that is, they might go away
+ * before the next call to ofctrl_run() if a call to ofctrl_remove_flows()
+ * removes one or the other.
*
* This just assembles the desired flow tables in memory. Nothing is actually
* sent to the switch until a later call to ofctrl_run(). */
-
void
ofctrl_add_flow(uint8_t table_id, uint16_t priority,
const struct match *match, const struct ofpbuf *actions,
const struct uuid *uuid)
{
- struct ovs_list existing;
-
/* Structure that uses table_id+priority+various things as hashes. */
struct ovn_flow *f = xmalloc(sizeof *f);
f->table_id = table_id;
@@ -542,38 +542,29 @@ ofctrl_add_flow(uint8_t table_id, uint16_t priority,
f->match = *match;
f->ofpacts = xmemdup(actions->data, actions->size);
f->ofpacts_len = actions->size;
- memcpy(&f->uuid, uuid, sizeof f->uuid);
+ f->uuid = *uuid;
f->match_hmap_node.hash = ovn_flow_match_hash(f);
f->uuid_hindex_node.hash = uuid_hash(&f->uuid);
- /* Check to see if other flows exist with the same key (table_id
- * priority, match criteria). If so, check if this flow's uuid
- * exists already. If so, discard this flow and log a warning.
- * If the uuid isn't present, then add the flow.
- *
- * If no other flows have this key, then add the flow. */
-
+ /* Check to see if other flows exist with the same key (table_id priority,
+ * match criteria) and uuid. If so, discard this flow and log a
+ * warning. */
+ struct ovs_list existing;
ovn_flow_lookup(&match_flow_table, f, &existing);
-
- if (!ovs_list_is_empty(&existing)) {
- struct ovn_flow *d;
- LIST_FOR_EACH(d, list_node, &existing) {
- if (f->table_id == d->table_id && f->priority == d->priority
- && ofpacts_equal(f->ofpacts, f->ofpacts_len,
- d->ofpacts, d->ofpacts_len)) {
- if (uuid_equals(&f->uuid, &d->uuid)) {
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
- char *s = ovn_flow_to_string(f);
- VLOG_WARN_RL(&rl, "found duplicate flow %s for parent "UUID_FMT,
- s, UUID_ARGS(&f->uuid));
- ovn_flow_destroy(f);
- free(s);
- return;
- }
- }
+ struct ovn_flow *d;
+ LIST_FOR_EACH (d, list_node, &existing) {
+ if (uuid_equals(&f->uuid, &d->uuid)) {
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+ char *s = ovn_flow_to_string(f);
+ VLOG_WARN_RL(&rl, "found duplicate flow %s for parent "UUID_FMT,
+ s, UUID_ARGS(&f->uuid));
+ ovn_flow_destroy(f);
+ free(s);
+ return;
}
}
+ /* Otherwise, add the flow. */
hmap_insert(&match_flow_table, &f->match_hmap_node,
f->match_hmap_node.hash);
hindex_insert(&uuid_flow_table, &f->uuid_hindex_node,
@@ -581,28 +572,18 @@ ofctrl_add_flow(uint8_t table_id, uint16_t priority,
}
/* Removes a bundles of flows from the flow table. */
-
void
ofctrl_remove_flows(const struct uuid *uuid)
{
- /* Since there isn't an HINDEX_FOR_EACH_SAFE_WITH_HASH macro,
- * first, process the hindex and put matching flows into a local list.
- * Then, the local list is processed and the items deleted. */
- struct ovs_list delete_list;
- ovs_list_init(&delete_list);
-
- struct ovn_flow *f;
- HINDEX_FOR_EACH_WITH_HASH(f, uuid_hindex_node, uuid_hash(uuid),
- &uuid_flow_table) {
+ struct ovn_flow *f, *next;
+ HINDEX_FOR_EACH_WITH_HASH_SAFE (f, next, uuid_hindex_node, uuid_hash(uuid),
+ &uuid_flow_table) {
if (uuid_equals(&f->uuid, uuid)) {
- ovs_list_push_back(&delete_list, &f->list_node);
+ hmap_remove(&match_flow_table, &f->match_hmap_node);
+ hindex_remove(&uuid_flow_table, &f->uuid_hindex_node);
+ ovn_flow_destroy(f);
}
}
- LIST_FOR_EACH_POP(f, list_node, &delete_list) {
- hmap_remove(&match_flow_table, &f->match_hmap_node);
- hindex_remove(&uuid_flow_table, &f->uuid_hindex_node);
- ovn_flow_destroy(f);
- }
}
/* Shortcut to remove all flows matching the supplied UUID and add this
@@ -620,18 +601,18 @@ ofctrl_set_flow(uint8_t table_id, uint16_t priority,
/* Duplicate an ovn_flow structure. */
struct ovn_flow *
-ofctrl_dup_flow(struct ovn_flow *source)
-{
- struct ovn_flow *answer = xmalloc(sizeof *answer);
- answer->table_id = source->table_id;
- answer->priority = source->priority;
- answer->match = source->match;
- answer->ofpacts = xmemdup(source->ofpacts, source->ofpacts_len);
- answer->ofpacts_len = source->ofpacts_len;
- answer->uuid = source->uuid;
- answer->match_hmap_node.hash = ovn_flow_match_hash(answer);
- answer->uuid_hindex_node.hash = uuid_hash(&source->uuid);
- return answer;
+ofctrl_dup_flow(struct ovn_flow *src)
+{
+ struct ovn_flow *dst = xmalloc(sizeof *dst);
+ dst->table_id = src->table_id;
+ dst->priority = src->priority;
+ dst->match = src->match;
+ dst->ofpacts = xmemdup(src->ofpacts, src->ofpacts_len);
+ dst->ofpacts_len = src->ofpacts_len;
+ dst->uuid = src->uuid;
+ dst->match_hmap_node.hash = ovn_flow_match_hash(dst);
+ dst->uuid_hindex_node.hash = uuid_hash(&src->uuid);
+ return dst;
}
/* Returns a hash of the match key in 'f'. */
@@ -645,19 +626,20 @@ ovn_flow_match_hash(const struct ovn_flow *f)
/* Compare two flows and return -1, 0, 1 based on whether a if less than,
* equal to or greater than b. */
static int
-ovn_flow_compare_flows(struct ovn_flow *a, struct ovn_flow *b) {
+ovn_flow_compare_flows(struct ovn_flow *a, struct ovn_flow *b)
+{
return uuid_compare_3way(&a->uuid, &b->uuid);
}
/* Given a list of ovn_flows, goes through the list and returns
- * a single flow, based on a helper method. */
-
+ * a single flow, in a deterministic way. */
static struct ovn_flow *
-ovn_flow_select_from_list(struct ovs_list *flows) {
+ovn_flow_select_from_list(struct ovs_list *flows)
+{
struct ovn_flow *candidate;
- struct ovn_flow *answer = ovn_flow_from_node(ovs_list_front(flows));
+ struct ovn_flow *answer = NULL;
LIST_FOR_EACH (candidate, list_node, flows) {
- if (ovn_flow_compare_flows(candidate, answer) < 0) {
+ if (!answer || ovn_flow_compare_flows(candidate, answer) < 0) {
answer = candidate;
}
}
@@ -667,7 +649,7 @@ ovn_flow_select_from_list(struct ovs_list *flows) {
/* Initializes and files in the supplied list with ovn_flows from 'flow_table'
* whose key is identical to 'target''s key. */
static void
-ovn_flow_lookup(struct hmap* flow_table, const struct ovn_flow *target,
+ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target,
struct ovs_list *answer)
{
struct ovn_flow *f;
@@ -710,8 +692,8 @@ ovn_flow_destroy(struct ovn_flow *f)
{
if (f) {
free(f->ofpacts);
+ free(f);
}
- free(f);
}
/* Flow tables of struct ovn_flow. */
@@ -790,11 +772,8 @@ queue_group_mod(struct ofputil_group_mod *gm)
}
-/* Replaces the flow table on the switch, if possible, by the flows in
- * 'flow_table', which should have been added with ofctrl_add_flow().
- * Regardless of whether the flow table is updated, this deletes all of the
- * flows from 'flow_table' and frees them. (The hmap itself isn't
- * destroyed.)
+/* Replaces the flow table on the switch, if possible, by the flows in added
+ * with ofctrl_add_flow().
*
* Replaces the group table on the switch, if possible, by the groups in
* 'group_table->desired_groups'. Regardless of whether the group table
@@ -879,7 +858,7 @@ ofctrl_put(struct group_table *group_table)
struct ovn_flow *d = ovn_flow_select_from_list(&matches);
if (!uuid_equals(&i->uuid, &d->uuid)) {
/* Update installed flow's UUID. */
- memcpy(&i->uuid, &d->uuid, sizeof i->uuid);
+ i->uuid = d->uuid;
}
if (!ofpacts_equal(i->ofpacts, i->ofpacts_len,
d->ofpacts, d->ofpacts_len)) {
@@ -915,7 +894,7 @@ ofctrl_put(struct group_table *group_table)
* that match this key, and select the one to be installed. */
struct ovs_list candidates;
ovn_flow_lookup(&match_flow_table, c, &candidates);
- struct ovn_flow *d = ovn_flow_select_from_list(&candidates);
+ struct ovn_flow *d = ovn_flow_select_from_list(&candidates);
/* Send flow_mod to add flow. */
struct ofputil_flow_mod fm = {
.match = d->match,
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 63d6cb8..226ac72 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -51,7 +51,6 @@ physical_register_ovs_idl(struct ovsdb_idl *ovs_idl)
ovsdb_idl_add_column(ovs_idl, &ovsrec_interface_col_external_ids);
}
-
static struct simap localvif_to_ofport =
SIMAP_INITIALIZER(&localvif_to_ofport);
static struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
More information about the dev
mailing list