| Message ID | 20240617065826.76656-3-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 |
Hi Ales, just a nit inline. Other than that: Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > 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 <amusil@redhat.com> > --- > 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) > + 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Jun 21, 2024 at 6:09 PM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > Hi Ales, > > just a nit inline. Other than that: > > Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > 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 <amusil@redhat.com> > > --- > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Thanks, Ales
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; + updated |= ct_zone_handle_port_update(&ct_zones_data->ctx, + t_lport->pb->logical_port, + port_updated, &scan_start); } }
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 <amusil@redhat.com> --- 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(-)