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

Justin Pettit jpettit at ovn.org
Wed Aug 7 04:46:11 UTC 2019


> On Aug 1, 2019, at 3:07 PM, Yi-Hung Wei <yihung.wei at gmail.com> wrote:
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 751535249e21..6336494e0bc8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> 
> +static struct ct_timeout_policy *
> +ct_timeout_policy_alloc(struct ovsrec_ct_timeout_policy *tp_cfg,
> +                        struct id_pool *tp_ids)

As we discussed this in person, bringing the OVSDB configuration into ofproto-dpif is a pretty different model than we've used before.  I think it would be better to abstract the config and carry it through the ofproto layer.  As that will require a pretty significant restructuring of this patch, I'm going to give this a mostly cursory review.

> +{
> +    struct ct_timeout_policy *tp;
> +    size_t i;

This can be declared in a tighter scope.

> +static void
> +ct_timeout_policy_destroy(struct dpif_backer *backer,
> +                          struct ct_timeout_policy *tp)
> +{
> +    id_pool_free_id(backer->timeout_policy_ids, tp->cdtp.id);
> +    ovsrcu_postpone(free, tp);
> +}

I don't think there's currently a way for this to happen, but I think it might be safer to make sure that this entry is no longer on the 'ct_tps' or 'ct_tp_kill_list'  

> +static void
> +init_ct_zone_timeout_policy(struct dpif_backer *backer)

I think this function name would be clearer if policy were plural.

> +{
> ...
> +    /* Remove existing timeout policy from datapath. */
> +    struct ct_dpif_timeout_policy cdtp;
> +    struct ct_timeout_policy *tp;

This can be declared in a tighter scope.

> +static void
> +destroy_ct_zone_timeout_policy(struct dpif_backer *backer)

I think this function name would be clearer if policy were plural.

> +{
> +    struct ct_timeout_policy *tp;
> +    struct ct_zone *zone;
> +
> +    CMAP_FOR_EACH (zone, node, &backer->ct_zones) {
> +        ct_zone_remove_and_destroy(backer, zone);
> +    }
> +
> +    CMAP_FOR_EACH (tp, node, &backer->ct_tps) {
> +        cmap_remove(&backer->ct_tps, &tp->node, uuid_hash(&tp->uuid));
> +        ct_timeout_policy_destroy(backer, tp);
> +    }
> +
> +    cmap_destroy(&backer->ct_zones);
> +    cmap_destroy(&backer->ct_tps);
> +    id_pool_destroy(backer->timeout_policy_ids);
> +}

Don't we also need to free the items on 'ct_tp_kill_list'?

> +static void
> +ct_zone_timeout_policy_sweep(const struct ofproto *ofproto)
> +{
> +    struct dpif_backer *backer = ofproto_dpif_cast(ofproto)->backer;
> +    struct ct_timeout_policy *tp;
> +    int err;
> +
> +    if (!ovs_list_is_empty(&backer->ct_tp_kill_list)) {
> +        LIST_FOR_EACH (tp, list_node, &backer->ct_tp_kill_list) {
> +            err = ct_dpif_del_timeout_policy(backer->dpif, tp->cdtp.id);
> +            if (!err) {
> +                ovs_list_remove(&tp->list_node);

Should this loop be using LIST_FOR_EACH_SAFE(), since it looks like it can be modified?

> +static void
> +ct_zone_timeout_policy_reconfig(const struct ofproto *ofproto,
> +                                const struct ovsrec_datapath *dp_cfg,
> +                                unsigned int idl_seqno)
> +{
> +    struct dpif_backer *backer = ofproto_dpif_cast(ofproto)->backer;
> +    struct ovsrec_ct_timeout_policy *tp_cfg;
> +    struct ovsrec_ct_zone *zone_cfg;
> +    struct ct_timeout_policy *tp;
> +    struct ct_zone *zone;
> +    uint16_t zone_id;
> +    bool new_zone;
> +    size_t i;

Almost all of these can be declared in a tighter scope.

> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 7907d4bfb416..41e07f0ee23e 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> 
> @@ -1872,6 +1873,13 @@ struct ofproto_class {
>     /* Flushes the connection tracking tables. If 'zone' is not NULL,
>      * only deletes connections in '*zone'. */
>     void (*ct_flush)(const struct ofproto *, const uint16_t *zone);
> +
> +    /* Reconfigures the zone-based conntrack tiemout policy based on the

s/tiemout/timeout/

> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 2976771aeaba..39b7de7d8182 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> 
> static void
> +reconfigure_conntrack_timeout_policy(const struct ovsrec_open_vswitch *cfg)

This function uses "conntrack" in the name and others use "ct".  It would be nice to be consistent.  I would lean towards "ct", since it's shorter and used elsewhere in the code.

As I mentioned, I'll do a more thorough review of v3.

Thanks,

--Justin


-=-=-=-=-=-=-=-=-=-


diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 6336494e0bc8..7a7d72c89440 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -169,7 +169,7 @@ struct ct_timeout_policy {
 struct ct_zone {
     uint16_t id;
     unsigned int last_used_seqno;
-    struct uuid tp_uuid;            /* uuid that identifies a timeout policy in
+    struct uuid tp_uuid;            /* UUID that identifies a timeout policy in
                                      * struct dpif_backer's "ct_tps"  cmap. */
     struct cmap_node node;          /* Element in struct dpif_backer's
                                      * "ct_zones" cmap. */
@@ -215,8 +215,8 @@ static struct hmap all_ofproto_dpifs_by_uuid =
 
 static bool ofproto_use_tnl_push_pop = true;
 static void ofproto_unixctl_init(void);
-static void destroy_ct_zone_timeout_policy(struct dpif_backer *backer);
-static void init_ct_zone_timeout_policy(struct dpif_backer *backer);
+static void destroy_ct_zone_timeout_policies(struct dpif_backer *backer);
+static void init_ct_zone_timeout_policies(struct dpif_backer *backer);
 
 static inline struct ofproto_dpif *
 ofproto_dpif_cast(const struct ofproto *ofproto)
@@ -704,7 +704,7 @@ close_dpif_backer(struct dpif_backer *backer, bool del)
     }
     dpif_close(backer->dpif);
     id_pool_destroy(backer->meter_ids);
-    destroy_ct_zone_timeout_policy(backer);
+    destroy_ct_zone_timeout_policies(backer);
     free(backer);
 }
 
@@ -835,7 +835,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp)
         backer->meter_ids = NULL;
     }
 
-    init_ct_zone_timeout_policy(backer);
+    init_ct_zone_timeout_policies(backer);
 
     /* Make a pristine snapshot of 'support' into 'boottime_support'.
      * 'boottime_support' can be checked to prevent 'support' to be changed
@@ -5160,12 +5160,9 @@ static struct ct_timeout_policy *
 ct_timeout_policy_alloc(struct ovsrec_ct_timeout_policy *tp_cfg,
                         struct id_pool *tp_ids)
 {
-    struct ct_timeout_policy *tp;
-    size_t i;
-
-    tp = xzalloc(sizeof *tp);
+    struct ct_timeout_policy *tp = xzalloc(sizeof *tp);
     tp->uuid = tp_cfg->header_.uuid;
-    for (i = 0; i < tp_cfg->n_timeouts; i++) {
+    for (size_t i = 0; i < tp_cfg->n_timeouts; i++) {
         ct_dpif_set_timeout_policy_attr_by_name(&tp->cdtp,
             tp_cfg->key_timeouts[i], tp_cfg->value_timeouts[i]);
     }
@@ -5201,27 +5198,25 @@ ct_timeout_policy_update(struct ovsrec_ct_timeout_policy *tp_cfg,
 }
 
 static void
-init_ct_zone_timeout_policy(struct dpif_backer *backer)
+init_ct_zone_timeout_policies(struct dpif_backer *backer)
 {
     cmap_init(&backer->ct_zones);
     cmap_init(&backer->ct_tps);
     backer->timeout_policy_ids = id_pool_create(0, MAX_TIMEOUT_POLICY_ID);
     ovs_list_init(&backer->ct_tp_kill_list);
 
-    /* Remove existing timeout policy from datapath. */
-    struct ct_dpif_timeout_policy cdtp;
-    struct ct_timeout_policy *tp;
+    /* Remove existing timeout policies from the datapath. */
     void *state;
-    int err;
 
-    err = ct_dpif_timeout_policy_dump_start(backer->dpif, &state);
+    int err = ct_dpif_timeout_policy_dump_start(backer->dpif, &state);
     if (err) {
         return;
     }
 
+    struct ct_dpif_timeout_policy cdtp;
     while (!(err = ct_dpif_timeout_policy_dump_next(backer->dpif, state,
                                                     &cdtp))) {
-        tp = xzalloc(sizeof *tp);
+        struct ct_timeout_policy *tp = xzalloc(sizeof *tp);
         tp->cdtp = cdtp;
         id_pool_add(backer->timeout_policy_ids, cdtp.id);
         ovs_list_insert(&backer->ct_tp_kill_list, &tp->list_node);
@@ -5231,7 +5226,7 @@ init_ct_zone_timeout_policy(struct dpif_backer *backer)
 }
 
 static void
-destroy_ct_zone_timeout_policy(struct dpif_backer *backer)
+destroy_ct_zone_timeout_policies(struct dpif_backer *backer)
 {
     struct ct_timeout_policy *tp;
     struct ct_zone *zone;
@@ -5245,6 +5240,10 @@ destroy_ct_zone_timeout_policy(struct dpif_backer *backer)
         ct_timeout_policy_destroy(backer, tp);
     }
 
+    LIST_FOR_EACH_POP (tp, list_node, &backer->ct_tp_kill_list) {
+        ct_timeout_policy_destroy(backer, tp);
+    }
+
     cmap_destroy(&backer->ct_zones);
     cmap_destroy(&backer->ct_tps);
     id_pool_destroy(backer->timeout_policy_ids);
@@ -5280,11 +5279,11 @@ static void
 ct_zone_timeout_policy_sweep(const struct ofproto *ofproto)
 {
     struct dpif_backer *backer = ofproto_dpif_cast(ofproto)->backer;
-    struct ct_timeout_policy *tp;
     int err;
 
     if (!ovs_list_is_empty(&backer->ct_tp_kill_list)) {
-        LIST_FOR_EACH (tp, list_node, &backer->ct_tp_kill_list) {
+        struct ct_timeout_policy *tp, *next;
+        LIST_FOR_EACH_SAFE (tp, next, list_node, &backer->ct_tp_kill_list) {
             err = ct_dpif_del_timeout_policy(backer->dpif, tp->cdtp.id);
             if (!err) {
                 ovs_list_remove(&tp->list_node);
@@ -5303,19 +5302,14 @@ ct_zone_timeout_policy_reconfig(const struct ofproto *ofproto,
                                 unsigned int idl_seqno)
 {
     struct dpif_backer *backer = ofproto_dpif_cast(ofproto)->backer;
-    struct ovsrec_ct_timeout_policy *tp_cfg;
-    struct ovsrec_ct_zone *zone_cfg;
-    struct ct_timeout_policy *tp;
-    struct ct_zone *zone;
-    uint16_t zone_id;
-    bool new_zone;
-    size_t i;
 
-    for (i = 0; i < dp_cfg->n_ct_zones; i++) {
+    for (size_t i = 0; i < dp_cfg->n_ct_zones; i++) {
         /* Update ct_zone config. */
-        zone_cfg = dp_cfg->value_ct_zones[i];
-        zone_id = dp_cfg->key_ct_zones[i];
-        zone = ct_zone_lookup(&backer->ct_zones, zone_id);
+        bool new_zone;
+
+        struct ovsrec_ct_zone *zone_cfg = dp_cfg->value_ct_zones[i];
+        uint16_t zone_id = dp_cfg->key_ct_zones[i];
+        struct ct_zone *zone = ct_zone_lookup(&backer->ct_zones, zone_id);
         if (!zone) {
             new_zone = true;
             zone = ct_zone_alloc(zone_id);
@@ -5325,6 +5319,9 @@ ct_zone_timeout_policy_reconfig(const struct ofproto *ofproto,
         zone->last_used_seqno = idl_seqno;
 
         /* Update timeout policy. */
+        struct ovsrec_ct_timeout_policy *tp_cfg;
+        struct ct_timeout_policy *tp;
+
         tp_cfg = zone_cfg->timeout_policy;
         tp = ct_timeout_policy_lookup(&backer->ct_tps, &tp_cfg->header_.uuid);
         if (!tp) {
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 170edf0e0e70..fb7b97a3b1bc 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -245,14 +245,14 @@ struct dpif_backer {
     /* Meter. */
     struct id_pool *meter_ids;     /* Datapath meter allocation. */
 
-    /* Connection tracking */
-    struct id_pool  *timeout_policy_ids;    /* Datapath timeout policy id
+    /* Connection tracking. */
+    struct id_pool *timeout_policy_ids;     /* Datapath timeout policy id
                                              * allocation. */
     struct cmap ct_zones;                   /* "struct ct_zone"s indexed by
                                              * zone id. */
     struct cmap ct_tps;                     /* "struct ct_timeout_policy"s
                                              * indexed by uuid. */
-    struct ovs_list ct_tp_kill_list;        /* a list of timeout policy to be
+    struct ovs_list ct_tp_kill_list;        /* A list of timeout policy to be
                                              * deleted. */
 
     /* Version string of the datapath stored in OVSDB. */
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 41e07f0ee23e..da151cf24072 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1874,11 +1874,11 @@ struct ofproto_class {
      * only deletes connections in '*zone'. */
     void (*ct_flush)(const struct ofproto *, const uint16_t *zone);
 
-    /* Reconfigures the zone-based conntrack tiemout policy based on the
-     * ovsdb configuration. */
+    /* Reconfigures the zone-based conntrack timeout policy based on the
+     * OVSDB configuration. */
     void (*ct_zone_timeout_policy_reconfig)(const struct ofproto *ofproto_,
             const struct ovsrec_datapath *dp_cfg, unsigned int idl_seqno);
-    /* Cleans up the to be deleted timeout policy in the pending kill list. */
+    /* Cleans up the to-be-deleted timeout policy in the pending kill list. */
     void (*ct_zone_timeout_policy_sweep)(const struct ofproto *ofproto_);
 };
 
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 39b7de7d8182..919d0394d566 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -593,7 +593,7 @@ config_ofproto_types(const struct smap *other_config)
 }
 
 static void
-reconfigure_conntrack_timeout_policy(const struct ovsrec_open_vswitch *cfg)
+reconfigure_ct_timeout_policy(const struct ovsrec_open_vswitch *cfg)
 {
     struct bridge *br;
     int i;
@@ -708,7 +708,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
     }
 
     reconfigure_system_stats(ovs_cfg);
-    reconfigure_conntrack_timeout_policy(ovs_cfg);
+    reconfigure_ct_timeout_policy(ovs_cfg);
 
     /* Complete the configuration. */
     sflow_bridge_number = 0;








More information about the dev mailing list