On Fri, Jun 21, 2024 at 6:09 PM Lorenzo Bianconi < [email protected]> wrote:
> Hi Ales, > > just a nit inline. Other than that: > > Acked-by: Lorenzo Bianconi <[email protected]> > > Hi Lorenzo, thank you for the review. > > > 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]> > > --- > > v3: Rebase on top of latest main. > > --- > > 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 3e37fedb6..e4f66a52a 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); > > @@ -277,12 +241,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) > > { > > @@ -296,6 +254,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 54a9d7a13..2152195c3 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -2261,34 +2261,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, dp)) { > > return false; > > } > > } > > @@ -2333,21 +2306,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; > > probably enum en_tracked_resource_type will never change, but I guess it > would > be more robust to check both TRACKED_RESOURCE_NEW and > TRACKED_RESOURCE_UPDATED > (as it was before) > > > Makes sense, I've changed it to that form in v4. > > + updated |= ct_zone_handle_port_update(&ct_zones_data->ctx, > > + > t_lport->pb->logical_port, > > + port_updated, > &scan_start); > > } > > } > > > > -- > > 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
