On Tue, Aug 02, 2016 at 04:38:00PM +0300, Liran Schour wrote:
> Conditional monitor of: Port_Binding, Logical_Flow, Multicast_Group
> MAC_Binding tables. As a result ovn-controller will be notified only about
> records belongs to a datapath that is being served by this hypervisor.
>
> Performance evaluation:
> OVN is the main candidate for conditional monitoring usage. It is clear that
> conditional monitoring reduces computation on the ovn-controller (client) side
> due to the reduced size of flow tables and update messages. Performance
> evaluation shows up to 75% computation reduction on the ovn-controller side.
> However, performance evaluation shows also a reduction in computation on the
> SB
> ovsdb-server side proportional to the degree that each logical network is
> spread over physical hosts in the DC. Evaluation shows that in a realistic
> scenarios there is a computation reduction also in the server side.
>
> Evaluation on simulated environment of 50 hosts and 1000 logical ports shows
> the following results (cycles #):
>
> LN spread over # hosts| master | patch | change
> -------------------------------------------------------------
> 1 | 24597200127 | 24339235374 | 1.0%
> 6 | 23788521572 | 19145229352 | 19.5%
> 12 | 23886405758 | 17913143176 | 25.0%
> 18 | 25812686279 | 23675094540 | 8.2%
> 24 | 28414671499 | 24770202308 | 12.8%
> 30 | 31487218890 | 28397543436 | 9.8%
> 36 | 36116993930 | 34105388739 | 5.5%
> 42 | 37898342465 | 38647139083 | -1.9%
> 48 | 41637996229 | 41846616306 | -0.5%
> 50 | 41679995357 | 43455565977 | -4.2%
>
> TODO:
> - If performance evaluation will show the need, add a tenant-id
> (connectivity-id) column in the datapath_binding table to reduce the number
> of hops that is required to retrieve all datapaths.
>
> Signed-off-by: Liran Schour <lir...@il.ibm.com>
Thanks for the patch. This seems very promising. I have a number of
style and comment suggestions to fold in, which I'm appending. Also
there's one serious bug fix: xmalloc(strlen(string)) does not allocate
enough memory to copy in 'string'!
I was on the verge of applying this but then I noticed what appears to
be a serious error. This code retains a pointer to "struct
sbrec_datapath_binding"s across calls to ovsdb_idl_run(). That's going
to cause a use-after-free error in a case where the Datapath_Binding
record gets deleted, causing the struct sbrec_datapath_binding to be
freed. Some other way to keep track of the Datapath_Binding record
will be necessary.
Thanks,
Ben.
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/controller/filter.c b/ovn/controller/filter.c
index d90fb79..b49bd8f 100644
--- a/ovn/controller/filter.c
+++ b/ovn/controller/filter.c
@@ -38,6 +38,8 @@ struct filtered_lp {
bool used;
};
+/* Initializes 'idl' so that by default no rows are replicated in tables that
+ * ovn-controller does not need to be fully replicated. */
void
filter_init(struct ovsdb_idl *idl)
{
@@ -47,39 +49,39 @@ filter_init(struct ovsdb_idl *idl)
sbrec_multicast_group_add_clause_false(idl);
}
+/* Marks all replicated ports as "unused". */
void
filter_mark_unused(void)
{
struct filtered_lp *lp;
- HMAP_FOR_EACH(lp, hmap_node, &filtered_lps) {
+ HMAP_FOR_EACH (lp, hmap_node, &filtered_lps) {
lp->used = false;
}
}
+/* Clears the filter conditions, so that no rows are replicated. */
void
filter_clear(struct ovsdb_idl *idl)
{
- struct filtered_lp *lp, *next;
+ struct filtered_lp *lp, *next_lp;
struct filtered_lp *dp, *next_dp;
- HMAP_FOR_EACH_SAFE(lp, next, hmap_node, &filtered_lps) {
+ HMAP_FOR_EACH_SAFE (lp, next_lp, hmap_node, &filtered_lps) {
hmap_remove(&filtered_lps, &lp->hmap_node);
free(lp);
}
- HMAP_FOR_EACH_SAFE(dp, next_dp, hmap_node, &filtered_dps) {
+ HMAP_FOR_EACH_SAFE (dp, next_dp, hmap_node, &filtered_dps) {
hmap_remove(&filtered_dps, &dp->hmap_node);
free(dp);
}
- ovsdb_idl_condition_reset(idl,
- &sbrec_table_port_binding);
- ovsdb_idl_condition_reset(idl,
- &sbrec_table_logical_flow);
- ovsdb_idl_condition_reset(idl,
- &sbrec_table_mac_binding);
- ovsdb_idl_condition_reset(idl,
- &sbrec_table_multicast_group);
+ ovsdb_idl_condition_reset(idl, &sbrec_table_port_binding);
+ ovsdb_idl_condition_reset(idl, &sbrec_table_logical_flow);
+ ovsdb_idl_condition_reset(idl, &sbrec_table_mac_binding);
+ ovsdb_idl_condition_reset(idl, &sbrec_table_multicast_group);
+
+ filter_init(idl);
}
static struct filtered_dp *
@@ -87,8 +89,7 @@ lookup_dp_by_key(int64_t tunnel_key)
{
struct filtered_dp *dp;
- HMAP_FOR_EACH_WITH_HASH(dp, hmap_node, tunnel_key,
- &filtered_dps) {
+ HMAP_FOR_EACH_WITH_HASH (dp, hmap_node, tunnel_key, &filtered_dps) {
if (dp->tunnel_key == tunnel_key) {
return dp;
}
@@ -96,13 +97,15 @@ lookup_dp_by_key(int64_t tunnel_key)
return NULL;
}
+/* Un-replicates logical ports that have not been re-added via filter_lport()
+ * since the last call to filter_mark_unused(). */
void
filter_remove_unused_lports(struct controller_ctx *ctx,
const struct lport_index *lports_index)
{
struct filtered_lp *lp, *next;
- HMAP_FOR_EACH_SAFE(lp, next, hmap_node, &filtered_lps) {
+ HMAP_FOR_EACH_SAFE (lp, next, hmap_node, &filtered_lps) {
if (!lp->used) {
const struct sbrec_port_binding *pb =
lport_lookup_by_name(lports_index, lp->lport_name);
@@ -110,7 +113,7 @@ filter_remove_unused_lports(struct controller_ctx *ctx,
continue;
}
if (lookup_dp_by_key(pb->datapath->tunnel_key)) {
- VLOG_INFO("Unfilter Port %s", lp->lport_name);
+ VLOG_DBG("Unfilter Port %s", lp->lport_name);
sbrec_port_binding_remove_clause_logical_port(ctx->ovnsb_idl,
OVSDB_F_EQ,
lp->lport_name);
@@ -121,11 +124,13 @@ filter_remove_unused_lports(struct controller_ctx *ctx,
}
}
+/* Adds 'lport_name' to the logical ports whose Port_Binding rows are
+ * replicated. */
void
filter_lport(struct controller_ctx *ctx, const char *lport_name)
{
struct filtered_lp *lp;
- size_t hash = hash_bytes(lport_name, strlen(lport_name), 0);
+ size_t hash = hash_string(lport_name, 0);
HMAP_FOR_EACH_WITH_HASH(lp, hmap_node, hash, &filtered_lps) {
if (!strcmp(lp->lport_name, lport_name)) {
@@ -134,22 +139,20 @@ filter_lport(struct controller_ctx *ctx, const char
*lport_name)
}
}
- VLOG_INFO("Filter Port %s", lport_name);
+ VLOG_DBG("Filter Port %s", lport_name);
sbrec_port_binding_add_clause_logical_port(ctx->ovnsb_idl,
OVSDB_F_EQ,
lport_name);
- char *name = xmalloc(strlen(lport_name));
lp = xmalloc(sizeof *lp);
-
- strcpy(name, lport_name);
- lp->lport_name = name;
+ lp->lport_name = xstrdup(lport_name);
lp->used = true;
- hmap_insert(&filtered_lps, &lp->hmap_node,
- hash);
+ hmap_insert(&filtered_lps, &lp->hmap_node, hash);
}
+/* Adds 'datapath' to the datapaths whose Port_Binding, Mac_Binding,
+ * Logical_Flow, and Multicast_Group rows are replicated. */
void
filter_datapath(struct controller_ctx *ctx,
const struct sbrec_datapath_binding *datapath)
@@ -161,7 +164,7 @@ filter_datapath(struct controller_ctx *ctx,
return;
}
- VLOG_INFO("Filter DP "UUID_FMT, UUID_ARGS(&datapath->header_.uuid));
+ VLOG_DBG("Filter DP "UUID_FMT, UUID_ARGS(&datapath->header_.uuid));
sbrec_port_binding_add_clause_datapath(ctx->ovnsb_idl,
OVSDB_F_EQ,
datapath);
@@ -181,13 +184,15 @@ filter_datapath(struct controller_ctx *ctx,
hmap_insert(&filtered_dps, &dp->hmap_node, datapath->tunnel_key);
}
+/* Removes 'datapath' from the datapaths whose Port_Binding, Mac_Binding,
+ * Logical_Flow, and Multicast_Group rows are replicated. */
void
unfilter_datapath(struct controller_ctx *ctx, int64_t tunnel_key)
{
struct filtered_dp *dp = lookup_dp_by_key(tunnel_key);
if (dp) {
- VLOG_INFO("Unfilter DP "UUID_FMT,
+ VLOG_DBG("Unfilter DP "UUID_FMT,
UUID_ARGS(&dp->datapath->header_.uuid));
sbrec_port_binding_remove_clause_datapath(ctx->ovnsb_idl,
OVSDB_F_EQ,
diff --git a/ovn/controller/filter.h b/ovn/controller/filter.h
index e09c1c8..788fe72 100644
--- a/ovn/controller/filter.h
+++ b/ovn/controller/filter.h
@@ -16,6 +16,16 @@
#ifndef OVN_FILTER_H
#define OVN_FILTER_H 1
+/* Database client filtering
+ * -------------------------
+ *
+ * By default the OVSDB IDL replicates the entire contents of each table. For
+ * some tables, however, ovn-controller only needs some rows. For example, in
+ * the Logical_Flow table, it only needs the rows for logical datapaths that
+ * are in use directly or indirectly on this hypervisor. These functions aid
+ * in limiting the rows that the IDL replicates.
+ */
+
#include <stdint.h>
struct controller_ctx;
@@ -23,14 +33,14 @@ struct ovsdb_idl;
struct sbrec_datapath_binding;
struct lport_index;
-void filter_init(struct ovsdb_idl *idl);
-void filter_clear(struct ovsdb_idl *idl);
+void filter_init(struct ovsdb_idl *);
+void filter_clear(struct ovsdb_idl *);
void filter_mark_unused(void);
-void filter_remove_unused_lports(struct controller_ctx *ctx,
- const struct lport_index *lports);
-void filter_lport(struct controller_ctx *ctx, const char *lport_name);
-void filter_datapath(struct controller_ctx *ctx,
- const struct sbrec_datapath_binding *datapath);
-void unfilter_datapath(struct controller_ctx *ctx, int64_t tunnel_key);
+void filter_remove_unused_lports(struct controller_ctx *,
+ const struct lport_index *);
+void filter_lport(struct controller_ctx *, const char *lport_name);
+void filter_datapath(struct controller_ctx *,
+ const struct sbrec_datapath_binding *);
+void unfilter_datapath(struct controller_ctx *, int64_t tunnel_key);
#endif /* ovn/controller/filter.h */
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
