On Mon, May 27, 2024 at 9:00 AM Ales Musil <[email protected]> wrote:
> Move more code into the new ct-zone module and encapsulate > functionality that is strictly related to CT zone handling. > > Signed-off-by: Ales Musil <[email protected]> > --- > controller/ct-zone.c | 156 +++++++++++++++++++++++++----------- > controller/ct-zone.h | 8 +- > controller/ovn-controller.c | 49 ++--------- > 3 files changed, 118 insertions(+), 95 deletions(-) > > diff --git a/controller/ct-zone.c b/controller/ct-zone.c > index 96084fd9e..16452bc2d 100644 > --- a/controller/ct-zone.c > +++ b/controller/ct-zone.c > @@ -27,6 +27,11 @@ ct_zone_restore(const struct > sbrec_datapath_binding_table *dp_table, > static void ct_zone_add_pending(struct shash *pending_ct_zones, > enum ct_zone_pending_state state, > int 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); > > void > ct_zones_restore(struct ct_zone_ctx *ctx, > @@ -82,47 +87,6 @@ ct_zones_restore(struct ct_zone_ctx *ctx, > } > } > > -bool > -ct_zone_assign_unused(struct ct_zone_ctx *ctx, const char *zone_name, > - int *scan_start) > -{ > - /* We assume that there are 64K zones and that we own them all. */ > - int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, MAX_CT_ZONES + 1); > - if (zone == MAX_CT_ZONES + 1) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_WARN_RL(&rl, "exhausted all ct zones"); > - return false; > - } > - > - *scan_start = zone + 1; > - > - 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; > -} > - > -bool > -ct_zone_remove(struct ct_zone_ctx *ctx, const char *name) > -{ > - struct simap_node *ct_zone = simap_find(&ctx->current, name); > - if (!ct_zone) { > - return false; > - } > - > - VLOG_DBG("removing ct zone %"PRId32" for '%s'", ct_zone->data, > - ct_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); > - > - return true; > -} > - > void > ct_zones_update(const struct sset *local_lports, > const struct hmap *local_datapaths, struct ct_zone_ctx > *ctx) > @@ -170,7 +134,7 @@ 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->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); > @@ -276,12 +240,6 @@ ct_zones_commit(const struct ovsrec_bridge *br_int, > } > } > > -int > -ct_zone_get_snat(const struct sbrec_datapath_binding *dp) > -{ > - return smap_get_int(&dp->external_ids, "snat-ct-zone", -1); > -} > - > void > ct_zones_pending_clear_commited(struct shash *pending) > { > @@ -295,6 +253,108 @@ ct_zones_pending_clear_commited(struct shash > *pending) > } > } > > +/* Returns "true" when there is no need for full recompute. */ > +bool > +ct_zone_handle_dp_update(struct ct_zone_ctx *ctx, > + const struct sbrec_datapath_binding *dp) > +{ > + int req_snat_zone = ct_zone_get_snat(dp); > + if (req_snat_zone == -1) { > + /* datapath snat ct zone is not set. This condition will also hit > + * when CMS clears the snat-ct-zone for the logical router. > + * In this case there is no harm in using the previosly specified > + * snat ct zone for this datapath. Also it is hard to know > + * if this option was cleared or if this option is never set. */ > + return true; > + } > + > + const char *name = smap_get(&dp->external_ids, "name"); > + if (!name) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"' skipping" > + "zone check.", UUID_ARGS(&dp->header_.uuid)); > + return true; > + } > + > + /* Check if the requested snat zone has changed for the datapath > + * 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); > + 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. */ > + return false; > + } > + > + return true; > +} > + > +/* Returns "true" if there was an update to the context. */ > +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) { > + ct_zone_assign_unused(ctx, name, scan_start); > + return true; > + } else if (!updated && ct_zone_remove(ctx, ct_zone)) { > + return true; > + } > + > + return false; > +} > + > + > +static bool > +ct_zone_assign_unused(struct ct_zone_ctx *ctx, const char *zone_name, > + int *scan_start) > +{ > + /* We assume that there are 64K zones and that we own them all. */ > + int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, MAX_CT_ZONES + 1); > + if (zone == MAX_CT_ZONES + 1) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "exhausted all ct zones"); > + return false; > + } > + > + *scan_start = zone + 1; > + > + 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) > +{ > + if (!ct_zone) { > + return false; > + } > + > + VLOG_DBG("removing ct zone %"PRId32" for '%s'", ct_zone->data, > + ct_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); > + > + return true; > +} > + > +static int > +ct_zone_get_snat(const struct sbrec_datapath_binding *dp) > +{ > + return smap_get_int(&dp->external_ids, "snat-ct-zone", -1); > +} > + > static void > ct_zone_add_pending(struct shash *pending_ct_zones, > enum ct_zone_pending_state state, > diff --git a/controller/ct-zone.h b/controller/ct-zone.h > index 6b14de935..889bdf2fc 100644 > --- a/controller/ct-zone.h > +++ b/controller/ct-zone.h > @@ -60,15 +60,15 @@ 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, > const struct ovsrec_bridge *br_int); > -bool ct_zone_assign_unused(struct ct_zone_ctx *ctx, const char *zone_name, > - int *scan_start); > -bool ct_zone_remove(struct ct_zone_ctx *ctx, const char *name); > void ct_zones_update(const struct sset *local_lports, > const struct hmap *local_datapaths, > struct ct_zone_ctx *ctx); > void ct_zones_commit(const struct ovsrec_bridge *br_int, > struct shash *pending_ct_zones); > -int ct_zone_get_snat(const struct sbrec_datapath_binding *dp); > void ct_zones_pending_clear_commited(struct shash *pending); > +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); > > #endif /* controller/ct-zone.h */ > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index fc72e5e2c..7b4ca441a 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -2256,34 +2256,7 @@ ct_zones_datapath_binding_handler(struct > engine_node *node, void *data) > return false; > } > > - int req_snat_zone = ct_zone_get_snat(dp); > - if (req_snat_zone == -1) { > - /* datapath snat ct zone is not set. This condition will > also hit > - * when CMS clears the snat-ct-zone for the logical router. > - * In this case there is no harm in using the previosly > specified > - * snat ct zone for this datapath. Also it is hard to know > - * if this option was cleared or if this option is never set. > */ > - continue; > - } > - > - /* Check if the requested snat zone has changed for the datapath > - * or not. If so, then fall back to full recompute of > - * ct_zone engine. */ > - const char *name = smap_get(&dp->external_ids, "name"); > - if (!name) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > - VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"' > skipping" > - "zone check.", UUID_ARGS(&dp->header_.uuid)); > - continue; > - } > - > - char *snat_dp_zone_key = alloc_nat_zone_key(name, "snat"); > - struct simap_node *simap_node = > simap_find(&ct_zones_data->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 (!ct_zone_handle_dp_update(&ct_zones_data->ctx.current, dp)) { > One issue slipped through, it should be "if (!ct_zone_handle_dp_update(&ct_zones_data->ctx, dp)) {" instead. I'll wait for some feedback before posting v3. > return false; > } > } > @@ -2328,21 +2301,11 @@ ct_zones_runtime_data_handler(struct engine_node > *node, void *data) > continue; > } > > - if (t_lport->tracked_type == TRACKED_RESOURCE_NEW || > - t_lport->tracked_type == TRACKED_RESOURCE_UPDATED) { > - if (!simap_contains(&ct_zones_data->ctx.current, > - t_lport->pb->logical_port)) { > - ct_zone_assign_unused(&ct_zones_data->ctx, > - t_lport->pb->logical_port, > - &scan_start); > - updated = true; > - } > - } else if (t_lport->tracked_type == TRACKED_RESOURCE_REMOVED) > { > - if (ct_zone_remove(&ct_zones_data->ctx, > - t_lport->pb->logical_port)) { > - updated = true; > - } > - } > + bool port_updated = > + t_lport->tracked_type != TRACKED_RESOURCE_REMOVED; > + updated |= ct_zone_handle_port_update(&ct_zones_data->ctx, > + > t_lport->pb->logical_port, > + port_updated, > &scan_start); > } > } > > -- > 2.45.0 > > 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
