Message ID | 20240118080717.65255-1-amusil@redhat.com |
---|---|
State | Deferred |
Headers | show |
Series | [ovs-dev] controller: Slightly simplify the ct zone assignment | expand |
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. 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 <amusil@redhat.com> > --- > 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); > } >
On Thu, Feb 1, 2024 at 9:06 PM Mark Michelson <mmichels@redhat.com> 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 <amusil@redhat.com> > > --- > > 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); > > } > > > >
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); }
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 <amusil@redhat.com> --- controller/ovn-controller.c | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-)