[ovs-dev] ovn-controller: Fix group_id allocation.
diff mbox

Message ID 1473231326-16611-1-git-send-email-guru@ovn.org
State Accepted
Headers show

Commit Message

Gurucharan Shetty Sept. 7, 2016, 6:55 a.m. UTC
A bitmap in 'struct group_table' is used to track all the allocated
group_ids.  For every run of logical flows action parsing, we
add 'group_info' structure to a hmap called 'desired_groups'. The
group_id assigned to this group_info either comes from an already
installed 'existing groups' or a new reservation done in the bitmap.

In ofctrl_put(), if there is a backlog, we call ovn_group_table_clear().
This could unreserve a group_id that comes from an already existing group.
This could result in re-use of group_id in the future causing errors while
installed new groups.

This commit fixes the above scenario.

Signed-off-by: Gurucharan Shetty <guru@ovn.org>
---
 include/ovn/actions.h   | 2 ++
 ovn/controller/ofctrl.c | 6 +++++-
 ovn/lib/actions.c       | 3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

Comments

Ben Pfaff Sept. 9, 2016, 8:31 p.m. UTC | #1
On Tue, Sep 06, 2016 at 11:55:26PM -0700, Gurucharan Shetty wrote:
> A bitmap in 'struct group_table' is used to track all the allocated
> group_ids.  For every run of logical flows action parsing, we
> add 'group_info' structure to a hmap called 'desired_groups'. The
> group_id assigned to this group_info either comes from an already
> installed 'existing groups' or a new reservation done in the bitmap.
> 
> In ofctrl_put(), if there is a backlog, we call ovn_group_table_clear().
> This could unreserve a group_id that comes from an already existing group.
> This could result in re-use of group_id in the future causing errors while
> installed new groups.
> 
> This commit fixes the above scenario.
> 
> Signed-off-by: Gurucharan Shetty <guru@ovn.org>

Acked-by: Ben Pfaff <blp@ovn.org>

Patch
diff mbox

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index d1942b3..fbf1f58 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -291,6 +291,8 @@  struct group_info {
     struct hmap_node hmap_node;
     struct ds group;
     uint32_t group_id;
+    bool new_group_id;  /* 'True' if 'group_id' was reserved from
+                         * group_table's 'group_ids' bitmap. */
 };
 
 enum action_opcode {
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 584e260..fe72d79 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -744,7 +744,11 @@  ovn_group_table_clear(struct group_table *group_table, bool existing)
 
     HMAP_FOR_EACH_SAFE (g, next, hmap_node, target_group) {
         hmap_remove(target_group, &g->hmap_node);
-        bitmap_set0(group_table->group_ids, g->group_id);
+        /* Don't unset bitmap for desired group_info if the group_id
+         * was not freshly reserved. */
+        if (existing || g->new_group_id) {
+            bitmap_set0(group_table->group_ids, g->group_id);
+        }
         ds_destroy(&g->group);
         free(g);
     }
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 28b66ed..8ed6ddc 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -974,10 +974,12 @@  encode_CT_LB(const struct ovnact_ct_lb *cl,
             }
         }
 
+        bool new_group_id = false;
         if (!group_id) {
             /* Reserve a new group_id. */
             group_id = bitmap_scan(ep->group_table->group_ids, 0, 1,
                                    MAX_OVN_GROUPS + 1);
+            new_group_id = true;
         }
 
         if (group_id == MAX_OVN_GROUPS + 1) {
@@ -993,6 +995,7 @@  encode_CT_LB(const struct ovnact_ct_lb *cl,
         group_info->group = ds;
         group_info->group_id = group_id;
         group_info->hmap_node.hash = hash;
+        group_info->new_group_id = new_group_id;
 
         hmap_insert(&ep->group_table->desired_groups,
                     &group_info->hmap_node, group_info->hmap_node.hash);