[ovs-dev] controller: Slightly simplify the ct zone assignment

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(-)

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);
> >   }
> >
>
>