diff mbox

[ovs-dev,RFC] ovn: Extend Address_Set to Macros_Set to support define port sets

Message ID 1470222115-24449-1-git-send-email-zealokii@gmail.com
State Superseded
Headers show

Commit Message

Zong Kai LI Aug. 3, 2016, 11:01 a.m. UTC
This patch aims to extend Address_Set to Macros_Set, make it more common to
accept variable set, not only address. And by that, we can skinny down ACLs,
if we use Macros_Set to defines port sets for ACL to use, since lots of ACL
entries are similar but only "inport" and "outport" fields are different.

Address_Set improves the performance of OVN control plane. But what more
important is, its implementation introduced a "$macros -- *_Set" mechanism,
it's good way to do bunch jobs for ACLs and lflows.

How does Address_Set improve OVN performance?
 - it reduces the match part length for lflows, who has addresses to match.
 - indeed, it means less tokens in expression to parse, less constants, less
   combination symbols.
 - and for CMS integration component, for its DB sync job, a ACL entry has
   less match column is easier to compare.

What changes:
  Adderss_Set           -> Macros_Set
  Adderss_Set.addresses -> Macros_Set.elements
                           (addresses, or ports names)
                        -> Macros_Set.type
                           (only "address", "port" are valid now)

Will port set defined under Macros_Set benefit in the same way as Address_Set
did? -- NO, it may be better.
 - it will not decrease lflow's length, but the number. ACL entries who have
   similar match expressions, but with particular "inport" or "outport"
   can use port set defined in Macros_Set to combine into one ACL.
 - less ACL entries, will help CMS integration component has less workload
   in DB sync job, comparing port name should be quicker then comparing whole
   ACL entry.
 - less ACL entries, mess less lflows, means less match part in lflows to
   parse, less tokens to parse, make parsing work quicker.

Port set defined under Macros_Set will improve ovn-controller performance on
installation plenty of similar ACLs/lflows, but not for port comes and leaves
scenario.

Potential drawback: not sure yet.

TODO: go check how will ovn-northd use port sets defined under Macros_Set

Signed-off-by: Zong Kai LI <zealokii@gmail.com>
---
 include/ovn/expr.h        |   2 +-
 ovn/controller/lflow.c    | 176 ++++++++++++++++++++++++----------------------
 ovn/lib/expr.c            |  68 +++++++++++-------
 ovn/northd/ovn-northd.c   |  54 +++++++-------
 ovn/ovn-nb.ovsschema      |  11 +--
 ovn/ovn-nb.xml            |  20 ++++--
 ovn/ovn-sb.ovsschema      |  11 +--
 ovn/ovn-sb.xml            |   7 +-
 ovn/utilities/ovn-nbctl.c |   4 +-
 ovn/utilities/ovn-sbctl.c |   4 +-
 tests/ovn.at              |   8 +--
 11 files changed, 201 insertions(+), 164 deletions(-)

Comments

Russell Bryant Aug. 3, 2016, 11:56 a.m. UTC | #1
On Wed, Aug 3, 2016 at 7:01 AM, Zong Kai LI <zealokii@gmail.com> wrote:

> This patch aims to extend Address_Set to Macros_Set, make it more common to
> accept variable set, not only address. And by that, we can skinny down
> ACLs,
> if we use Macros_Set to defines port sets for ACL to use, since lots of ACL
> entries are similar but only "inport" and "outport" fields are different.
>

I think the idea makes sense.

I wonder if just "Set" would be an appropriate name.  One catch is that it
may be too late to get into OVS 2.6.  If so, then we have to take into
account backwards compatibility.  We won't be able to just rename the
table.  One option would be to at least rename the table now (before 2.6)
to reflect a more broad purpose, and leave the support for other types of
entries to later patches.

Are you looking for any other specific feedback?
Zong Kai LI Aug. 3, 2016, 12:23 p.m. UTC | #2
On Wed, Aug 3, 2016 at 7:56 PM, Russell Bryant <russell@ovn.org> wrote:
>
>
> On Wed, Aug 3, 2016 at 7:01 AM, Zong Kai LI <zealokii@gmail.com> wrote:
>>
>> This patch aims to extend Address_Set to Macros_Set, make it more common
>> to
>> accept variable set, not only address. And by that, we can skinny down
>> ACLs,
>> if we use Macros_Set to defines port sets for ACL to use, since lots of
>> ACL
>> entries are similar but only "inport" and "outport" fields are different.
>
>
> I think the idea makes sense.
>
> I wonder if just "Set" would be an appropriate name.  One catch is that it
> may be too late to get into OVS 2.6.  If so, then we have to take into
> account backwards compatibility.  We won't be able to just rename the table.
> One option would be to at least rename the table now (before 2.6) to reflect
> a more broad purpose, and leave the support for other types of entries to
> later patches.
>
> Are you looking for any other specific feedback?
>
> --
> Russell Bryant

Hi, Russell.
I noticed "Macros_Set" is redundant to "Macros", but I'm not so sure,
so I left it in this version.
Most code in this patch is "rename", fewer is to extend to support
string type members, like port name. So I think it's ok to do both
rename and support string members. And left any other support and
enhance in follow patches.
Any other special feedback, yes, I hope that. To make a simple
beginning, I didn't do any changing in ovn-northd, but I think it's
possible to make ovn-northd to use it on some lflows. Not sure whether
it's worth to do that, or will it make ovn-northd code complex. So I
list this in TODO.

Thanks,
Zongkai, LI
diff mbox

Patch

diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index d790c49..5020ff4 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -466,7 +466,7 @@  void expr_constant_set_destroy(struct expr_constant_set *cs);
  * The values that don't qualify are ignored.
  */
 
-void expr_macros_add(struct shash *macros, const char *name,
+void expr_macros_add(struct shash *macros, const char *name, bool parse_value,
                      const char * const *values, size_t n_values);
 void expr_macros_remove(struct shash *macros, const char *name);
 void expr_macros_destroy(struct shash *macros);
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index fda10eb..5eae183 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -38,8 +38,8 @@  VLOG_DEFINE_THIS_MODULE(lflow);
 /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */
 static struct shash symtab;
 
-/* Contains an internal expr datastructure that represents an address set. */
-static struct shash expr_address_sets;
+/* Contains an internal expr datastructure that represents an macro set. */
+static struct shash expr_macros_sets;
 
 static bool full_flow_processing = false;
 static bool full_logical_flow_processing = false;
@@ -65,7 +65,7 @@  void
 lflow_init(void)
 {
     shash_init(&symtab);
-    shash_init(&expr_address_sets);
+    shash_init(&expr_macros_sets);
 
     /* Reserve a pair of registers for the logical inport and outport.  A full
      * 32-bit register each is bigger than we need, but the expression code
@@ -187,149 +187,159 @@  lflow_init(void)
     expr_symtab_add_field(&symtab, "sctp.dst", MFF_SCTP_DST, "sctp", false);
 }
 
-/* Details of an address set currently in address_sets. We keep a cached
+/* Details of a macro set currently in macros_sets. We keep a cached
  * copy of sets still in their string form here to make it easier to compare
  * with the current values in the OVN_Southbound database. */
-struct address_set {
-    char **addresses;
-    size_t n_addresses;
+struct macro_set {
+    char **elements;
+    size_t n_elements;
 };
 
-/* struct address_set instances for address sets currently in the symtab,
- * hashed on the address set name. */
-static struct shash local_address_sets = SHASH_INITIALIZER(&local_address_sets);
+/* struct macro_set instances for address sets and port set currently in the
+ * symtab, hashed on the macro set name. */
+static struct shash local_macros_sets = SHASH_INITIALIZER(&local_macros_sets);
 
 static int
-addr_cmp(const void *p1, const void *p2)
+macro_element_cmp(const void *p1, const void *p2)
 {
     const char *s1 = p1;
     const char *s2 = p2;
     return strcmp(s1, s2);
 }
 
-/* Return true if the address sets match, false otherwise. */
+/* Return true if the macro sets match, false otherwise. */
 static bool
-address_sets_match(const struct address_set *addr_set,
-                   const struct sbrec_address_set *addr_set_rec)
+macro_sets_match(const struct macro_set *m_set,
+                 const struct sbrec_macros_set *macros_set_rec)
 {
-    char **addrs1;
-    char **addrs2;
-
-    if (addr_set->n_addresses != addr_set_rec->n_addresses) {
+    if (m_set->n_elements != macros_set_rec->n_elements) {
         return false;
     }
-    size_t n_addresses = addr_set->n_addresses;
 
-    addrs1 = xmemdup(addr_set->addresses,
-                     n_addresses * sizeof addr_set->addresses[0]);
-    addrs2 = xmemdup(addr_set_rec->addresses,
-                     n_addresses * sizeof addr_set_rec->addresses[0]);
+    char **elements1;
+    char **elements2;
+    size_t n_elements = m_set->n_elements;
+
+    elements1 = xmemdup(m_set->elements,
+                        n_elements * sizeof m_set->elements[0]);
+    elements2 = xmemdup(macros_set_rec->elements,
+                        n_elements * sizeof macros_set_rec->elements[0]);
 
-    qsort(addrs1, n_addresses, sizeof *addrs1, addr_cmp);
-    qsort(addrs2, n_addresses, sizeof *addrs2, addr_cmp);
+    qsort(elements1, n_elements, sizeof *elements1, macro_element_cmp);
+    qsort(elements2, n_elements, sizeof *elements2, macro_element_cmp);
 
     bool res = true;
     size_t i;
-    for (i = 0; i <  n_addresses; i++) {
-        if (strcmp(addrs1[i], addrs2[i])) {
+    for (i = 0; i <  n_elements; i++) {
+        if (strcmp(elements1[i], elements2[i])) {
             res = false;
             break;
         }
     }
 
-    free(addrs1);
-    free(addrs2);
+    free(elements1);
+    free(elements2);
 
     return res;
 }
 
 static void
-address_set_destroy(struct address_set *addr_set)
+macro_set_destroy(struct macro_set *m_set)
 {
     size_t i;
-    for (i = 0; i < addr_set->n_addresses; i++) {
-        free(addr_set->addresses[i]);
+    for (i = 0; i < m_set->n_elements; i++) {
+        free(m_set->elements[i]);
     }
-    if (addr_set->n_addresses) {
-        free(addr_set->addresses);
+    if (m_set->n_elements) {
+        free(m_set->elements);
     }
-    free(addr_set);
+    free(m_set);
 }
 
 static void
-update_address_sets(struct controller_ctx *ctx)
+update_macros_sets(struct controller_ctx *ctx)
 {
-    /* Remember the names of all address sets currently in expr_address_sets
-     * so we can detect address sets that have been deleted. */
-    struct sset cur_addr_set_names = SSET_INITIALIZER(&cur_addr_set_names);
+    /* Remember the names of all macros sets currently in expr_macors_sets
+     * so we can detect macros sets that have been deleted. */
+    struct sset cur_macros_set_names = SSET_INITIALIZER(&cur_macros_set_names);
 
     struct shash_node *node;
-    SHASH_FOR_EACH (node, &local_address_sets) {
-        sset_add(&cur_addr_set_names, node->name);
+    SHASH_FOR_EACH (node, &local_macros_sets) {
+        sset_add(&cur_macros_set_names, node->name);
     }
 
-    /* Iterate address sets in the southbound database.  Create and update the
-     * corresponding symtab entries as necessary. */
-    const struct sbrec_address_set *addr_set_rec;
-    SBREC_ADDRESS_SET_FOR_EACH (addr_set_rec, ctx->ovnsb_idl) {
-        struct address_set *addr_set =
-            shash_find_data(&local_address_sets, addr_set_rec->name);
+    /* Iterate address sets and port sets in the southbound database.
+     * Create and update the corresponding symtab entries as necessary. */
+    const struct sbrec_macros_set *macros_set_rec;
+    SBREC_MACROS_SET_FOR_EACH (macros_set_rec, ctx->ovnsb_idl) {
+        if (!macros_set_rec->type) {
+            continue;
+        }
+        bool is_addr_set = !strcmp(macros_set_rec->type, "address");
+        bool is_port_set = !strcmp(macros_set_rec->type, "port");
+        if (!is_addr_set && !is_port_set) {
+            continue;
+        }
+        struct macro_set *m_set =
+            shash_find_data(&local_macros_sets, macros_set_rec->name);
 
         bool create_set = false;
-        if (addr_set) {
-            /* This address set has already been added.  We must determine
+        if (m_set) {
+            /* This macro set has already been added.  We must determine
              * if the symtab entry needs to be updated due to a change. */
-            sset_find_and_delete(&cur_addr_set_names, addr_set_rec->name);
-            if (!address_sets_match(addr_set, addr_set_rec)) {
-                shash_find_and_delete(&local_address_sets, addr_set_rec->name);
-                expr_macros_remove(&expr_address_sets, addr_set_rec->name);
-                address_set_destroy(addr_set);
-                addr_set = NULL;
+            sset_find_and_delete(&cur_macros_set_names, macros_set_rec->name);
+            if (!macro_sets_match(m_set, macros_set_rec)) {
+                shash_find_and_delete(&local_macros_sets,
+                                      macros_set_rec->name);
+                expr_macros_remove(&expr_macros_sets,
+                                   macros_set_rec->name);
+                macro_set_destroy(m_set);
+                m_set = NULL;
                 create_set = true;
             }
         } else {
-            /* This address set is not yet in the symtab, so add it. */
+            /* This macro set is not yet in the symtab, so add it. */
             create_set = true;
         }
 
         if (create_set) {
-            /* The address set is either new or has changed.  Create a symbol
-             * that resolves to the full set of addresses.  Store it in
-             * address_sets to remember that we created this symbol. */
-            addr_set = xzalloc(sizeof *addr_set);
-            addr_set->n_addresses = addr_set_rec->n_addresses;
-            if (addr_set_rec->n_addresses) {
-                addr_set->addresses = xmalloc(addr_set_rec->n_addresses
-                                              * sizeof addr_set->addresses[0]);
+            /* The macro set is either new or has changed.  Create a symbol
+             * that resolves to the full set of addresses or ports.  Store it
+             * in macro_set to remember that we created this symbol. */
+            m_set = xzalloc(sizeof *m_set);
+            m_set->n_elements = macros_set_rec->n_elements;
+            if (macros_set_rec->n_elements) {
+                m_set->elements = xmalloc(macros_set_rec->n_elements
+                                          * sizeof m_set->elements[0]);
                 size_t i;
-                for (i = 0; i < addr_set_rec->n_addresses; i++) {
-                    addr_set->addresses[i] = xstrdup(addr_set_rec->addresses[i]);
+                for (i = 0; i < macros_set_rec->n_elements; i++) {
+                    m_set->elements[i] = xstrdup(macros_set_rec->elements[i]);
                 }
             }
-            shash_add(&local_address_sets, addr_set_rec->name, addr_set);
-
-            expr_macros_add(&expr_address_sets, addr_set_rec->name,
-                            (const char * const *) addr_set->addresses,
-                            addr_set->n_addresses);
+            shash_add(&local_macros_sets, macros_set_rec->name, m_set);
+            expr_macros_add(&expr_macros_sets, macros_set_rec->name,
+                            is_addr_set,
+                            (const char * const *) m_set->elements,
+                            m_set->n_elements);
         }
     }
 
-    /* Anything remaining in cur_addr_set_names refers to an address set that
+    /* Anything remaining in cur_macros_set_names refers to an address set that
      * has been deleted from the southbound database.  We should delete
      * the corresponding symtab entry. */
     const char *cur_node, *next_node;
-    SSET_FOR_EACH_SAFE (cur_node, next_node, &cur_addr_set_names) {
-        expr_macros_remove(&expr_address_sets, cur_node);
+    SSET_FOR_EACH_SAFE (cur_node, next_node, &cur_macros_set_names) {
+        expr_macros_remove(&expr_macros_sets, cur_node);
 
-        struct address_set *addr_set
-            = shash_find_and_delete(&local_address_sets, cur_node);
-        address_set_destroy(addr_set);
+        struct macro_set *m_set
+            = shash_find_and_delete(&local_macros_sets, cur_node);
+        macro_set_destroy(m_set);
 
         struct sset_node *sset_node = SSET_NODE_FROM_NAME(cur_node);
-        sset_delete(&cur_addr_set_names, sset_node);
+        sset_delete(&cur_macros_set_names, sset_node);
     }
 
-    sset_destroy(&cur_addr_set_names);
+    sset_destroy(&cur_macros_set_names);
 }
 
 struct lookup_port_aux {
@@ -544,7 +554,7 @@  consider_logical_flow(const struct lport_index *lports,
     struct expr *expr;
 
     expr = expr_parse_string(lflow->match, &symtab,
-                             &expr_address_sets, &error);
+                             &expr_macros_sets, &error);
     if (!error) {
         if (prereqs) {
             expr = expr_combine(EXPR_T_AND, expr, prereqs);
@@ -713,7 +723,7 @@  lflow_run(struct controller_ctx *ctx, const struct lport_index *lports,
           struct group_table *group_table,
           const struct simap *ct_zones)
 {
-    update_address_sets(ctx);
+    update_macros_sets(ctx);
     add_logical_flows(ctx, lports, mcgroups, local_datapaths,
                       patched_datapaths, group_table, ct_zones);
     add_neighbor_flows(ctx, lports);
@@ -724,6 +734,6 @@  lflow_destroy(void)
 {
     expr_symtab_destroy(&symtab);
     shash_destroy(&symtab);
-    expr_macros_destroy(&expr_address_sets);
-    shash_destroy(&expr_address_sets);
+    expr_macros_destroy(&expr_macros_sets);
+    shash_destroy(&expr_macros_sets);
 }
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 1649c05..6d55949 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -734,26 +734,32 @@  static bool
 parse_macros(struct expr_context *ctx, struct expr_constant_set *cs,
              size_t *allocated_values)
 {
-    struct expr_constant_set *addr_set
+    struct expr_constant_set *macros_set
         = shash_find_data(ctx->macros, ctx->lexer->token.s);
-    if (!addr_set) {
+    if (!macros_set) {
         expr_syntax_error(ctx, "expecting address set name.");
         return false;
     }
 
-    if (!assign_constant_set_type(ctx, cs, EXPR_C_INTEGER)) {
+    if (!assign_constant_set_type(ctx, cs, macros_set->type)) {
         return false;
     }
 
-    size_t n_values = cs->n_values + addr_set->n_values;
+    size_t n_values = cs->n_values + macros_set->n_values;
     if (n_values >= *allocated_values) {
         cs->values = xrealloc(cs->values, n_values * sizeof *cs->values);
         *allocated_values = n_values;
     }
-    for (size_t i = 0; i < addr_set->n_values; i++) {
-        cs->values[cs->n_values++] = addr_set->values[i];
+    if (macros_set->type == EXPR_C_INTEGER) {
+        for (size_t i = 0; i < macros_set->n_values; i++) {
+            cs->values[cs->n_values++] = macros_set->values[i];
+        }
+    } else {
+        for (size_t i = 0; i < macros_set->n_values; i++) {
+            cs->values[cs->n_values++].string = xstrdup(
+                macros_set->values[i].string);
+        }
     }
-
     return true;
 }
 
@@ -846,37 +852,45 @@  expr_constant_set_destroy(struct expr_constant_set *cs)
 /* Adds a macro named 'name' to 'macros', replacing any existing macro with the
  * given name. */
 void
-expr_macros_add(struct shash *macros, const char *name,
+expr_macros_add(struct shash *macros, const char *name, bool parse_value,
                 const char *const *values, size_t n_values)
 {
     /* Replace any existing entry for this name. */
     expr_macros_remove(macros, name);
 
     struct expr_constant_set *cs = xzalloc(sizeof *cs);
-    cs->type = EXPR_C_INTEGER;
     cs->in_curlies = true;
     cs->n_values = 0;
     cs->values = xmalloc(n_values * sizeof *cs->values);
-    for (size_t i = 0; i < n_values; i++) {
-        /* Use the lexer to convert each macro into the proper
-         * integer format. */
-        struct lexer lex;
-        lexer_init(&lex, values[i]);
-        lexer_get(&lex);
-        if (lex.token.type != LEX_T_INTEGER
-            && lex.token.type != LEX_T_MASKED_INTEGER) {
-            VLOG_WARN("Invalid address set entry: '%s', token type: %d",
-                      values[i], lex.token.type);
-        } else {
-            union expr_constant *c = &cs->values[cs->n_values++];
-            c->value = lex.token.value;
-            c->format = lex.token.format;
-            c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
-            if (c->masked) {
-                c->mask = lex.token.mask;
+    if (parse_value) {
+        cs->type = EXPR_C_INTEGER;
+        for (size_t i = 0; i < n_values; i++) {
+            /* Use the lexer to convert each macro into the proper
+             * integer format. */
+            struct lexer lex;
+            lexer_init(&lex, values[i]);
+            lexer_get(&lex);
+            if (lex.token.type == LEX_T_INTEGER
+                || lex.token.type == LEX_T_MASKED_INTEGER) {
+                union expr_constant *c = &cs->values[cs->n_values++];
+                c->value = lex.token.value;
+                c->format = lex.token.format;
+                c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
+                if (c->masked) {
+                    c->mask = lex.token.mask;
+                }
+            } else {
+                VLOG_WARN("Invalid macro set entry: '%s', token type: %d",
+                          values[i], lex.token.type);
             }
+            lexer_destroy(&lex);
+        }
+    } else {
+        cs->type = EXPR_C_STRING;
+        for (size_t i = 0; i < n_values; i++) {
+            union expr_constant *c = &cs->values[cs->n_values++];
+            c->string = xstrdup(values[i]);
         }
-        lexer_destroy(&lex);
     }
 
     shash_add(macros, name, cs);
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index d6c14cf..12db9f3 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -3781,41 +3781,42 @@  build_lflows(struct northd_context *ctx, struct hmap *datapaths,
     hmap_destroy(&mcgroups);
 }
 
-/* OVN_Northbound and OVN_Southbound have an identical Address_Set table.
+/* OVN_Northbound and OVN_Southbound have an identical Macros_Set table.
  * We always update OVN_Southbound to match the current data in
- * OVN_Northbound, so that the address sets used in Logical_Flows in
- * OVN_Southbound is checked against the proper set.*/
+ * OVN_Northbound, so that the address sets and ports set used in
+ * Logical_Flows in OVN_Southbound is checked against the proper set.*/
 static void
-sync_address_sets(struct northd_context *ctx)
+sync_macros_sets(struct northd_context *ctx)
 {
-    struct shash sb_address_sets = SHASH_INITIALIZER(&sb_address_sets);
+    struct shash sb_macros_sets = SHASH_INITIALIZER(&sb_macros_sets);
 
-    const struct sbrec_address_set *sb_address_set;
-    SBREC_ADDRESS_SET_FOR_EACH (sb_address_set, ctx->ovnsb_idl) {
-        shash_add(&sb_address_sets, sb_address_set->name, sb_address_set);
+    const struct sbrec_macros_set *sb_macros_set;
+    SBREC_MACROS_SET_FOR_EACH (sb_macros_set, ctx->ovnsb_idl) {
+        shash_add(&sb_macros_sets, sb_macros_set->name, sb_macros_set);
     }
 
-    const struct nbrec_address_set *nb_address_set;
-    NBREC_ADDRESS_SET_FOR_EACH (nb_address_set, ctx->ovnnb_idl) {
-        sb_address_set = shash_find_and_delete(&sb_address_sets,
-                                               nb_address_set->name);
-        if (!sb_address_set) {
-            sb_address_set = sbrec_address_set_insert(ctx->ovnsb_txn);
-            sbrec_address_set_set_name(sb_address_set, nb_address_set->name);
+    const struct nbrec_macros_set *nb_macros_set;
+    NBREC_MACROS_SET_FOR_EACH (nb_macros_set, ctx->ovnnb_idl) {
+        sb_macros_set = shash_find_and_delete(&sb_macros_sets,
+                                               nb_macros_set->name);
+        if (!sb_macros_set) {
+            sb_macros_set = sbrec_macros_set_insert(ctx->ovnsb_txn);
+            sbrec_macros_set_set_name(sb_macros_set, nb_macros_set->name);
         }
 
-        sbrec_address_set_set_addresses(sb_address_set,
+        sbrec_macros_set_set_type(sb_macros_set, nb_macros_set->type);
+        sbrec_macros_set_set_elements(sb_macros_set,
                 /* "char **" is not compatible with "const char **" */
-                (const char **) nb_address_set->addresses,
-                nb_address_set->n_addresses);
+                (const char **) nb_macros_set->elements,
+                nb_macros_set->n_elements);
     }
 
     struct shash_node *node, *next;
-    SHASH_FOR_EACH_SAFE (node, next, &sb_address_sets) {
-        sbrec_address_set_delete(node->data);
-        shash_delete(&sb_address_sets, node);
+    SHASH_FOR_EACH_SAFE (node, next, &sb_macros_sets) {
+        sbrec_macros_set_delete(node->data);
+        shash_delete(&sb_macros_sets, node);
     }
-    shash_destroy(&sb_address_sets);
+    shash_destroy(&sb_macros_sets);
 }
 
 static void
@@ -3830,7 +3831,7 @@  ovnnb_db_run(struct northd_context *ctx, struct ovsdb_idl_loop *sb_loop)
     build_ipam(ctx, &datapaths, &ports);
     build_lflows(ctx, &datapaths, &ports);
 
-    sync_address_sets(ctx);
+    sync_macros_sets(ctx);
 
     struct ovn_datapath *dp, *next_dp;
     HMAP_FOR_EACH_SAFE (dp, next_dp, key_node, &datapaths) {
@@ -4194,9 +4195,10 @@  main(int argc, char *argv[])
     add_column_noalert(ovnsb_idl_loop.idl, &sbrec_dhcp_options_col_type);
     add_column_noalert(ovnsb_idl_loop.idl, &sbrec_dhcp_options_col_name);
 
-    ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_address_set);
-    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_address_set_col_name);
-    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_address_set_col_addresses);
+    ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_macros_set);
+    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_macros_set_col_name);
+    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_macros_set_col_type);
+    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_macros_set_col_elements);
 
     ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_chassis);
     ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_nb_cfg);
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 660db76..9aae036 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Northbound",
     "version": "5.3.0",
-    "cksum": "1305504870 9051",
+    "cksum": "2107372947 9091",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -74,12 +74,13 @@ 
                              "min": 0, "max": "unlimited"}}},
             "indexes": [["name"]],
             "isRoot": false},
-        "Address_Set": {
+        "Macros_Set": {
             "columns": {
                 "name": {"type": "string"},
-                "addresses": {"type": {"key": "string",
-                                       "min": 0,
-                                       "max": "unlimited"}},
+                "type": {"type": "string"},
+                "elements": {"type": {"key": "string",
+                                      "min": 0,
+                                      "max": "unlimited"}},
                 "external_ids": {
                     "type": {"key": "string", "value": "string",
                              "min": 0, "max": "unlimited"}}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 4ce295a..c01759e 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -599,15 +599,19 @@ 
     </group>
   </table>
 
-  <table name="Address_Set" title="Address Sets">
+  <table name="Macros_Set" title="Macros Sets">
     <p>
-      Each row in this table represents a named set of addresses.
+      Each row in this table represents a named set of addresses, or ports.
       An address set may contain Ethernet, IPv4, or IPv6 addresses
       with optional bitwise or CIDR masks.
+      An port set may contain name string of Logical_Switch_Port.
       Address set may ultimately be used in ACLs to compare against
       fields such as <code>ip4.src</code> or <code>ip6.src</code>.
+      Port sets may ultimately be used in ACLs to compare against
+      fileds such as <code>inport</code> or <code>outport</code>.
       A single address set must contain addresses of the
-      same type. As an example, the following would create an address set
+      same type.
+      As an example, the following would create an address set
       with three IP addresses:
     </p>
 
@@ -616,7 +620,7 @@ 
     </pre>
 
     <p>
-      Address sets may be used in the <ref column="match" table="ACL"/> column
+      Macros sets may be used in the <ref column="match" table="ACL"/> column
       of the <ref table="ACL"/> table.  For syntax information, see the details
       of the expression language used for the <ref column="match"
       table="Logical_Flow" db="OVN_Southbound"/> column in the <ref
@@ -628,8 +632,12 @@ 
       A name for the address set.  This must be unique among all address sets.
     </column>
 
-    <column name="addresses">
-      The set of addresses in string form.
+    <column name="type">
+      Type of address set. "address" and "port" are valid for now.
+    </column>
+
+    <column name="elements">
+      The set of addresses or ports in string form.
     </column>
 
     <group title="Common Columns">
diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
index b1737f5..2dc4c11 100644
--- a/ovn/ovn-sb.ovsschema
+++ b/ovn/ovn-sb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Southbound",
     "version": "1.7.0",
-    "cksum": "3677179333 6917",
+    "cksum": "2431376416 6939",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -37,12 +37,13 @@ 
                                      "min": 0,
                                      "max": "unlimited"}},
                 "ip": {"type": "string"}}},
-        "Address_Set": {
+        "Macros_Set": {
             "columns": {
                 "name": {"type": "string"},
-                "addresses": {"type": {"key": "string",
-                                       "min": 0,
-                                       "max": "unlimited"}}},
+                "type": {"type": "string"},
+                "elements": {"type": {"key": "string",
+                             "min": 0,
+                             "max": "unlimited"}}},
             "indexes": [["name"]],
             "isRoot": true},
         "Logical_Flow": {
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 13c9526..b3fca9e 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -283,15 +283,16 @@ 
     </column>
   </table>
 
-  <table name="Address_Set" title="Address Sets">
+  <table name="Macros_Set" title="Macros Sets">
     <p>
-      See the documentation for the <ref table="Address_Set"
+      See the documentation for the <ref table="Macros_Set"
       db="OVN_Northbound"/> table in the <ref db="OVN_Northbound"/> database
       for details.
     </p>
 
     <column name="name"/>
-    <column name="addresses"/>
+    <column name="type"/>
+    <column name="elements"/>
   </table>
 
   <table name="Logical_Flow" title="Logical Network Flows">
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 03b6c2b..448e450 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -2142,8 +2142,8 @@  static const struct ctl_table_class tables[] = {
        NULL},
       {NULL, NULL, NULL}}},
 
-    {&nbrec_table_address_set,
-     {{&nbrec_table_address_set, &nbrec_address_set_col_name, NULL},
+    {&nbrec_table_macros_set,
+     {{&nbrec_table_macros_set, &nbrec_macros_set_col_name, NULL},
       {NULL, NULL, NULL}}},
 
     {&nbrec_table_dhcp_options,
diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
index 936915b..dac70cf 100644
--- a/ovn/utilities/ovn-sbctl.c
+++ b/ovn/utilities/ovn-sbctl.c
@@ -782,8 +782,8 @@  static const struct ctl_table_class tables[] = {
      {{&sbrec_table_mac_binding, &sbrec_mac_binding_col_logical_port, NULL},
       {NULL, NULL, NULL}}},
 
-    {&sbrec_table_address_set,
-     {{&sbrec_table_address_set, &sbrec_address_set_col_name, NULL},
+    {&sbrec_table_macros_set,
+     {{&sbrec_table_macros_set, &sbrec_macros_set_col_name, NULL},
       {NULL, NULL, NULL}}},
 
     {NULL, {{NULL, NULL, NULL}, {NULL, NULL, NULL}}}
diff --git a/tests/ovn.at b/tests/ovn.at
index 54fa8c5..b0ad136 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -760,7 +760,7 @@  done
 ovn-nbctl acl-add lsw0 from-lport 1000 'eth.type == 0x1234' drop
 ovn-nbctl acl-add lsw0 from-lport 1000 'eth.type == 0x1235 && inport == "lp11"' drop
 ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1236 && outport == "lp33"' drop
-ovn-nbctl create Address_Set name=set1 addresses=\"f0:00:00:00:00:11\",\"f0:00:00:00:00:21\",\"f0:00:00:00:00:31\"
+ovn-nbctl create Macros_Set name=set1 type="address" elements=\"f0:00:00:00:00:11\",\"f0:00:00:00:00:21\",\"f0:00:00:00:00:31\"
 ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1237 && eth.src == $set1 && outport == "lp33"' drop
 
 # Pre-populate the hypervisors' ARP tables so that we don't lose any
@@ -3718,9 +3718,9 @@  as hv1
 ovs-vsctl add-br br-phys
 ovn_attach n1 br-phys 192.168.0.1
 
-row=`ovn-nbctl create Address_Set name=set1 addresses=\"1.1.1.1\"`
-ovn-nbctl set Address_Set $row name=set1 addresses=\"1.1.1.1,1.1.1.2\"
-ovn-nbctl destroy Address_Set $row
+row=`ovn-nbctl create Macros_Set name=set1 type="address" elements=\"1.1.1.1\"`
+ovn-nbctl set Macros_Set $row name=set1 type="address" elements=\"1.1.1.1,1.1.1.2\"
+ovn-nbctl destroy Macros_Set $row
 
 sleep 1