| Message ID | 20240725102159.1416553-2-amusil@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add ability to limit CT entries per LS/LR/LSP | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Thu, Jul 25, 2024 at 6:22 AM Ales Musil <amusil@redhat.com> 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 <mmichels@redhat.com> > Signed-off-by: Ales Musil <amusil@redhat.com> Acked-by: Numan Siddique <numans@ovn.org> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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;