[ovs-dev] [PATCH v5 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

[Yi-Hung Wei]

On Wed, Aug 28, 2019 at 3:15 PM Yi-Hung Wei <yihung.wei at gmail.com> wrote:
>
> This patch consumes the CT_Zone and CT_Timeout_Policy tables, maintains
> the zone-based configuration in the vswitchd.  Whenever there is a
> database change, vswitchd will read the datapath, CT_Zone, and
> CT_Timeout_Policy tables from ovsdb, builds an internal snapshot of the
> database configuration in bridge.c, and pushes down the change into
> ofproto and dpif layer.
>
> If a new zone-based timeout policy is added, it updates the zone to
> timeout policy mapping in the per datapath type datapath structure in
> dpif-backer, and pushes down the timeout policy into the datapath via
> dpif interface.
>
> If a timeout policy is no longer used, for kernel datapath, vswitchd
> may not be able to remove it from datapath immediately since
> datapath flows can still reference the to-be-deleted timeout policies.
> Thus, we keep an timeout policy kill list, that vswitchd will go
> back to the list periodically and try to kill the unused timeout policies.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> ---

When reviewing this patch (the 6th patch in v5), please squash in the
following diff from Darrell.

Thanks,

-Yi-Hung

<---------- beginning of patch ------------------>
>From 7518e557281140d58d6e9ce89bd575f73e33f615 Mon Sep 17 00:00:00 2001
From: Darrell Ball <dball at vmware.com>
Date: Fri, 6 Sep 2019 12:52:54 -0700
Subject: [PATCH] bridge: Don't keep IDL state across iterations.

Refactor code to remove keeping OVSDB client IDL state
across iterations, since this leads to unpredictable
behavior, including crashes.

VMWare-BZ: #2413581

Signed-off-by: Darrell Ball <dball at vmware.com>
---
 vswitchd/bridge.c | 55 ++++++++++++++++++++-----------------------------------
 1 file changed, 20 insertions(+), 35 deletions(-)

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 56f42c0736a2..5defaf428571 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -171,7 +171,6 @@ struct datapath {
     struct hmap ct_zones;       /* Map of 'struct ct_zone' elements, indexed
                                  * by 'zone'. */
     struct hmap_node node;      /* Node in 'all_datapaths' hmap. */
-    const struct ovsrec_datapath *dp_cfg;
     unsigned int last_used;     /* The last idl_seqno that this 'datapath'
                                  * used in OVSDB. This number is used for
                                  * garbage collection. */
@@ -680,12 +679,10 @@ datapath_lookup(const char *type)
 }

 static struct datapath *
-datapath_create(const struct ovsrec_datapath *dp_cfg, const char *type)
+datapath_create(const char *type)
 {
     struct datapath *dp = xzalloc(sizeof *dp);
     dp->type = xstrdup(type);
-    dp->dp_cfg = dp_cfg;
-
     hmap_init(&dp->ct_zones);
     hmap_insert(&all_datapaths, &dp->node, hash_string(type, 0));
     return dp;
@@ -710,34 +707,8 @@ datapath_destroy(struct datapath *dp)
 }

 static void
-update_datapath_cfgs(const struct ovsrec_open_vswitch *cfg)
+reconfigure_ct_zones(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
 {
-    struct datapath *dp, *next;
-
-    /* Add new 'datapath's or update existing ones. */
-    for (size_t i = 0; i < cfg->n_datapaths; i++) {
-        const struct ovsrec_datapath *dp_cfg = cfg->value_datapaths[i];
-        char *dp_name = cfg->key_datapaths[i];
-
-        dp = datapath_lookup(dp_name);
-        if (!dp) {
-            dp = datapath_create(dp_cfg, dp_name);
-        }
-        dp->last_used = idl_seqno;
-    }
-
-    /* Purge deleted 'datapath's. */
-    HMAP_FOR_EACH_SAFE (dp, next, node, &all_datapaths) {
-        if (dp->last_used != idl_seqno) {
-            datapath_destroy(dp);
-        }
-    }
-}
-
-static void
-reconfigure_ct_zones(struct datapath *dp)
-{
-    const struct ovsrec_datapath *dp_cfg = dp->dp_cfg;
     struct ct_zone *ct_zone, *next;

     /* Add new 'ct_zone's or update existing 'ct_zone's based on the database
@@ -776,12 +747,26 @@ reconfigure_ct_zones(struct datapath *dp)
 static void
 reconfigure_datapath_cfgs(const struct ovsrec_open_vswitch *cfg)
 {
-    struct datapath *dp;
+    struct datapath *dp, *next;

-    update_datapath_cfgs(cfg);
+    /* Add new 'datapath's or update existing ones. */
+    for (size_t i = 0; i < cfg->n_datapaths; i++) {
+        struct ovsrec_datapath *dp_cfg = cfg->value_datapaths[i];
+        char *dp_name = cfg->key_datapaths[i];

-    HMAP_FOR_EACH (dp, node, &all_datapaths) {
-        reconfigure_ct_zones(dp);
+        dp = datapath_lookup(dp_name);
+        if (!dp) {
+            dp = datapath_create(dp_name);
+        }
+        dp->last_used = idl_seqno;
+        reconfigure_ct_zones(dp, dp_cfg);
+    }
+
+    /* Purge deleted 'datapath's. */
+    HMAP_FOR_EACH_SAFE (dp, next, node, &all_datapaths) {
+        if (dp->last_used != idl_seqno) {
+            datapath_destroy(dp);
+        }
     }
 }

-- 
2.7.4

<---------- end of patch -------------------------->