> On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei <yihung....@gmail.com> wrote:
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 751535249e21..4b4c4d722645 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
>
> +static void
> +ct_timeout_policy_destroy(struct ct_timeout_policy *ct_tp,
> + struct id_pool *tp_ids)
> +{
> + id_pool_free_id(tp_ids, ct_tp->tp_id);
> + simap_destroy(&ct_tp->tp);
> + ovsrcu_postpone(free, ct_tp);
> +}
I assume we're delaying the freeing of 'ct_tp' in case there are people that
still have references to it. Should id_pool_free_id() and simap_destroy() be
called from ovsrcu_postpone()? (I didn't propose a change for this.)
> +static void
> +ct_set_zone_timeout_policy(const char *datapath_type, uint16_t zone,
> + struct simap *timeout_policy)
> +{
> + struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> + datapath_type);
> + if (!backer) {
> + return;
> + }
> +
> + struct ct_timeout_policy *ct_tp =
> ct_timeout_policy_lookup(&backer->ct_tps,
> +
> timeout_policy);
> + if (!ct_tp) {
> + ct_tp = ct_timeout_policy_alloc(timeout_policy, backer->tp_ids);
> + if (ct_tp) {
> + hmap_insert(&backer->ct_tps, &ct_tp->node,
> simap_hash(&ct_tp->tp));
> + ct_add_timeout_policy_to_dpif(backer->dpif, ct_tp);
> + } else {
> + VLOG_ERR_RL(&rl, "failed to allocate timeout policy");
The call to ct_timeout_policy_alloc() will already log this error message if
there was a problem.
> + return;
> + }
> + }
> +
> + struct ct_zone *ct_zone = ct_zone_lookup(&backer->ct_zones, zone);
> + if (ct_zone) {
> + if (ct_zone->ct_tp != ct_tp) {
> + /* Add the new zone timeout pollicy. */
> + struct ct_zone *new_ct_zone = ct_zone_alloc(zone);
> + new_ct_zone->ct_tp = ct_tp;
> + ct_tp->ref_count++;
> + cmap_replace(&backer->ct_zones, &ct_zone->node,
> &new_ct_zone->node,
> + hash_int(zone, 0));
> +
> + /* Deletes the old zone timeout policy. */
> + ct_timeout_policy_unref(backer, ct_zone->ct_tp);
> + ct_zone_destroy(ct_zone);
Is there a reason that a new ct_zone needs to be allocated anew? Couldn't you
just do something like the following?
struct ct_zone *ct_zone = ct_zone_lookup(&backer->ct_zones, zone_id);
if (ct_zone) {
if (ct_zone->ct_tp != ct_tp) {
- /* Add the new zone timeout pollicy. */
- struct ct_zone *new_ct_zone = ct_zone_alloc(zone);
- new_ct_zone->ct_tp = ct_tp;
- ct_tp->ref_count++;
- cmap_replace(&backer->ct_zones, &ct_zone->node, &new_ct_zone->node,
- hash_int(zone, 0));
-
- /* Deletes the old zone timeout policy. */
+ /* Update the zone timeout policy. */
ct_timeout_policy_unref(backer, ct_zone->ct_tp);
- ct_zone_destroy(ct_zone);
+ ct_zone->ct_tp = ct_tp;
+ ct_tp->ref_count++;
}
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 6cc454371dc7..d53c8e2f80a4 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -58,6 +58,7 @@
> #include "tun-metadata.h"
> #include "versions.h"
> #include "vl-mff-map.h"
> +#include "vswitch-idl.h"
I don't think this #include is necessary with these changes.
> + /* Sets conntrack timeout policy specified by 'timeout_policy' to 'zone'
> + * in datapath type 'dp_type'. */
> + void (*ct_set_zone_timeout_policy)(const char *dp_type, uint16_t zone,
> + struct simap *timeout_policy);
> +
> + /* Deletes the timeout policy associated with 'zone' in datapath type
> + * 'dp_type'. */
> + void (*ct_del_zone_timeout_policy)(const char *dp_type, uint16_t zone);
> };
I think it's clearer to use 'zone_id' throughout instead of 'zone'.
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index d921c4ef8d5f..c43f2ac34e67 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -153,9 +153,36 @@ struct aa_mapping {
> char *br_name;
> };
>
> +/* Internal representation of conntrack zone configuration table in OVSDB. */
> +struct ct_zone {
> + uint16_t zone;
I think 'zone_id' is a clearer name for what this is. I'd also suggest using
that through bridge.c.
> + struct simap tp; /* A map from timeout policy attribute to
> + * timeout value. */
> + unsigned int last_used; /* The last idl_seqno that this 'ct_zone'
> used
> + * in OVSDB. This number is used for garbage
> + * collection. */
> + struct hmap_node node; /* Node in 'struct datapath' 'ct_zones'
> + * hmap. */
> +};
> +
> +/* Internal representation of datapath configuration table in OVSDB. */
> +struct datapath {
> + char *type; /* Datapath type. */
> + 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. */
> +};
It really doesn't matter, but I'd suggest making the order of "last_used" and
"node" in the same order.
> +/* All datapath configuartions, indexed by type. */
> +static struct hmap all_datapaths = HMAP_INITIALIZER(&all_datapaths);
I don't think it's necessary to use an hmap to store the datapaths, since the
usual case will be a single datapath and at most two. The hmap code
special-cases a one-node hmap, so there won't be too much overhead usually, but
there is some unnecessary complication and hashing on every lookup. I'll send
out a patch later to switch it to a linked list, and we can see how it compares.
> +/* Replace 'old_tp' by 'new_tp' (destroyed 'new_tp'). Returns true if
> 'old_tp'
> + * and 'new_tp' contains different data, false if they are the same. */
> +static bool
> +update_timeout_policy(struct simap *old_tp, struct simap *new_tp)
> +{
> + bool changed = !simap_equal(old_tp, new_tp);
> + simap_swap(old_tp, new_tp);
> + simap_destroy(new_tp);
> + return changed;
> +}
It seems like unnecessary work to always call simap_swap(). What about only
doing it they're changed?
> +static void
> +reconfigure_ct_zones(struct datapath *dp)
I think it would be a bit more consistent to call this ct_zones_reconfigure()
to match bridge_reconfigure().
> +static void
> +reconfigure_datapath_cfgs(const struct ovsrec_open_vswitch *cfg)
I think it would be a bit more consistent to call this datapath_reconfigure()
to match bridge_reconfigure().
I agree with Darrell's follow-on patch to this function not to depend "dp_cfg"
between database runs.
I would recommend calling datapath_destroy() on all the datapaths on
bridge_exit(). It's not strictly necessary, but it makes it easier to track
down memory leaks if things are cleaned up as much as possible.
Thanks for the patch! Let me know what you think about the questions I posed
above and the incremental below.
--Justin
-=-=-=-=-=-=-=-=-
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 4b4c4d722645..2271f9f48bd8 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5183,12 +5183,12 @@ ct_timeout_policy_unref(struct dpif_backer *backer,
}
static struct ct_zone *
-ct_zone_lookup(const struct cmap *ct_zones, uint16_t zone)
+ct_zone_lookup(const struct cmap *ct_zones, uint16_t zone_id)
{
struct ct_zone *ct_zone;
- CMAP_FOR_EACH_WITH_HASH (ct_zone, node, hash_int(zone, 0), ct_zones) {
- if (ct_zone->zone_id == zone) {
+ CMAP_FOR_EACH_WITH_HASH (ct_zone, node, hash_int(zone_id, 0), ct_zones) {
+ if (ct_zone->zone_id == zone_id) {
return ct_zone;
}
}
@@ -5196,10 +5196,10 @@ ct_zone_lookup(const struct cmap *ct_zones, uint16_t
zone)
}
static struct ct_zone *
-ct_zone_alloc(uint16_t zone)
+ct_zone_alloc(uint16_t zone_id)
{
struct ct_zone *ct_zone = xzalloc(sizeof *ct_zone);
- ct_zone->zone_id = zone;
+ ct_zone->zone_id = zone_id;
return ct_zone;
}
@@ -5240,7 +5240,7 @@ static void
clear_existing_ct_timeout_policies(struct dpif_backer *backer)
{
/* In kernel datapath, when OVS starts, there may be some pre-existing
- * timeout policies in the kernel. To avoid reassign the same timeout
+ * timeout policies in the kernel. To avoid reassigning the same timeout
* policy ids, we dump all the pre-existing timeout policies and keep
* the ids in the pool. Since OVS will not use those timeout policies
* for new datapath flow, we add them to the kill list and remove
@@ -5267,9 +5267,9 @@ clear_existing_ct_timeout_policies(struct dpif_backer
*backer)
static void
ct_zone_config_init(struct dpif_backer *backer)
{
+ backer->tp_ids = id_pool_create(0, MAX_TIMEOUT_POLICY_ID);
cmap_init(&backer->ct_zones);
hmap_init(&backer->ct_tps);
- backer->tp_ids = id_pool_create(0, MAX_TIMEOUT_POLICY_ID);
ovs_list_init(&backer->ct_tp_kill_list);
clear_existing_ct_timeout_policies(backer);
}
@@ -5277,13 +5277,12 @@ ct_zone_config_init(struct dpif_backer *backer)
static void
ct_zone_config_uninit(struct dpif_backer *backer)
{
- struct ct_timeout_policy *ct_tp;
struct ct_zone *ct_zone;
-
CMAP_FOR_EACH (ct_zone, node, &backer->ct_zones) {
ct_zone_remove_and_destroy(backer, ct_zone);
}
+ struct ct_timeout_policy *ct_tp;
HMAP_FOR_EACH_POP (ct_tp, node, &backer->ct_tps) {
ct_timeout_policy_destroy(ct_tp, backer->tp_ids);
}
@@ -5292,9 +5291,9 @@ ct_zone_config_uninit(struct dpif_backer *backer)
ct_timeout_policy_destroy(ct_tp, backer->tp_ids);
}
+ id_pool_destroy(backer->tp_ids);
cmap_destroy(&backer->ct_zones);
hmap_destroy(&backer->ct_tps);
- id_pool_destroy(backer->tp_ids);
}
static void
@@ -5318,7 +5317,7 @@ ct_zone_timeout_policy_sweep(struct dpif_backer *backer)
}
static void
-ct_set_zone_timeout_policy(const char *datapath_type, uint16_t zone,
+ct_set_zone_timeout_policy(const char *datapath_type, uint16_t zone_id,
struct simap *timeout_policy)
{
struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
@@ -5335,35 +5334,29 @@ ct_set_zone_timeout_policy(const char *datapath_type,
uint16_t zone,
hmap_insert(&backer->ct_tps, &ct_tp->node, simap_hash(&ct_tp->tp));
ct_add_timeout_policy_to_dpif(backer->dpif, ct_tp);
} else {
- VLOG_ERR_RL(&rl, "failed to allocate timeout policy");
return;
}
}
- struct ct_zone *ct_zone = ct_zone_lookup(&backer->ct_zones, zone);
+ struct ct_zone *ct_zone = ct_zone_lookup(&backer->ct_zones, zone_id);
if (ct_zone) {
if (ct_zone->ct_tp != ct_tp) {
- /* Add the new zone timeout pollicy. */
- struct ct_zone *new_ct_zone = ct_zone_alloc(zone);
- new_ct_zone->ct_tp = ct_tp;
- ct_tp->ref_count++;
- cmap_replace(&backer->ct_zones, &ct_zone->node, &new_ct_zone->node,
- hash_int(zone, 0));
-
- /* Deletes the old zone timeout policy. */
+ /* Update the zone timeout policy. */
ct_timeout_policy_unref(backer, ct_zone->ct_tp);
- ct_zone_destroy(ct_zone);
+ ct_zone->ct_tp = ct_tp;
+ ct_tp->ref_count++;
}
} else {
- struct ct_zone *new_ct_zone = ct_zone_alloc(zone);
+ struct ct_zone *new_ct_zone = ct_zone_alloc(zone_id);
new_ct_zone->ct_tp = ct_tp;
- cmap_insert(&backer->ct_zones, &new_ct_zone->node, hash_int(zone, 0));
+ cmap_insert(&backer->ct_zones, &new_ct_zone->node,
+ hash_int(zone_id, 0));
ct_tp->ref_count++;
}
}
static void
-ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone)
+ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone_id)
{
struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
datapath_type);
@@ -5371,7 +5364,7 @@ ct_del_zone_timeout_policy(const char *datapath_type,
uint16_t zone)
return;
}
- struct ct_zone *ct_zone = ct_zone_lookup(&backer->ct_zones, zone);
+ struct ct_zone *ct_zone = ct_zone_lookup(&backer->ct_zones, zone_id);
if (ct_zone) {
ct_timeout_policy_unref(backer, ct_zone->ct_tp);
ct_zone_remove_and_destroy(backer, ct_zone);
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index d53c8e2f80a4..c980e6bffff5 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -58,7 +58,6 @@
#include "tun-metadata.h"
#include "versions.h"
#include "vl-mff-map.h"
-#include "vswitch-idl.h"
struct match;
struct ofputil_flow_mod;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index e1fe3814c5ac..b4249b0d8818 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -956,26 +956,26 @@ ofproto_get_flow_restore_wait(void)
/* Connection tracking configuration. */
void
-ofproto_ct_set_zone_timeout_policy(const char *datapath_type, uint16_t zone,
+ofproto_ct_set_zone_timeout_policy(const char *datapath_type, uint16_t zone_id,
struct simap *timeout_policy)
{
datapath_type = ofproto_normalize_type(datapath_type);
const struct ofproto_class *class = ofproto_class_find__(datapath_type);
if (class->ct_set_zone_timeout_policy) {
- class->ct_set_zone_timeout_policy(datapath_type, zone,
+ class->ct_set_zone_timeout_policy(datapath_type, zone_id,
timeout_policy);
}
}
void
-ofproto_ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone)
+ofproto_ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone_id)
{
datapath_type = ofproto_normalize_type(datapath_type);
const struct ofproto_class *class = ofproto_class_find__(datapath_type);
if (class->ct_del_zone_timeout_policy) {
- class->ct_del_zone_timeout_policy(datapath_type, zone);
+ class->ct_del_zone_timeout_policy(datapath_type, zone_id);
}
}
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 13e3d0c74b99..280ee81e37ae 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -155,14 +155,14 @@ struct aa_mapping {
/* Internal representation of conntrack zone configuration table in OVSDB. */
struct ct_zone {
- uint16_t zone;
+ uint16_t zone_id;
struct simap tp; /* A map from timeout policy attribute to
* timeout value. */
+ struct hmap_node node; /* Node in 'struct datapath' 'ct_zones'
+ * hmap. */
unsigned int last_used; /* The last idl_seqno that this 'ct_zone' used
* in OVSDB. This number is used for garbage
* collection. */
- struct hmap_node node; /* Node in 'struct datapath' 'ct_zones'
- * hmap. */
};
/* Internal representation of datapath configuration table in OVSDB. */
@@ -297,6 +297,8 @@ static bool bridge_has_bond_fake_iface(const struct bridge
*,
const char *name);
static bool port_is_bond_fake_iface(const struct port *);
+static void datapath_destroy(struct datapath *dp);
+
static unixctl_cb_func qos_unixctl_show_types;
static unixctl_cb_func qos_unixctl_show;
@@ -527,13 +529,19 @@ bridge_init(const char *remote)
void
bridge_exit(bool delete_datapath)
{
- struct bridge *br, *next_br;
-
if_notifier_destroy(ifnotifier);
seq_destroy(ifaces_changed);
+
+ struct datapath *dp, *next;
+ HMAP_FOR_EACH_SAFE (dp, next, node, &all_datapaths) {
+ datapath_destroy(dp);
+ }
+
+ struct bridge *br, *next_br;
HMAP_FOR_EACH_SAFE (br, next_br, node, &all_bridges) {
bridge_destroy(br, delete_datapath);
}
+
ovsdb_idl_destroy(idl);
}
@@ -623,12 +631,12 @@ get_timeout_policy_from_ovsrec(struct simap *tp,
}
static struct ct_zone *
-ct_zone_lookup(struct hmap *ct_zones, uint16_t zone)
+ct_zone_lookup(struct hmap *ct_zones, uint16_t zone_id)
{
struct ct_zone *ct_zone;
- HMAP_FOR_EACH_WITH_HASH (ct_zone, node, hash_int(zone, 0), ct_zones) {
- if (ct_zone->zone == zone) {
+ HMAP_FOR_EACH_WITH_HASH (ct_zone, node, hash_int(zone_id, 0), ct_zones) {
+ if (ct_zone->zone_id == zone_id) {
return ct_zone;
}
}
@@ -636,11 +644,11 @@ ct_zone_lookup(struct hmap *ct_zones, uint16_t zone)
}
static struct ct_zone *
-ct_zone_alloc(uint16_t zone, struct ovsrec_ct_timeout_policy *tp_cfg)
+ct_zone_alloc(uint16_t zone_id, struct ovsrec_ct_timeout_policy *tp_cfg)
{
struct ct_zone *ct_zone = xzalloc(sizeof *ct_zone);
- ct_zone->zone = zone;
+ ct_zone->zone_id = zone_id;
simap_init(&ct_zone->tp);
get_timeout_policy_from_ovsrec(&ct_zone->tp, tp_cfg);
return ct_zone;
@@ -660,7 +668,9 @@ static bool
update_timeout_policy(struct simap *old_tp, struct simap *new_tp)
{
bool changed = !simap_equal(old_tp, new_tp);
- simap_swap(old_tp, new_tp);
+ if (changed) {
+ simap_swap(old_tp, new_tp);
+ }
simap_destroy(new_tp);
return changed;
}
@@ -691,11 +701,10 @@ datapath_create(const char *type)
static void
datapath_destroy(struct datapath *dp)
{
- struct ct_zone *ct_zone, *next;
-
if (dp) {
+ struct ct_zone *ct_zone, *next;
HMAP_FOR_EACH_SAFE (ct_zone, next, node, &dp->ct_zones) {
- ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone);
+ ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone_id);
ct_zone_remove_and_destroy(dp, ct_zone);
}
@@ -707,29 +716,29 @@ datapath_destroy(struct datapath *dp)
}
static void
-reconfigure_ct_zones(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
+ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
{
struct ct_zone *ct_zone, *next;
/* Add new 'ct_zone's or update existing 'ct_zone's based on the database
* state. */
for (size_t i = 0; i < dp_cfg->n_ct_zones; i++) {
- uint16_t zone = dp_cfg->key_ct_zones[i];
+ uint16_t zone_id = dp_cfg->key_ct_zones[i];
struct ovsrec_ct_zone *zone_cfg = dp_cfg->value_ct_zones[i];
struct ovsrec_ct_timeout_policy *tp_cfg = zone_cfg->timeout_policy;
- ct_zone = ct_zone_lookup(&dp->ct_zones, zone);
+ ct_zone = ct_zone_lookup(&dp->ct_zones, zone_id);
if (ct_zone) {
struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
get_timeout_policy_from_ovsrec(&new_tp, tp_cfg);
if (update_timeout_policy(&ct_zone->tp, &new_tp)) {
- ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone,
+ ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id,
&ct_zone->tp);
}
} else {
- ct_zone = ct_zone_alloc(zone, tp_cfg);
- hmap_insert(&dp->ct_zones, &ct_zone->node, hash_int(zone, 0));
- ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone,
+ ct_zone = ct_zone_alloc(zone_id, tp_cfg);
+ hmap_insert(&dp->ct_zones, &ct_zone->node, hash_int(zone_id, 0));
+ ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id,
&ct_zone->tp);
}
ct_zone->last_used = idl_seqno;
@@ -738,14 +747,14 @@ reconfigure_ct_zones(struct datapath *dp, struct
ovsrec_datapath *dp_cfg)
/* Purge 'ct_zone's no longer found in the database. */
HMAP_FOR_EACH_SAFE (ct_zone, next, node, &dp->ct_zones) {
if (ct_zone->last_used != idl_seqno) {
- ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone);
+ ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone_id);
ct_zone_remove_and_destroy(dp, ct_zone);
}
}
}
static void
-reconfigure_datapath_cfgs(const struct ovsrec_open_vswitch *cfg)
+datapath_reconfigure(const struct ovsrec_open_vswitch *cfg)
{
struct datapath *dp, *next;
@@ -759,7 +768,7 @@ reconfigure_datapath_cfgs(const struct ovsrec_open_vswitch
*cfg)
dp = datapath_create(dp_name);
}
dp->last_used = idl_seqno;
- reconfigure_ct_zones(dp, dp_cfg);
+ ct_zones_reconfigure(dp, dp_cfg);
}
/* Purge deleted 'datapath's. */
@@ -858,7 +867,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
*ovs_cfg)
}
reconfigure_system_stats(ovs_cfg);
- reconfigure_datapath_cfgs(ovs_cfg);
+ datapath_reconfigure(ovs_cfg);
/* Complete the configuration. */
sflow_bridge_number = 0;
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
