[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