On Thu, Jul 25, 2024 at 6:22 AM Ales Musil <[email protected]> wrote: > > 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. > > Acked-by: Mark Michelson <[email protected]> > Signed-off-by: Ales Musil <[email protected]>
Acked-by: Numan Siddique <[email protected]> Numan > --- > v6: Rebase on top of latest main. > Add ack from Mark. > v5: Rebase on top of latest main. > Address comments from Dumitru: > Remove the CT_ZONE_STATE_NEW enum variant. > Add wrappers for ct_zone_ctx_init/destroy(). > v4: Rebase on top of latest main. > Address comments from Lorenzo. > v3: Rebase on top of latest main. > v2: Fix NULL ptr deref. > --- > controller/ct-zone.c | 183 ++++++++++++++++++++++-------------- > controller/ct-zone.h | 13 ++- > controller/ofctrl.c | 2 +- > controller/ovn-controller.c | 17 ++-- > controller/physical.c | 17 ++-- > controller/physical.h | 2 +- > 6 files changed, 140 insertions(+), 94 deletions(-) > > diff --git a/controller/ct-zone.c b/controller/ct-zone.c > index e4f66a52a..ab0eec9d0 100644 > --- a/controller/ct-zone.c > +++ b/controller/ct-zone.c > @@ -26,12 +26,28 @@ ct_zone_restore(const struct sbrec_datapath_binding_table > *dp_table, > struct ct_zone_ctx *ctx, const char *name, int zone); > 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); > static int ct_zone_get_snat(const struct sbrec_datapath_binding *dp); > static bool ct_zone_assign_unused(struct ct_zone_ctx *ctx, > const char *zone_name, int *scan_start); > -static bool ct_zone_remove(struct ct_zone_ctx *ctx, > - struct simap_node *ct_zone); > +static bool ct_zone_remove(struct ct_zone_ctx *ctx, const char *name); > +static void ct_zone_add(struct ct_zone_ctx *ctx, const char *name, > + uint16_t zone, bool set_pending); > + > +void > +ct_zone_ctx_init(struct ct_zone_ctx *ctx) > +{ > + shash_init(&ctx->pending); > + shash_init(&ctx->current); > +} > + > +void > +ct_zone_ctx_destroy(struct ct_zone_ctx *ctx) > +{ > + shash_destroy_free_data(&ctx->current); > + shash_destroy_free_data(&ctx->pending); > +} > > void > ct_zones_restore(struct ct_zone_ctx *ctx, > @@ -47,7 +63,8 @@ ct_zones_restore(struct ct_zone_ctx *ctx, > struct ct_zone_pending_entry *ctpe = pending_node->data; > > if (ctpe->add) { > - ct_zone_restore(dp_table, ctx, pending_node->name, ctpe->zone); > + ct_zone_restore(dp_table, ctx, pending_node->name, > + ctpe->ct_zone.zone); > } > } > > @@ -91,7 +108,6 @@ void > ct_zones_update(const struct sset *local_lports, > const struct hmap *local_datapaths, struct ct_zone_ctx *ctx) > { > - struct simap_node *ct_zone; > int scan_start = 1; > const char *user; > struct sset all_users = SSET_INITIALIZER(&all_users); > @@ -132,12 +148,14 @@ ct_zones_update(const struct sset *local_lports, > } > > /* Delete zones that do not exist in above sset. */ > - SIMAP_FOR_EACH_SAFE (ct_zone, &ctx->current) { > - if (!sset_contains(&all_users, ct_zone->name)) { > - ct_zone_remove(ctx, ct_zone); > - } else if (!simap_find(&req_snat_zones, ct_zone->name)) { > - bitmap_set1(unreq_snat_zones_map, ct_zone->data); > - simap_put(&unreq_snat_zones, ct_zone->name, ct_zone->data); > + struct shash_node *node; > + SHASH_FOR_EACH_SAFE (node, &ctx->current) { > + struct ct_zone *ct_zone = node->data; > + if (!sset_contains(&all_users, node->name)) { > + ct_zone_remove(ctx, node->name); > + } else if (!simap_find(&req_snat_zones, node->name)) { > + bitmap_set1(unreq_snat_zones_map, ct_zone->zone); > + simap_put(&unreq_snat_zones, node->name, ct_zone->zone); > } > } > > @@ -152,7 +170,7 @@ ct_zones_update(const struct sset *local_lports, > struct simap_node *unreq_node; > SIMAP_FOR_EACH_SAFE (unreq_node, &unreq_snat_zones) { > if (unreq_node->data == snat_req_node->data) { > - simap_find_and_delete(&ctx->current, unreq_node->name); > + ct_zone_remove(ctx, unreq_node->name); > simap_delete(&unreq_snat_zones, unreq_node); > } > } > @@ -163,26 +181,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); > } > > /* xxx This is wasteful to assign a zone to each port--even if no > @@ -191,7 +195,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 +226,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 +285,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 +307,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 +339,53 @@ 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->zone = zone; > + > + 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 +395,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 = state; > + > + shash_add(pending_ct_zones, name, entry); > + } > + > + *entry = (struct ct_zone_pending_entry) { > + .ct_zone = *zone, > + .state = MIN(entry->state, state), > + .add = add, > + }; > } > > /* Replaces a UUID prefix from 'uuid_zone' (if any) with the > @@ -420,19 +458,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..a7c2011a1 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. */ > @@ -50,12 +54,14 @@ enum ct_zone_pending_state { > }; > > 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; > }; > > +void ct_zone_ctx_init(struct ct_zone_ctx *ctx); > +void ct_zone_ctx_destroy(struct ct_zone_ctx *ctx); > void ct_zones_restore(struct ct_zone_ctx *ctx, > const struct ovsrec_open_vswitch_table *ovs_table, > const struct sbrec_datapath_binding_table *dp_table, > @@ -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 4e30302ea..aee558f9a 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -2188,8 +2188,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); > + ct_zone_ctx_init(&data->ctx); > > return data; > } > @@ -2206,8 +2205,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.pending); > + ct_zone_ctx_destroy(&ct_zones_data->ctx); > } > > static void > @@ -4335,7 +4333,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; > > struct ed_type_northd_options *n_opts = > engine_get_input_data("northd_options", node); > @@ -6060,12 +6058,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 876ceccf1..290a06109 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, > @@ -2116,7 +2117,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, > @@ -2180,7 +2181,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 4dd228cf8..dd4be7041 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.2 > > _______________________________________________ > 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
