diff mbox series

[ovs-dev,v5,1/2] ovn-controller: Add extend_table instead of group_table to expand meter.

Message ID 20171206043240.6980-2-ligs@dtdream.com
State Changes Requested
Headers show
Series ovn: OVN Support QoS meter | expand

Commit Message

Guoshuai Li Dec. 6, 2017, 4:32 a.m. UTC
The structure and function of the group table and meter table are similar,
refactoring code is used to extend for add the meter table.
The following function as lib: table init/destroy/clear,
install contents from desired, remove contents from existing,
Move the contents of desired to existing.

Signed-off-by: Guoshuai Li <ligs@dtdream.com>
---
 include/ovn/actions.h           |  21 +----
 ovn/controller/lflow.c          |   8 +-
 ovn/controller/lflow.h          |   4 +-
 ovn/controller/ofctrl.c         | 180 +++++++++++++-----------------------
 ovn/controller/ofctrl.h         |   7 +-
 ovn/controller/ovn-controller.c |  20 +---
 ovn/lib/actions.c               |  58 ++----------
 ovn/lib/automake.mk             |   2 +
 ovn/lib/extend-table.c          | 198 ++++++++++++++++++++++++++++++++++++++++
 ovn/lib/extend-table.h          |  69 ++++++++++++++
 tests/test-ovn.c                |   8 +-
 11 files changed, 356 insertions(+), 219 deletions(-)
 create mode 100644 ovn/lib/extend-table.c
 create mode 100644 ovn/lib/extend-table.h

Comments

Ben Pfaff Jan. 24, 2018, 1:42 a.m. UTC | #1
On Wed, Dec 06, 2017 at 12:32:39PM +0800, Guoshuai Li wrote:
> The structure and function of the group table and meter table are similar,
> refactoring code is used to extend for add the meter table.
> The following function as lib: table init/destroy/clear,
> install contents from desired, remove contents from existing,
> Move the contents of desired to existing.
> 
> Signed-off-by: Guoshuai Li <ligs@dtdream.com>

Thanks a lot for working on this.  I mostly like the new approach.  I do
have one nontrivial request, which is to remove the use of the callback
functions.  Usually callback functions make code harder to write and
harder to understand because of how they break up code in unnatural
ways.  In this case, I think that we can just substitute iterator
macros, which I generally find easier on all accounts.

I posted a 3-patch series that shows the direction that I'm thinking
about.  The first patch is just your patch #1.  The second patch is an
example of what I mean about using an iterator instead of a callback
function.  I only removed the 'create' callback, not the 'remove'
callback, but I'd want both to be removed for the final commit.  The
third patch is just trivia to make the style better match the OVS coding
style.

The series is here:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=25039

Are you willing to revise the series in this direction?

Thanks,

Ben.
Guoshuai Li Jan. 24, 2018, 12:46 p.m. UTC | #2
> On Wed, Dec 06, 2017 at 12:32:39PM +0800, Guoshuai Li wrote:
>> The structure and function of the group table and meter table are similar,
>> refactoring code is used to extend for add the meter table.
>> The following function as lib: table init/destroy/clear,
>> install contents from desired, remove contents from existing,
>> Move the contents of desired to existing.
>>
>> Signed-off-by: Guoshuai Li <ligs@dtdream.com>
> Thanks a lot for working on this.  I mostly like the new approach.  I do
> have one nontrivial request, which is to remove the use of the callback
> functions.  Usually callback functions make code harder to write and
> harder to understand because of how they break up code in unnatural
> ways.  In this case, I think that we can just substitute iterator
> macros, which I generally find easier on all accounts.
>
> I posted a 3-patch series that shows the direction that I'm thinking
> about.  The first patch is just your patch #1.  The second patch is an
> example of what I mean about using an iterator instead of a callback
> function.  I only removed the 'create' callback, not the 'remove'
> callback, but I'd want both to be removed for the final commit.  The
> third patch is just trivia to make the style better match the OVS coding
> style.
>
> The series is here:
> https://patchwork.ozlabs.org/project/openvswitch/list/?series=25039
>
> Are you willing to revise the series in this direction?
>
> Thanks,
>
> Ben.
Hello Ben,

Thank you for your ideas.
I refactored the code based on your patch:
https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343646.html
Please help review again, thanks.
diff mbox series

Patch

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 85a484ffa..ea90dbb2a 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -31,6 +31,7 @@  struct lexer;
 struct ofpbuf;
 struct shash;
 struct simap;
+struct ovn_extend_table;
 
 /* List of OVN logical actions.
  *
@@ -338,24 +339,6 @@  void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len);
 OVNACTS
 #undef OVNACT
 
-#define MAX_OVN_GROUPS 65535
-
-struct group_table {
-    unsigned long *group_ids;  /* Used as a bitmap with value set
-                                * for allocated group ids in either
-                                * desired_groups or existing_groups. */
-    struct hmap desired_groups;
-    struct hmap existing_groups;
-};
-
-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 {
     /* "arp { ...actions... }".
      *
@@ -505,7 +488,7 @@  struct ovnact_encode_params {
     bool is_gateway_router;
 
     /* A struct to figure out the group_id for group actions. */
-    struct group_table *group_table;
+    struct ovn_extend_table *group_table;
 
     /* OVN maps each logical flow table (ltable), one-to-one, onto a physical
      * OpenFlow flow table (ptable).  A number of parameters describe this
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index a62ec6ebe..3d990c49c 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -61,7 +61,7 @@  static void consider_logical_flow(struct controller_ctx *ctx,
                                   const struct chassis_index *chassis_index,
                                   const struct sbrec_logical_flow *lflow,
                                   const struct hmap *local_datapaths,
-                                  struct group_table *group_table,
+                                  struct ovn_extend_table *group_table,
                                   const struct sbrec_chassis *chassis,
                                   struct hmap *dhcp_opts,
                                   struct hmap *dhcpv6_opts,
@@ -143,7 +143,7 @@  static void
 add_logical_flows(struct controller_ctx *ctx,
                   const struct chassis_index *chassis_index,
                   const struct hmap *local_datapaths,
-                  struct group_table *group_table,
+                  struct ovn_extend_table *group_table,
                   const struct sbrec_chassis *chassis,
                   const struct shash *addr_sets,
                   struct hmap *flow_table,
@@ -190,7 +190,7 @@  consider_logical_flow(struct controller_ctx *ctx,
                       const struct chassis_index *chassis_index,
                       const struct sbrec_logical_flow *lflow,
                       const struct hmap *local_datapaths,
-                      struct group_table *group_table,
+                      struct ovn_extend_table *group_table,
                       const struct sbrec_chassis *chassis,
                       struct hmap *dhcp_opts,
                       struct hmap *dhcpv6_opts,
@@ -434,7 +434,7 @@  lflow_run(struct controller_ctx *ctx,
           const struct sbrec_chassis *chassis,
           const struct chassis_index *chassis_index,
           const struct hmap *local_datapaths,
-          struct group_table *group_table,
+          struct ovn_extend_table *group_table,
           const struct shash *addr_sets,
           struct hmap *flow_table,
           struct sset *active_tunnels,
diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h
index bfb7415e2..087b0ed8d 100644
--- a/ovn/controller/lflow.h
+++ b/ovn/controller/lflow.h
@@ -37,7 +37,7 @@ 
 
 struct chassis_index;
 struct controller_ctx;
-struct group_table;
+struct ovn_extend_table;
 struct hmap;
 struct sbrec_chassis;
 struct simap;
@@ -66,7 +66,7 @@  void lflow_run(struct controller_ctx *,
                const struct sbrec_chassis *chassis,
                const struct chassis_index *,
                const struct hmap *local_datapaths,
-               struct group_table *group_table,
+               struct ovn_extend_table *group_table,
                const struct shash *addr_sets,
                struct hmap *flow_table,
                struct sset *active_tunnels,
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 90afddbb3..24894cb64 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -36,6 +36,7 @@ 
 #include "openvswitch/vlog.h"
 #include "ovn-controller.h"
 #include "ovn/actions.h"
+#include "ovn/lib/extend-table.h"
 #include "openvswitch/poll-loop.h"
 #include "physical.h"
 #include "openvswitch/rconn.h"
@@ -68,6 +69,9 @@  static char *ovn_flow_to_string(const struct ovn_flow *);
 static void ovn_flow_log(const struct ovn_flow *, const char *action);
 static void ovn_flow_destroy(struct ovn_flow *);
 
+static void add_group(uint32_t table_id, struct ds *ds, struct ovs_list *msgs);
+static void delete_group(uint32_t table_id, struct ovs_list *msgs);
+
 /* OpenFlow connection to the switch. */
 static struct rconn *swconn;
 
@@ -131,7 +135,7 @@  static struct rconn_packet_counter *tx_counter;
 static struct hmap installed_flows;
 
 /* A reference to the group_table. */
-static struct group_table *groups;
+static struct ovn_extend_table *groups;
 
 /* MFF_* field ID for our Geneve option.  In S_TLV_TABLE_MOD_SENT, this is
  * the option we requested (we don't know whether we obtained it yet).  In
@@ -150,7 +154,7 @@  static void ovn_flow_table_destroy(struct hmap *flow_table);
 static void ofctrl_recv(const struct ofp_header *, enum ofptype);
 
 void
-ofctrl_init(struct group_table *group_table)
+ofctrl_init(struct ovn_extend_table *group_table)
 {
     swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION);
     tx_counter = rconn_packet_counter_create();
@@ -158,6 +162,8 @@  ofctrl_init(struct group_table *group_table)
     ovs_list_init(&flow_updates);
     ovn_init_symtab(&symtab);
     groups = group_table;
+    groups->create = add_group;
+    groups->remove = delete_group;
 }
 
 /* S_NEW, for a new connection.
@@ -385,7 +391,7 @@  run_S_CLEAR_FLOWS(void)
 
     /* Clear existing groups, to match the state of the switch. */
     if (groups) {
-        ovn_group_table_clear(groups, true);
+        ovn_extend_table_clear(groups, true);
     }
 
     /* All flow updates are irrelevant now. */
@@ -747,44 +753,6 @@  add_flow_mod(struct ofputil_flow_mod *fm, struct ovs_list *msgs)
 
 /* group_table. */
 
-/* Finds and returns a group_info in 'existing_groups' whose key is identical
- * to 'target''s key, or NULL if there is none. */
-static struct group_info *
-ovn_group_lookup(struct hmap *exisiting_groups,
-                 const struct group_info *target)
-{
-    struct group_info *e;
-
-    HMAP_FOR_EACH_WITH_HASH(e, hmap_node, target->hmap_node.hash,
-                            exisiting_groups) {
-        if (e->group_id == target->group_id) {
-            return e;
-        }
-   }
-    return NULL;
-}
-
-/* Clear either desired_groups or existing_groups in group_table. */
-void
-ovn_group_table_clear(struct group_table *group_table, bool existing)
-{
-    struct group_info *g, *next;
-    struct hmap *target_group = existing
-                                ? &group_table->existing_groups
-                                : &group_table->desired_groups;
-
-    HMAP_FOR_EACH_SAFE (g, next, hmap_node, target_group) {
-        hmap_remove(target_group, &g->hmap_node);
-        /* 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);
-    }
-}
-
 static struct ofpbuf *
 encode_group_mod(const struct ofputil_group_mod *gm)
 {
@@ -797,6 +765,54 @@  add_group_mod(const struct ofputil_group_mod *gm, struct ovs_list *msgs)
     struct ofpbuf *msg = encode_group_mod(gm);
     ovs_list_push_back(msgs, &msg->list_node);
 }
+
+static void
+add_group(uint32_t table_id, struct ds *ds, struct ovs_list *msgs) {
+    /* Create and install new group. */
+    struct ofputil_group_mod gm;
+    enum ofputil_protocol usable_protocols;
+    char *error;
+    struct ds group_string = DS_EMPTY_INITIALIZER;
+    ds_put_format(&group_string, "group_id=%"PRIu32",%s",
+                  table_id, ds_cstr(ds));
+
+    error = parse_ofp_group_mod_str(&gm, OFPGC11_ADD, ds_cstr(&group_string),
+                                    NULL, &usable_protocols);
+    if (!error) {
+        add_group_mod(&gm, msgs);
+    } else {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+        VLOG_ERR_RL(&rl, "new group %s %s", error,
+                    ds_cstr(&group_string));
+        free(error);
+    }
+    ds_destroy(&group_string);
+    ofputil_uninit_group_mod(&gm);
+}
+
+static void
+delete_group(uint32_t table_id, struct ovs_list *msgs) {
+    /* Delete the group. */
+    struct ofputil_group_mod gm;
+    enum ofputil_protocol usable_protocols;
+    char *error;
+    struct ds group_string = DS_EMPTY_INITIALIZER;
+    ds_put_format(&group_string, "group_id=%"PRIu32"", table_id);
+
+    error = parse_ofp_group_mod_str(&gm, OFPGC11_DELETE,
+                                    ds_cstr(&group_string), NULL,
+                                    &usable_protocols);
+    if (!error) {
+        add_group_mod(&gm, msgs);
+    } else {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+        VLOG_ERR_RL(&rl, "Error deleting group %"PRIu32": %s",
+                    table_id, error);
+        free(error);
+    }
+    ds_destroy(&group_string);
+    ofputil_uninit_group_mod(&gm);
+}
 
 static void
 add_ct_flush_zone(uint16_t zone_id, struct ovs_list *msgs)
@@ -844,7 +860,7 @@  ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
 {
     if (!ofctrl_can_put()) {
         ovn_flow_table_clear(flow_table);
-        ovn_group_table_clear(groups, false);
+        ovn_extend_table_clear(groups, false);
         return;
     }
 
@@ -862,34 +878,7 @@  ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
         }
     }
 
-    /* Iterate through all the desired groups. If there are new ones,
-     * add them to the switch. */
-    struct group_info *desired;
-    HMAP_FOR_EACH(desired, hmap_node, &groups->desired_groups) {
-        if (!ovn_group_lookup(&groups->existing_groups, desired)) {
-            /* Create and install new group. */
-            struct ofputil_group_mod gm;
-            enum ofputil_protocol usable_protocols;
-            char *error;
-            struct ds group_string = DS_EMPTY_INITIALIZER;
-            ds_put_format(&group_string, "group_id=%u,%s",
-                          desired->group_id, ds_cstr(&desired->group));
-
-            error = parse_ofp_group_mod_str(&gm, OFPGC11_ADD,
-                                            ds_cstr(&group_string), NULL,
-                                            &usable_protocols);
-            if (!error) {
-                add_group_mod(&gm, &msgs);
-            } else {
-                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_ERR_RL(&rl, "new group %s %s", error,
-                         ds_cstr(&group_string));
-                free(error);
-            }
-            ds_destroy(&group_string);
-            ofputil_uninit_group_mod(&gm);
-        }
-    }
+    ovn_extend_table_install_desired(groups, &msgs);
 
     /* Iterate through all of the installed flows.  If any of them are no
      * longer desired, delete them; if any of them should have different
@@ -962,55 +951,10 @@  ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
         hmap_insert(&installed_flows, &d->hmap_node, d->hmap_node.hash);
     }
 
-    /* Iterate through the installed groups from previous runs. If they
-     * are not needed delete them. */
-    struct group_info *installed, *next_group;
-    HMAP_FOR_EACH_SAFE(installed, next_group, hmap_node,
-                       &groups->existing_groups) {
-        if (!ovn_group_lookup(&groups->desired_groups, installed)) {
-            /* Delete the group. */
-            struct ofputil_group_mod gm;
-            enum ofputil_protocol usable_protocols;
-            char *error;
-            struct ds group_string = DS_EMPTY_INITIALIZER;
-            ds_put_format(&group_string, "group_id=%u", installed->group_id);
-
-            error = parse_ofp_group_mod_str(&gm, OFPGC11_DELETE,
-                                            ds_cstr(&group_string), NULL,
-                                            &usable_protocols);
-            if (!error) {
-                add_group_mod(&gm, &msgs);
-            } else {
-                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_ERR_RL(&rl, "Error deleting group %d: %s",
-                         installed->group_id, error);
-                free(error);
-            }
-            ds_destroy(&group_string);
-            ofputil_uninit_group_mod(&gm);
-
-            /* Remove 'installed' from 'groups->existing_groups' */
-            hmap_remove(&groups->existing_groups, &installed->hmap_node);
-            ds_destroy(&installed->group);
-
-            /* Dealloc group_id. */
-            bitmap_set0(groups->group_ids, installed->group_id);
-            free(installed);
-        }
-    }
+    ovn_extend_table_remove_existing(groups, &msgs);
 
-    /* Move the contents of desired_groups to existing_groups. */
-    HMAP_FOR_EACH_SAFE(desired, next_group, hmap_node,
-                       &groups->desired_groups) {
-        hmap_remove(&groups->desired_groups, &desired->hmap_node);
-        if (!ovn_group_lookup(&groups->existing_groups, desired)) {
-            hmap_insert(&groups->existing_groups, &desired->hmap_node,
-                        desired->hmap_node.hash);
-        } else {
-           ds_destroy(&desired->group);
-           free(desired);
-        }
-    }
+    /* Move the contents of groups->desired to groups->existing. */
+    ovn_extend_table_move(groups);
 
     if (!ovs_list_is_empty(&msgs)) {
         /* Add a barrier to the list of messages. */
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index d83f6aec4..9b5eab1f4 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -23,7 +23,7 @@ 
 #include "ovsdb-idl.h"
 
 struct controller_ctx;
-struct group_table;
+struct ovn_extend_table;
 struct hmap;
 struct match;
 struct ofpbuf;
@@ -31,7 +31,7 @@  struct ovsrec_bridge;
 struct shash;
 
 /* Interface for OVN main loop. */
-void ofctrl_init(struct group_table *group_table);
+void ofctrl_init(struct ovn_extend_table *group_table);
 enum mf_field_id ofctrl_run(const struct ovsrec_bridge *br_int,
                             struct shash *pending_ct_zones);
 bool ofctrl_can_put(void);
@@ -55,7 +55,4 @@  void ofctrl_add_flow(struct hmap *desired_flows, uint8_t table_id,
 
 void ofctrl_flow_table_clear(void);
 
-void ovn_group_table_clear(struct group_table *group_table,
-                           bool existing);
-
 #endif /* ovn/ofctrl.h */
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index c286ccbca..c486887a5 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -42,6 +42,7 @@ 
 #include "openvswitch/vlog.h"
 #include "ovn/actions.h"
 #include "ovn/lib/chassis-index.h"
+#include "ovn/lib/extend-table.h"
 #include "ovn/lib/ovn-sb-idl.h"
 #include "ovn/lib/ovn-util.h"
 #include "patch.h"
@@ -593,11 +594,8 @@  main(int argc, char *argv[])
     unixctl_command_register("exit", "", 0, 0, ovn_controller_exit, &exiting);
 
     /* Initialize group ids for loadbalancing. */
-    struct group_table group_table;
-    group_table.group_ids = bitmap_allocate(MAX_OVN_GROUPS);
-    bitmap_set1(group_table.group_ids, 0); /* Group id 0 is invalid. */
-    hmap_init(&group_table.desired_groups);
-    hmap_init(&group_table.existing_groups);
+    struct ovn_extend_table group_table;
+    ovn_extend_table_init(&group_table);
 
     daemonize_complete();
 
@@ -845,17 +843,7 @@  main(int argc, char *argv[])
     simap_destroy(&ct_zones);
     shash_destroy(&pending_ct_zones);
 
-    bitmap_free(group_table.group_ids);
-    hmap_destroy(&group_table.desired_groups);
-
-    struct group_info *installed, *next_group;
-    HMAP_FOR_EACH_SAFE(installed, next_group, hmap_node,
-                       &group_table.existing_groups) {
-        hmap_remove(&group_table.existing_groups, &installed->hmap_node);
-        ds_destroy(&installed->group);
-        free(installed);
-    }
-    hmap_destroy(&group_table.existing_groups);
+    ovn_extend_table_destroy(&group_table);
 
     ovsdb_idl_loop_destroy(&ovs_idl_loop);
     ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 69a2172a8..8a7fe4f59 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -35,6 +35,7 @@ 
 #include "ovn/expr.h"
 #include "ovn/lex.h"
 #include "ovn/lib/acl-log.h"
+#include "ovn/lib/extend-table.h"
 #include "packets.h"
 #include "openvswitch/shash.h"
 #include "simap.h"
@@ -1026,8 +1027,7 @@  encode_CT_LB(const struct ovnact_ct_lb *cl,
         return;
     }
 
-    uint32_t group_id = 0, hash;
-    struct group_info *group_info;
+    uint32_t table_id = 0;
     struct ofpact_group *og;
     uint32_t zone_reg = ep->is_switch ? MFF_LOG_CT_ZONE - MFF_REG0
                             : MFF_LOG_DNAT_ZONE - MFF_REG0;
@@ -1059,59 +1059,17 @@  encode_CT_LB(const struct ovnact_ct_lb *cl,
                       recirc_table, zone_reg);
     }
 
-    hash = hash_string(ds_cstr(&ds), 0);
-
-    /* Check whether we have non installed but allocated group_id. */
-    HMAP_FOR_EACH_WITH_HASH (group_info, hmap_node, hash,
-                             &ep->group_table->desired_groups) {
-        if (!strcmp(ds_cstr(&group_info->group), ds_cstr(&ds))) {
-            group_id = group_info->group_id;
-            break;
-        }
-    }
-
-    if (!group_id) {
-        /* Check whether we already have an installed entry for this
-         * combination. */
-        HMAP_FOR_EACH_WITH_HASH (group_info, hmap_node, hash,
-                                 &ep->group_table->existing_groups) {
-            if (!strcmp(ds_cstr(&group_info->group), ds_cstr(&ds))) {
-                group_id = group_info->group_id;
-            }
-        }
-
-        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) {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-            VLOG_ERR_RL(&rl, "out of group ids");
-
-            ds_destroy(&ds);
-            return;
-        }
-        bitmap_set1(ep->group_table->group_ids, group_id);
-
-        group_info = xmalloc(sizeof *group_info);
-        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);
-    } else {
+    table_id = ovn_extend_table_assign_id(ep->group_table, &ds);
+    if (table_id == EXT_TABLE_ID_INVALID) {
         ds_destroy(&ds);
+        return;
     }
 
+    ds_destroy(&ds);
+
     /* Create an action to set the group. */
     og = ofpact_put_GROUP(ofpacts);
-    og->group_id = group_id;
+    og->group_id = table_id;
 }
 
 static void
diff --git a/ovn/lib/automake.mk b/ovn/lib/automake.mk
index dae06a5c1..6178fc2d5 100644
--- a/ovn/lib/automake.mk
+++ b/ovn/lib/automake.mk
@@ -10,6 +10,8 @@  ovn_lib_libovn_la_SOURCES = \
 	ovn/lib/chassis-index.c \
 	ovn/lib/chassis-index.h \
 	ovn/lib/expr.c \
+	ovn/lib/extend-table.h \
+	ovn/lib/extend-table.c \
 	ovn/lib/lex.c \
 	ovn/lib/ovn-l7.h \
 	ovn/lib/ovn-util.c \
diff --git a/ovn/lib/extend-table.c b/ovn/lib/extend-table.c
new file mode 100644
index 000000000..dfd1a9413
--- /dev/null
+++ b/ovn/lib/extend-table.c
@@ -0,0 +1,198 @@ 
+/*
+ * Copyright (c) 2017 DtDream Technology Co.,Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <config.h>
+#include "bitmap.h"
+#include "hash.h"
+#include "openvswitch/vlog.h"
+#include "ovn/lib/extend-table.h"
+
+VLOG_DEFINE_THIS_MODULE(extend_table);
+
+void ovn_extend_table_init(struct ovn_extend_table *table)
+{
+    table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
+    bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
+    hmap_init(&table->desired);
+    hmap_init(&table->existing);
+}
+
+void ovn_extend_table_destroy(struct ovn_extend_table *table)
+{
+    bitmap_free(table->table_ids);
+
+    struct ovn_extend_table_info *desired, *desired_next;
+    HMAP_FOR_EACH_SAFE (desired, desired_next, hmap_node, &table->existing) {
+        hmap_remove(&table->existing, &desired->hmap_node);
+        ds_destroy(&desired->info);
+        free(desired);
+    }
+    hmap_destroy(&table->desired);
+
+    struct ovn_extend_table_info *existing, *existing_next;
+    HMAP_FOR_EACH_SAFE (existing, existing_next, hmap_node, &table->existing) {
+        hmap_remove(&table->existing, &existing->hmap_node);
+        ds_destroy(&existing->info);
+        free(existing);
+    }
+    hmap_destroy(&table->existing);
+}
+
+/* Finds and returns a group_info in 'existing' whose key is identical
+ * to 'target''s key, or NULL if there is none. */
+static struct ovn_extend_table_info *
+ovn_extend_table_lookup(struct hmap *exisiting,
+                        const struct ovn_extend_table_info *target)
+{
+    struct ovn_extend_table_info *e;
+
+    HMAP_FOR_EACH_WITH_HASH (e, hmap_node, target->hmap_node.hash,
+                             exisiting) {
+        if (e->table_id == target->table_id) {
+            return e;
+        }
+   }
+    return NULL;
+}
+
+/* Clear either desired or existing in ovn_extend_table. */
+void
+ovn_extend_table_clear(struct ovn_extend_table *table, bool existing)
+{
+    struct ovn_extend_table_info *g, *next;
+    struct hmap *target = existing ? &table->existing : &table->desired;
+
+    HMAP_FOR_EACH_SAFE (g, next, hmap_node, target) {
+        hmap_remove(target, &g->hmap_node);
+        /* Don't unset bitmap for desired group_info if the group_id
+         * was not freshly reserved. */
+        if (existing || g->new_table_id) {
+            bitmap_set0(table->table_ids, g->table_id);
+        }
+        ds_destroy(&g->info);
+        free(g);
+    }
+}
+
+void
+ovn_extend_table_install_desired(struct ovn_extend_table *table,
+                                 struct ovs_list *msgs)
+{
+    struct ovn_extend_table_info *desired;
+
+    /* Iterate through all the desired tables. If there are new ones,
+     * add them to the switch. */
+    HMAP_FOR_EACH (desired, hmap_node, &table->desired) {
+        if (!ovn_extend_table_lookup(&table->existing, desired)) {
+            /* Create and install new group. */
+            table->create(desired->table_id, &desired->info, msgs);
+        }
+    }
+}
+
+void
+ovn_extend_table_remove_existing(struct ovn_extend_table *table,
+                                 struct ovs_list *msgs)
+{
+    struct ovn_extend_table_info *existing, *next;
+
+    /* Iterate through the installed tables from previous runs. If they
+     * are not needed delete them. */
+    HMAP_FOR_EACH_SAFE (existing, next, hmap_node, &table->existing) {
+        if (!ovn_extend_table_lookup(&table->desired, existing)) {
+
+            table->remove(existing->table_id, msgs);
+
+            /* Remove 'installed' from 'groups->existing' */
+            hmap_remove(&table->existing, &existing->hmap_node);
+            ds_destroy(&existing->info);
+
+            /* Dealloc group_id. */
+            bitmap_set0(table->table_ids, existing->table_id);
+            free(existing);
+        }
+    }
+}
+
+void
+ovn_extend_table_move(struct ovn_extend_table *table)
+{
+    struct ovn_extend_table_info *desired, *next;
+
+    /* Move the contents of desired to existing. */
+    HMAP_FOR_EACH_SAFE (desired, next, hmap_node, &table->desired) {
+        hmap_remove(&table->desired, &desired->hmap_node);
+
+        if (!ovn_extend_table_lookup(&table->existing, desired)) {
+            hmap_insert(&table->existing, &desired->hmap_node,
+                        desired->hmap_node.hash);
+        } else {
+           ds_destroy(&desired->info);
+           free(desired);
+        }
+    }
+}
+
+/* Assign a new table ID for the table information from the bitmap.
+ * If it already exists, return the old ID. */
+uint32_t
+ovn_extend_table_assign_id(struct ovn_extend_table *table, struct ds *ds)
+{
+    uint32_t table_id = 0, hash;
+    struct ovn_extend_table_info *table_info;
+
+    hash = hash_string(ds_cstr(ds), 0);
+
+    /* Check whether we have non installed but allocated group_id. */
+    HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash, &table->desired) {
+        if (!strcmp(ds_cstr(&table_info->info), ds_cstr(ds))) {
+            return table_info->table_id;
+        }
+    }
+
+    /* Check whether we already have an installed entry for this
+     * combination. */
+    HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash, &table->existing) {
+        if (!strcmp(ds_cstr(&table_info->info), ds_cstr(ds))) {
+            table_id = table_info->table_id;
+        }
+    }
+
+    bool new_table_id = false;
+    if (!table_id) {
+        /* Reserve a new group_id. */
+        table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID + 1);
+        new_table_id = true;
+    }
+
+    if (table_id == MAX_EXT_TABLE_ID + 1) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+        VLOG_ERR_RL(&rl, "%"PRIu32" out of table ids.", table_id);
+        return EXT_TABLE_ID_INVALID;
+    }
+    bitmap_set1(table->table_ids, table_id);
+
+    table_info = xmalloc(sizeof *table_info);
+    ds_clone(&table_info->info, ds);
+    table_info->table_id = table_id;
+    table_info->hmap_node.hash = hash;
+    table_info->new_table_id = new_table_id;
+
+    hmap_insert(&table->desired,
+                &table_info->hmap_node, table_info->hmap_node.hash);
+
+    return table_id;
+}
diff --git a/ovn/lib/extend-table.h b/ovn/lib/extend-table.h
new file mode 100644
index 000000000..ededba59d
--- /dev/null
+++ b/ovn/lib/extend-table.h
@@ -0,0 +1,69 @@ 
+/*
+ * Copyright (c) 2017 DtDream Technology Co.,Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef EXTEND_TABLE_H
+#define EXTEND_TABLE_H 1
+
+#define MAX_EXT_TABLE_ID 65535
+#define EXT_TABLE_ID_INVALID 0
+
+#include "openvswitch/dynamic-string.h"
+#include "openvswitch/hmap.h"
+#include "openvswitch/list.h"
+
+/* Used to manage expansion tables associated with Flow table,
+ * such as the Group Table or Meter Table. */
+struct ovn_extend_table {
+    unsigned long *table_ids;  /* Used as a bitmap with value set
+                                * for allocated group ids in either
+                                * desired or existing. */
+    struct hmap desired;
+    struct hmap existing;
+
+    /* Used to create a form entity through the openflow channel. */
+    void (*create)(uint32_t, struct ds *, struct ovs_list *);
+
+    /* Used to remove a form entity through the openflow channel. */
+    void (*remove)(uint32_t, struct ovs_list *);
+};
+
+struct ovn_extend_table_info {
+    struct hmap_node hmap_node;
+    struct ds info;     /* Details string for the table entity. */
+    uint32_t table_id;
+    bool new_table_id;  /* 'True' if 'table_id' was reserved from
+                         * ovn_extend_table's 'table_ids' bitmap. */
+};
+
+void ovn_extend_table_init(struct ovn_extend_table *table);
+
+void ovn_extend_table_destroy(struct ovn_extend_table *table);
+
+void ovn_extend_table_install_desired(struct ovn_extend_table *table,
+                                      struct ovs_list *msgs);
+
+void ovn_extend_table_remove_existing(struct ovn_extend_table *table,
+                                      struct ovs_list *msgs);
+
+/* Move the contents of desired to existing. */
+void ovn_extend_table_move(struct ovn_extend_table *table);
+
+void ovn_extend_table_clear(struct ovn_extend_table *table, bool existing);
+
+uint32_t ovn_extend_table_assign_id(struct ovn_extend_table *table,
+                                    struct ds *ds);
+
+#endif /* ovn/lib/extend-table.h */
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index f9a5085f7..4f65ee9d1 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -33,6 +33,7 @@ 
 #include "ovn/lex.h"
 #include "ovn/lib/logical-fields.h"
 #include "ovn/lib/ovn-l7.h"
+#include "ovn/lib/extend-table.h"
 #include "ovs-thread.h"
 #include "ovstest.h"
 #include "openvswitch/shash.h"
@@ -1207,11 +1208,8 @@  test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
     create_gen_opts(&dhcp_opts, &dhcpv6_opts, &nd_ra_opts);
 
     /* Initialize group ids. */
-    struct group_table group_table;
-    group_table.group_ids = bitmap_allocate(MAX_OVN_GROUPS);
-    bitmap_set1(group_table.group_ids, 0); /* Group id 0 is invalid. */
-    hmap_init(&group_table.desired_groups);
-    hmap_init(&group_table.existing_groups);
+    struct ovn_extend_table group_table;
+    ovn_extend_table_init(&group_table);
 
     simap_init(&ports);
     simap_put(&ports, "eth0", 5);