> In order to be able to store CT limits for specified zone, store the
> zone inside separate struct instead of simap. This allows to add
> the addition of limit without changing the whole infrastructure again.
>
> This is a preparation step for the CT zone limits.
>
> Signed-off-by: Ales Musil <[email protected]>
> ---
> v3: Rebase on top of latest main.
> v2: Fix NULL ptr deref.
> ---
> controller/ct-zone.c | 171 +++++++++++++++++++++---------------
> controller/ct-zone.h | 13 ++-
> controller/ofctrl.c | 2 +-
> controller/ovn-controller.c | 15 ++--
> controller/physical.c | 17 ++--
> controller/physical.h | 2 +-
> 6 files changed, 128 insertions(+), 92 deletions(-)
>
> diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> index e4f66a52a..95faec2f1 100644
> --- a/controller/ct-zone.c
> +++ b/controller/ct-zone.c
[...]
> @@ -163,26 +167,12 @@ ct_zones_update(const struct sset *local_lports,
> bitmap_set0(unreq_snat_zones_map, snat_req_node->data);
> }
>
> - struct simap_node *node = simap_find(&ctx->current,
> - snat_req_node->name);
> - if (node) {
> - if (node->data != snat_req_node->data) {
> - /* Zone request has changed for this node. delete old entry
> and
> - * create new one*/
> - ct_zone_add_pending(&ctx->pending, CT_ZONE_OF_QUEUED,
> - snat_req_node->data, true,
> - snat_req_node->name);
> - bitmap_set0(ctx->bitmap, node->data);
> - }
> - bitmap_set1(ctx->bitmap, snat_req_node->data);
> - node->data = snat_req_node->data;
> - } else {
> - ct_zone_add_pending(&ctx->pending, CT_ZONE_OF_QUEUED,
> - snat_req_node->data, true,
> - snat_req_node->name);
> - bitmap_set1(ctx->bitmap, snat_req_node->data);
> - simap_put(&ctx->current, snat_req_node->name,
> snat_req_node->data);
> + struct ct_zone *ct_zone = shash_find_data(&ctx->current,
> + snat_req_node->name);
> + if (node && ct_zone->zone != snat_req_node->data) {
> + ct_zone_remove(ctx, snat_req_node->name);
> }
> + ct_zone_add(ctx, snat_req_node->name, snat_req_node->data, true);
IIUC we will perform the same lookup twice here (here and in
ct_zone_remove/ct_zone_add). Can we rearrange ct_zone_add and ct_zone_remove
signatures to avoid it?
> }
>
> /* xxx This is wasteful to assign a zone to each port--even if no
> @@ -191,7 +181,7 @@ ct_zones_update(const struct sset *local_lports,
> /* Assign a unique zone id for each logical port and two zones
> * to a gateway router. */
> SSET_FOR_EACH (user, &all_users) {
> - if (simap_contains(&ctx->current, user)) {
> + if (shash_find(&ctx->current, user)) {
> continue;
> }
>
> @@ -222,7 +212,7 @@ ct_zones_commit(const struct ovsrec_bridge *br_int,
>
> char *user_str = xasprintf("ct-zone-%s", iter->name);
> if (ctzpe->add) {
> - char *zone_str = xasprintf("%"PRId32, ctzpe->zone);
> + char *zone_str = xasprintf("%"PRIu16, ctzpe->ct_zone.zone);
> struct smap_node *node =
> smap_get_node(&br_int->external_ids, user_str);
> if (!node || strcmp(node->value, zone_str)) {
> @@ -281,12 +271,17 @@ ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
> * or not. If so, then fall back to full recompute of
> * ct_zone engine. */
> char *snat_dp_zone_key = alloc_nat_zone_key(name, "snat");
> - struct simap_node *simap_node =
> - simap_find(&ctx->current, snat_dp_zone_key);
> + struct shash_node *node = shash_find(&ctx->current, snat_dp_zone_key);
> free(snat_dp_zone_key);
> - if (!simap_node || simap_node->data != req_snat_zone) {
> - /* There is no entry yet or the requested snat zone has changed.
> - * Trigger full recompute of ct_zones engine. */
> +
> + if (!node) {
> + return false;
> + }
> +
> + struct ct_zone *ct_zone = node->data;
> + if (ct_zone->zone != req_snat_zone) {
> + /* The requested snat zone has changed. Trigger full recompute of
> + * ct_zones engine. */
> return false;
> }
>
> @@ -298,17 +293,24 @@ bool
> ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char *name,
> bool updated, int *scan_start)
> {
> - struct simap_node *ct_zone = simap_find(&ctx->current, name);
> - if (updated && !ct_zone) {
> + struct shash_node *node = shash_find(&ctx->current, name);
> + if (updated && !node) {
> ct_zone_assign_unused(ctx, name, scan_start);
> return true;
> - } else if (!updated && ct_zone_remove(ctx, ct_zone)) {
> + } else if (!updated && node && ct_zone_remove(ctx, node->name)) {
> return true;
> }
>
> return false;
> }
>
> +uint16_t
> +ct_zone_find_zone(const struct shash *ct_zones, const char *name)
> +{
> + struct ct_zone *ct_zone = shash_find_data(ct_zones, name);
> + return ct_zone ? ct_zone->zone : 0;
> +}
> +
>
> static bool
> ct_zone_assign_unused(struct ct_zone_ctx *ctx, const char *zone_name,
> @@ -323,33 +325,55 @@ ct_zone_assign_unused(struct ct_zone_ctx *ctx, const
> char *zone_name,
> }
>
> *scan_start = zone + 1;
> + ct_zone_add(ctx, zone_name, zone, true);
>
> - ct_zone_add_pending(&ctx->pending, CT_ZONE_OF_QUEUED,
> - zone, true, zone_name);
> -
> - bitmap_set1(ctx->bitmap, zone);
> - simap_put(&ctx->current, zone_name, zone);
> return true;
> }
>
> static bool
> -ct_zone_remove(struct ct_zone_ctx *ctx, struct simap_node *ct_zone)
> +ct_zone_remove(struct ct_zone_ctx *ctx, const char *name)
> {
> - if (!ct_zone) {
> + struct shash_node *node = shash_find(&ctx->current, name);
> + if (!node) {
> return false;
> }
>
> - VLOG_DBG("removing ct zone %"PRId32" for '%s'", ct_zone->data,
> - ct_zone->name);
> + struct ct_zone *ct_zone = node->data;
> +
> + VLOG_DBG("removing ct zone %"PRIu16" for '%s'", ct_zone->zone, name);
>
> ct_zone_add_pending(&ctx->pending, CT_ZONE_OF_QUEUED,
> - ct_zone->data, false, ct_zone->name);
> - bitmap_set0(ctx->bitmap, ct_zone->data);
> - simap_delete(&ctx->current, ct_zone);
> + ct_zone, false, name);
> + bitmap_set0(ctx->bitmap, ct_zone->zone);
> + shash_delete(&ctx->current, node);
> + free(ct_zone);
>
> return true;
> }
>
> +static void
> +ct_zone_add(struct ct_zone_ctx *ctx, const char *name, uint16_t zone,
> + bool set_pending)
> +{
> + VLOG_DBG("assigning ct zone %"PRIu16" for '%s'", zone, name);
> +
> + struct ct_zone *ct_zone = shash_find_data(&ctx->current, name);
> + if (!ct_zone) {
> + ct_zone = xmalloc(sizeof *ct_zone);
> + shash_add(&ctx->current, name, ct_zone);
> + }
> +
> + *ct_zone = (struct ct_zone) {
> + .zone = zone,
> + };
nit: can you just do:
ct_zone->zone = zone;
I guess it is more readable
> +
> + if (set_pending) {
> + ct_zone_add_pending(&ctx->pending, CT_ZONE_OF_QUEUED,
> + ct_zone, true, name);
> + }
> + bitmap_set1(ctx->bitmap, zone);
> +}
> +
> static int
> ct_zone_get_snat(const struct sbrec_datapath_binding *dp)
> {
> @@ -359,27 +383,29 @@ ct_zone_get_snat(const struct sbrec_datapath_binding
> *dp)
> static void
> ct_zone_add_pending(struct shash *pending_ct_zones,
> enum ct_zone_pending_state state,
> - int zone, bool add, const char *name)
> + struct ct_zone *zone, bool add, const char *name)
> {
> - VLOG_DBG("%s ct zone %"PRId32" for '%s'",
> - add ? "assigning" : "removing", zone, name);
> -
> - struct ct_zone_pending_entry *pending = xmalloc(sizeof *pending);
> - *pending = (struct ct_zone_pending_entry) {
> - .state = state,
> - .zone = zone,
> - .add = add,
> - };
> -
> /* Its important that we add only one entry for the key 'name'.
> * Replace 'pending' with 'existing' and free up 'existing'.
> * Otherwise, we may end up in a continuous loop of adding
> * and deleting the zone entry in the 'external_ids' of
> * integration bridge.
> */
> - struct ct_zone_pending_entry *existing =
> - shash_replace(pending_ct_zones, name, pending);
> - free(existing);
> + struct ct_zone_pending_entry *entry =
> + shash_find_data(pending_ct_zones, name);
> +
> + if (!entry) {
> + entry = xmalloc(sizeof *entry);
> + entry->state = CT_ZONE_STATE_NEW;
> +
> + shash_add(pending_ct_zones, name, entry);
> + }
> +
> + *entry = (struct ct_zone_pending_entry) {
> + .ct_zone = *zone,
> + .state = MIN(entry->state, state),
> + .add = add,
> + };
same here, I would prefer to use the pointer directly
Regards,
Lorenzo
> }
>
> /* Replaces a UUID prefix from 'uuid_zone' (if any) with the
> @@ -420,19 +446,20 @@ ct_zone_restore(const struct
> sbrec_datapath_binding_table *dp_table,
> VLOG_DBG("ct zone %"PRId32" replace uuid name '%s' with '%s'",
> zone, name, new_name);
>
> + struct ct_zone ct_zone = {
> + .zone = zone,
> + };
> /* Make sure we remove the uuid one in the next OvS DB commit without
> * flush. */
> ct_zone_add_pending(&ctx->pending, CT_ZONE_DB_QUEUED,
> - zone, false, name);
> + &ct_zone, false, name);
> /* Store the zone in OvS DB with name instead of uuid without flush.
> * */
> ct_zone_add_pending(&ctx->pending, CT_ZONE_DB_QUEUED,
> - zone, true, new_name);
> + &ct_zone, true, new_name);
> current_name = new_name;
> }
>
> - simap_put(&ctx->current, current_name, zone);
> - bitmap_set1(ctx->bitmap, zone);
> -
> + ct_zone_add(ctx, current_name, zone, false);
> free(new_name);
> }
> diff --git a/controller/ct-zone.h b/controller/ct-zone.h
> index 889bdf2fc..af68de2d6 100644
> --- a/controller/ct-zone.h
> +++ b/controller/ct-zone.h
> @@ -37,8 +37,12 @@ struct ct_zone_ctx {
> struct shash pending; /* Pending entries,
> * 'struct ct_zone_pending_entry'
> * by name. */
> - struct simap current; /* Current CT zones mapping
> - * (zone id by name). */
> + struct shash current; /* Current CT zones mapping
> + * (struct ct_zone by name). */
> +};
> +
> +struct ct_zone {
> + uint16_t zone;
> };
>
> /* States to move through when a new conntrack zone has been allocated. */
> @@ -47,10 +51,12 @@ enum ct_zone_pending_state {
> CT_ZONE_OF_SENT, /* Sent and waiting for confirmation on flush. */
> CT_ZONE_DB_QUEUED, /* Waiting for DB transaction to open. */
> CT_ZONE_DB_SENT, /* Sent and waiting for confirmation from DB. */
> + CT_ZONE_STATE_NEW, /* Indication that the pending state was just
> + * created. Must remain last in the enum. */
> };
>
> struct ct_zone_pending_entry {
> - int zone;
> + struct ct_zone ct_zone;
> bool add; /* Is the entry being added? */
> ovs_be32 of_xid; /* Transaction id for barrier. */
> enum ct_zone_pending_state state;
> @@ -70,5 +76,6 @@ bool ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
> const struct sbrec_datapath_binding *dp);
> bool ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char *name,
> bool updated, int *scan_start);
> +uint16_t ct_zone_find_zone(const struct shash *ct_zones, const char *name);
>
> #endif /* controller/ct-zone.h */
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 8a19106c2..f9387d375 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -2709,7 +2709,7 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> SHASH_FOR_EACH(iter, pending_ct_zones) {
> struct ct_zone_pending_entry *ctzpe = iter->data;
> if (ctzpe->state == CT_ZONE_OF_QUEUED) {
> - add_ct_flush_zone(ctzpe->zone, &msgs);
> + add_ct_flush_zone(ctzpe->ct_zone.zone, &msgs);
> ctzpe->state = CT_ZONE_OF_SENT;
> ctzpe->of_xid = 0;
> }
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 2152195c3..7e1279237 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2191,7 +2191,7 @@ en_ct_zones_init(struct engine_node *node OVS_UNUSED,
> struct ed_type_ct_zones *data = xzalloc(sizeof *data);
>
> shash_init(&data->ctx.pending);
> - simap_init(&data->ctx.current);
> + shash_init(&data->ctx.current);
>
> return data;
> }
> @@ -2208,7 +2208,7 @@ en_ct_zones_cleanup(void *data)
> {
> struct ed_type_ct_zones *ct_zones_data = data;
>
> - simap_destroy(&ct_zones_data->ctx.current);
> + shash_destroy_free_data(&ct_zones_data->ctx.current);
> shash_destroy_free_data(&ct_zones_data->ctx.pending);
> }
>
> @@ -4334,7 +4334,7 @@ static void init_physical_ctx(struct engine_node *node,
>
> struct ed_type_ct_zones *ct_zones_data =
> engine_get_input_data("ct_zones", node);
> - struct simap *ct_zones = &ct_zones_data->ctx.current;
> + struct shash *ct_zones = &ct_zones_data->ctx.current;
>
> parse_encap_ips(ovs_table, &p_ctx->n_encap_ips, &p_ctx->encap_ips);
> p_ctx->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
> @@ -6054,12 +6054,13 @@ static void
> ct_zone_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
> const char *argv[] OVS_UNUSED, void *ct_zones_)
> {
> - struct simap *ct_zones = ct_zones_;
> + struct shash *ct_zones = ct_zones_;
> struct ds ds = DS_EMPTY_INITIALIZER;
> - struct simap_node *zone;
> + struct shash_node *node;
>
> - SIMAP_FOR_EACH(zone, ct_zones) {
> - ds_put_format(&ds, "%s %d\n", zone->name, zone->data);
> + SHASH_FOR_EACH (node, ct_zones) {
> + struct ct_zone *ct_zone = node->data;
> + ds_put_format(&ds, "%s %d\n", node->name, ct_zone->zone);
> }
>
> unixctl_command_reply(conn, ds_cstr(&ds));
> diff --git a/controller/physical.c b/controller/physical.c
> index 22756810f..082e00eda 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -17,6 +17,7 @@
> #include "binding.h"
> #include "coverage.h"
> #include "byte-order.h"
> +#include "ct-zone.h"
> #include "encaps.h"
> #include "flow.h"
> #include "ha-chassis.h"
> @@ -212,10 +213,10 @@ get_vtep_port(const struct hmap *local_datapaths,
> int64_t tunnel_key)
>
> static struct zone_ids
> get_zone_ids(const struct sbrec_port_binding *binding,
> - const struct simap *ct_zones)
> + const struct shash *ct_zones)
> {
> struct zone_ids zone_ids = {
> - .ct = simap_get(ct_zones, binding->logical_port)
> + .ct = ct_zone_find_zone(ct_zones, binding->logical_port)
> };
>
> const char *name = smap_get(&binding->datapath->external_ids, "name");
> @@ -228,11 +229,11 @@ get_zone_ids(const struct sbrec_port_binding *binding,
> }
>
> char *dnat = alloc_nat_zone_key(name, "dnat");
> - zone_ids.dnat = simap_get(ct_zones, dnat);
> + zone_ids.dnat = ct_zone_find_zone(ct_zones, dnat);
> free(dnat);
>
> char *snat = alloc_nat_zone_key(name, "snat");
> - zone_ids.snat = simap_get(ct_zones, snat);
> + zone_ids.snat = ct_zone_find_zone(ct_zones, snat);
> free(snat);
>
> return zone_ids;
> @@ -631,7 +632,7 @@ put_chassis_mac_conj_id_flow(const struct
> sbrec_chassis_table *chassis_table,
> }
>
> static void
> -put_replace_chassis_mac_flows(const struct simap *ct_zones,
> +put_replace_chassis_mac_flows(const struct shash *ct_zones,
> const struct
> sbrec_port_binding *localnet_port,
> const struct hmap *local_datapaths,
> @@ -1477,7 +1478,7 @@ enforce_tunneling_for_multichassis_ports(
> static void
> consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> enum mf_field_id mff_ovn_geneve,
> - const struct simap *ct_zones,
> + const struct shash *ct_zones,
> const struct sset *active_tunnels,
> const struct hmap *local_datapaths,
> const struct shash *local_bindings,
> @@ -2110,7 +2111,7 @@ mc_ofctrl_add_flow(const struct sbrec_multicast_group
> *mc,
> static void
> consider_mc_group(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> enum mf_field_id mff_ovn_geneve,
> - const struct simap *ct_zones,
> + const struct shash *ct_zones,
> const struct hmap *local_datapaths,
> struct shash *local_bindings,
> struct simap *patch_ofports,
> @@ -2174,7 +2175,7 @@ consider_mc_group(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> continue;
> }
>
> - int zone_id = simap_get(ct_zones, port->logical_port);
> + int zone_id = ct_zone_find_zone(ct_zones, port->logical_port);
> if (zone_id) {
> put_load(zone_id, MFF_LOG_CT_ZONE, 0, 16, &ofpacts);
> }
> diff --git a/controller/physical.h b/controller/physical.h
> index 7fe8ee3c1..8ec90a3ab 100644
> --- a/controller/physical.h
> +++ b/controller/physical.h
> @@ -61,7 +61,7 @@ struct physical_ctx {
> const struct if_status_mgr *if_mgr;
> struct hmap *local_datapaths;
> struct sset *local_lports;
> - const struct simap *ct_zones;
> + const struct shash *ct_zones;
> enum mf_field_id mff_ovn_geneve;
> struct shash *local_bindings;
> struct simap *patch_ofports;
> --
> 2.45.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
