On Thu, Feb 1, 2024 at 9:06 PM Mark Michelson <[email protected]> wrote:
> Hi Ales, > > Thanks for simplifying this. However, I think there is a subtle bug > introduced here. Consider the case where two datapaths, A and B, both > request SNAT zone 0. With the current code, both datapaths will be > assigned zone 0. I think with this change, only one datapath will get > their requested zone. > > Here is a rough outline of the process: > * Datapaths A and B both request zone 0, so both of their requests get > added to the req_snat_nodes simap. > * Next, we iterate through the req_snat_nodes map. > * We inspect A's request first. > * The ct_zone_bitmap doesn't have A's requested zone set. > * We add A to the ct_zones simap and set bit 0 in the > ct_zone_bitmap. > * We inspect B's request next. > * The ct_zone_bitmap has B's requested zone set (since we added > A's request of zone 0 to the bitmap in the last iteration). > * We iterate over the ct_zones simap. We find A's entry. Since > the zone number matches B's request but the datapath name does not match > (A != B), we delete A from the ct_zones simap. > * We add B to the ct_zones simap and set bit 0 in the > ct_zone_bitmap. > > * We iterate through all_users. > * We get to datapath A > * The ct_zones simap does not contain an entry for datapath A. > * We allocate a random zone assignment for datapath A. > * We get to datapath B. > * The ct_zones simap has an entry for datapath B. Nothing to do. > > The result is that A has a random zone assigned, and B has its requested > zone assigned. > > In the current code, this is why the unreq_snat_zones simap is present. > It only represents the datapaths that have not requested CT zones. By > only searching in this simap for existing zone assignments, we don't > accidentally remove a requested zone assignment. We can only unassign a > zone that is auto-assigned. > > Hopefully this makes sense. > Hi Mark, yes this makes sense, I didn't consider the scenario of two datapaths requesting the same SNAT zone. I'll mark this patch as deferred. Thanks, Ales > > On 1/18/24 03:07, Ales Musil wrote: > > There is no need to hold data in separate bitmap and simap as all the > > zones that are already assigned are in the inc-engine sctructures. > > > > Signed-off-by: Ales Musil <[email protected]> > > --- > > controller/ovn-controller.c | 22 +++++----------------- > > 1 file changed, 5 insertions(+), 17 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 856e5e270..bc5605e6b 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -731,8 +731,6 @@ update_ct_zones(const struct sset *local_lports, > > const char *user; > > struct sset all_users = SSET_INITIALIZER(&all_users); > > struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones); > > - unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)]; > > - struct simap unreq_snat_zones = > SIMAP_INITIALIZER(&unreq_snat_zones); > > > > const char *local_lport; > > SSET_FOR_EACH (local_lport, local_lports) { > > @@ -777,9 +775,6 @@ update_ct_zones(const struct sset *local_lports, > > > > bitmap_set0(ct_zone_bitmap, ct_zone->data); > > simap_delete(ct_zones, 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); > > } > > } > > > > @@ -790,19 +785,13 @@ update_ct_zones(const struct sset *local_lports, > > * If so, then they need to give up their assignment since > > * that zone is being explicitly requested now. > > */ > > - if (bitmap_is_set(unreq_snat_zones_map, snat_req_node->data)) { > > - 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(ct_zones, unreq_node->name); > > - simap_delete(&unreq_snat_zones, unreq_node); > > + if (bitmap_is_set(ct_zone_bitmap, snat_req_node->data)) { > > + SIMAP_FOR_EACH_SAFE (ct_zone, ct_zones) { > > + if (ct_zone->data == snat_req_node->data && > > + strcmp(ct_zone->name, snat_req_node->name)) { > > + simap_delete(ct_zones, ct_zone); > > } > > } > > - > > - /* Set this bit to 0 so that if multiple datapaths have > requested > > - * this zone, we don't needlessly double-detect this > condition. > > - */ > > - bitmap_set0(unreq_snat_zones_map, snat_req_node->data); > > } > > > > struct simap_node *node = simap_find(ct_zones, > snat_req_node->name); > > @@ -840,7 +829,6 @@ update_ct_zones(const struct sset *local_lports, > > } > > > > simap_destroy(&req_snat_zones); > > - simap_destroy(&unreq_snat_zones); > > sset_destroy(&all_users); > > } > > > > -- 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
