On Fri, Jun 21, 2024 at 6:33 PM Lorenzo Bianconi < [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. > > > > 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 > > [...] > Hi Lorenzo, thank you for the review. > > @@ -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? > I'm not sure if it's worth changing the signature just for this single use case. I don't think that performance is a concern as this gets hit only by the requested zones which is the minimum of cases, would you agree? > > > } > > > > /* 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 > Changed in v4. > > > + > > + 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 > This has the advantage that it will zero out anything that is not specified within that designated initializer. > > 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 > > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
