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 <[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);
}
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
