[ovs-dev] [PATCH monitor_cond V12 2/2] ovn: implementation of conditional monitoring usage

Ben Pfaff blp at ovn.org
Sun Aug 14 04:51:05 UTC 2016


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



More information about the dev mailing list