acl-plugin: change the src/dst L3 info in 5tuple struct to be always contiguous with... 08/13008/2
authorAndrew Yourtchenko <ayourtch@gmail.com>
Tue, 12 Jun 2018 13:15:49 +0000 (15:15 +0200)
committerDamjan Marion <dmarion@me.com>
Wed, 13 Jun 2018 12:13:11 +0000 (12:13 +0000)
Using ip46_address_t was convenient from operational point of view but created
some difficulties dealing with IPv4 addresses - the extra 3x of u32 padding
are costly, and the "holes" mean we can not use the smaller key-value
data structures for the lookup.

This commit changes the 5tuple layout for the IPv4 case, such that
the src/dst addresses directly precede the L4 information.
That will allow to treat the same data within 40x8 key-value
structure as a 16x8 key-value structure starting with 24 byte offset.

Change-Id: Ifea8d266ca0b9c931d44440bf6dc62446c1a83ec
Signed-off-by: Andrew Yourtchenko <ayourtch@gmail.com>
src/plugins/acl/dataplane_node.c
src/plugins/acl/fa_node.h
src/plugins/acl/hash_lookup.c
src/plugins/acl/public_inlines.h
src/plugins/acl/sess_mgmt_node.c
src/plugins/acl/session_inlines.h

index f1ed4c2..dead2ec 100644 (file)
@@ -414,19 +414,30 @@ format_fa_5tuple (u8 * s, va_list * args)
 {
   fa_5tuple_t *p5t = va_arg (*args, fa_5tuple_t *);
 
-  return format (s, "lc_index %d (lsb16 of sw_if_index %d) l3 %s%s %U -> %U"
-                " l4 proto %d l4_valid %d port %d -> %d tcp flags (%s) %02x rsvd %x",
-                p5t->pkt.lc_index, p5t->l4.lsb_of_sw_if_index,
-                p5t->pkt.is_ip6 ? "ip6" : "ip4",
-                p5t->pkt.is_nonfirst_fragment ? " non-initial fragment" : "",
-                format_ip46_address, &p5t->addr[0],
-                p5t->pkt.is_ip6 ? IP46_TYPE_IP6 : IP46_TYPE_IP4,
-                format_ip46_address, &p5t->addr[1],
-                p5t->pkt.is_ip6 ? IP46_TYPE_IP6 : IP46_TYPE_IP4,
-                p5t->l4.proto, p5t->pkt.l4_valid, p5t->l4.port[0],
-                p5t->l4.port[1],
-                p5t->pkt.tcp_flags_valid ? "valid" : "invalid",
-                p5t->pkt.tcp_flags, p5t->pkt.flags_reserved);
+  if (p5t->pkt.is_ip6)
+    return format (s, "lc_index %d (lsb16 of sw_if_index %d) l3 %s%s %U -> %U"
+                  " l4 proto %d l4_valid %d port %d -> %d tcp flags (%s) %02x rsvd %x",
+                  p5t->pkt.lc_index, p5t->l4.lsb_of_sw_if_index,
+                  "ip6",
+                  p5t->
+                  pkt.is_nonfirst_fragment ? " non-initial fragment" : "",
+                  format_ip6_address, &p5t->ip6_addr[0], format_ip6_address,
+                  &p5t->ip6_addr[1], p5t->l4.proto, p5t->pkt.l4_valid,
+                  p5t->l4.port[0], p5t->l4.port[1],
+                  p5t->pkt.tcp_flags_valid ? "valid" : "invalid",
+                  p5t->pkt.tcp_flags, p5t->pkt.flags_reserved);
+  else
+    return format (s, "lc_index %d (lsb16 of sw_if_index %d) l3 %s%s %U -> %U"
+                  " l4 proto %d l4_valid %d port %d -> %d tcp flags (%s) %02x rsvd %x",
+                  p5t->pkt.lc_index, p5t->l4.lsb_of_sw_if_index,
+                  "ip4",
+                  p5t->
+                  pkt.is_nonfirst_fragment ? " non-initial fragment" : "",
+                  format_ip4_address, &p5t->ip4_addr[0], format_ip4_address,
+                  &p5t->ip4_addr[1], p5t->l4.proto, p5t->pkt.l4_valid,
+                  p5t->l4.port[0], p5t->l4.port[1],
+                  p5t->pkt.tcp_flags_valid ? "valid" : "invalid",
+                  p5t->pkt.tcp_flags, p5t->pkt.flags_reserved);
 }
 
 u8 *
index 5c55cb9..ba08044 100644 (file)
@@ -54,7 +54,17 @@ typedef union {
 
 typedef union {
   struct {
-    ip46_address_t addr[2];
+    union {
+      struct {
+        /* we put the IPv4 addresses
+           after padding so we can still
+           use them as (shorter) key together with
+           L4 info */
+        u32 l3_zero_pad[6];
+        ip4_address_t ip4_addr[2];
+      };
+      ip6_address_t ip6_addr[2];
+    };
     fa_session_l4_key_t l4;
     /* This field should align with u64 value in bihash_40_8 keyvalue struct */
     fa_packet_info_t pkt;
@@ -81,7 +91,8 @@ typedef struct {
   u32 link_next_idx;      /* +4 bytes = 16 */
   u8 link_list_id;        /* +1 bytes = 17 */
   u8 deleted;             /* +1 bytes = 18 */
-  u8 reserved1[6];        /* +6 bytes = 24 */
+  u8 is_ip6;              /* +1 bytes = 19 */
+  u8 reserved1[5];        /* +5 bytes = 24 */
   u64 reserved2[5];       /* +5*8 bytes = 64 */
 } fa_session_t;
 
index 9a28003..4bcd905 100644 (file)
@@ -514,15 +514,33 @@ hash_acl_reapply(acl_main_t *am, u32 lc_index, int acl_index)
 }
 
 static void
-make_address_mask(ip46_address_t *addr, u8 is_ipv6, u8 prefix_len)
+make_ip6_address_mask(ip6_address_t *addr, u8 prefix_len)
 {
-  if (is_ipv6) {
-    ip6_address_mask_from_width(&addr->ip6, prefix_len);
-  } else {
-    /* FIXME: this may not be correct way */
-    ip6_address_mask_from_width(&addr->ip6, prefix_len + 3*32);
-    ip46_address_mask_ip4(addr);
-  }
+  ip6_address_mask_from_width(addr, prefix_len);
+}
+
+
+/* Maybe should be moved into the core somewhere */
+always_inline void
+ip4_address_mask_from_width (ip4_address_t * a, u32 width)
+{
+  int i, byte, bit, bitnum;
+  ASSERT (width <= 32);
+  memset (a, 0, sizeof (a[0]));
+  for (i = 0; i < width; i++)
+    {
+      bitnum = (7 - (i & 7));
+      byte = i / 8;
+      bit = 1 << bitnum;
+      a->as_u8[byte] |= bit;
+    }
+}
+
+
+static void
+make_ip4_address_mask(ip4_address_t *addr, u8 prefix_len)
+{
+  ip4_address_mask_from_width(addr, prefix_len);
 }
 
 static u8
@@ -566,11 +584,18 @@ make_mask_and_match_from_rule(fa_5tuple_t *mask, acl_rule_t *r, hash_ace_info_t
 
   mask->pkt.is_ip6 = 1;
   hi->match.pkt.is_ip6 = r->is_ipv6;
-
-  make_address_mask(&mask->addr[0], r->is_ipv6, r->src_prefixlen);
-  hi->match.addr[0] = r->src;
-  make_address_mask(&mask->addr[1], r->is_ipv6, r->dst_prefixlen);
-  hi->match.addr[1] = r->dst;
+  if (r->is_ipv6) {
+    make_ip6_address_mask(&mask->ip6_addr[0], r->src_prefixlen);
+    hi->match.ip6_addr[0] = r->src.ip6;
+    make_ip6_address_mask(&mask->ip6_addr[1], r->dst_prefixlen);
+    hi->match.ip6_addr[1] = r->dst.ip6;
+  } else {
+    memset(hi->match.l3_zero_pad, 0, sizeof(hi->match.l3_zero_pad));
+    make_ip4_address_mask(&mask->ip4_addr[0], r->src_prefixlen);
+    hi->match.ip4_addr[0] = r->src.ip4;
+    make_ip4_address_mask(&mask->ip4_addr[1], r->dst_prefixlen);
+    hi->match.ip4_addr[1] = r->dst.ip4;
+  }
 
   if (r->proto != 0) {
     mask->l4.proto = ~0; /* L4 proto needs to be matched */
index e7b0852..6b464b3 100644 (file)
@@ -214,11 +214,11 @@ acl_fill_5tuple (acl_main_t * am, vlib_buffer_t * b0, int is_ip6,
 
   if (is_ip6)
     {
-      clib_memcpy (&p5tuple_pkt->addr,
+      clib_memcpy (&p5tuple_pkt->ip6_addr,
                   get_ptr_to_offset (b0,
                                      offsetof (ip6_header_t,
                                                src_address) + l3_offset),
-                  sizeof (p5tuple_pkt->addr));
+                  sizeof (p5tuple_pkt->ip6_addr));
       proto =
        *(u8 *) get_ptr_to_offset (b0,
                                   offsetof (ip6_header_t,
@@ -270,18 +270,12 @@ acl_fill_5tuple (acl_main_t * am, vlib_buffer_t * b0, int is_ip6,
     }
   else
     {
-      ip46_address_mask_ip4(&p5tuple_pkt->addr[0]);
-      ip46_address_mask_ip4(&p5tuple_pkt->addr[1]);
-      clib_memcpy (&p5tuple_pkt->addr[0].ip4,
+      memset(p5tuple_pkt->l3_zero_pad, 0, sizeof(p5tuple_pkt->l3_zero_pad));
+      clib_memcpy (&p5tuple_pkt->ip4_addr,
                   get_ptr_to_offset (b0,
                                      offsetof (ip4_header_t,
                                                src_address) + l3_offset),
-                  sizeof (p5tuple_pkt->addr[0].ip4));
-      clib_memcpy (&p5tuple_pkt->addr[1].ip4,
-                  get_ptr_to_offset (b0,
-                                     offsetof (ip4_header_t,
-                                               dst_address) + l3_offset),
-                  sizeof (p5tuple_pkt->addr[1].ip4));
+                  sizeof (p5tuple_pkt->ip4_addr));
       proto =
        *(u8 *) get_ptr_to_offset (b0,
                                   offsetof (ip4_header_t,
@@ -359,16 +353,29 @@ acl_plugin_fill_5tuple_inline (u32 lc_index, vlib_buffer_t * b0, int is_ip6,
 
 
 always_inline int
-fa_acl_match_addr (ip46_address_t * addr1, ip46_address_t * addr2,
-                  int prefixlen, int is_ip6)
+fa_acl_match_ip4_addr (ip4_address_t * addr1, ip4_address_t * addr2,
+                  int prefixlen)
 {
   if (prefixlen == 0)
     {
       /* match any always succeeds */
       return 1;
     }
-  if (is_ip6)
+      uint32_t a1 = clib_net_to_host_u32 (addr1->as_u32);
+      uint32_t a2 = clib_net_to_host_u32 (addr2->as_u32);
+      uint32_t mask0 = 0xffffffff - ((1 << (32 - prefixlen)) - 1);
+      return (a1 & mask0) == a2;
+}
+
+always_inline int
+fa_acl_match_ip6_addr (ip6_address_t * addr1, ip6_address_t * addr2,
+                  int prefixlen)
+{
+  if (prefixlen == 0)
     {
+      /* match any always succeeds */
+      return 1;
+    }
       if (memcmp (addr1, addr2, prefixlen / 8))
        {
          /* If the starting full bytes do not match, no point in bittwidling the thumbs further */
@@ -386,14 +393,6 @@ fa_acl_match_addr (ip46_address_t * addr1, ip46_address_t * addr2,
          /* The prefix fits into integer number of bytes, so nothing left to do */
          return 1;
        }
-    }
-  else
-    {
-      uint32_t a1 = clib_net_to_host_u32 (addr1->ip4.as_u32);
-      uint32_t a2 = clib_net_to_host_u32 (addr2->ip4.as_u32);
-      uint32_t mask0 = 0xffffffff - ((1 << (32 - prefixlen)) - 1);
-      return (a1 & mask0) == a2;
-    }
 }
 
 always_inline int
@@ -424,41 +423,26 @@ single_acl_match_5tuple (acl_main_t * am, u32 acl_index, fa_5tuple_t * pkt_5tupl
   for (i = 0; i < a->count; i++)
     {
       r = a->rules + i;
-#ifdef FA_NODE_VERBOSE_DEBUG
-      clib_warning("ACL_FA_NODE_DBG acl %d rule %d tag %s", acl_index, i, a->tag);
-#endif
       if (is_ip6 != r->is_ipv6)
        {
          continue;
        }
-      if (!fa_acl_match_addr
-         (&pkt_5tuple->addr[1], &r->dst, r->dst_prefixlen, is_ip6))
+      if (is_ip6) {
+        if (!fa_acl_match_ip6_addr
+         (&pkt_5tuple->ip6_addr[1], &r->dst.ip6, r->dst_prefixlen))
        continue;
-
-#ifdef FA_NODE_VERBOSE_DEBUG
-      clib_warning
-       ("ACL_FA_NODE_DBG acl %d rule %d pkt dst addr %U match rule addr %U/%d",
-        acl_index, i, format_ip46_address, &pkt_5tuple->addr[1],
-        r->is_ipv6 ? IP46_TYPE_IP6: IP46_TYPE_IP4, format_ip46_address,
-         &r->dst, r->is_ipv6 ? IP46_TYPE_IP6: IP46_TYPE_IP4,
-        r->dst_prefixlen);
-#endif
-
-      if (!fa_acl_match_addr
-         (&pkt_5tuple->addr[0], &r->src, r->src_prefixlen, is_ip6))
+        if (!fa_acl_match_ip6_addr
+         (&pkt_5tuple->ip6_addr[0], &r->src.ip6, r->src_prefixlen))
+       continue;
+      } else {
+        if (!fa_acl_match_ip4_addr
+         (&pkt_5tuple->ip4_addr[1], &r->dst.ip4, r->dst_prefixlen))
+       continue;
+        if (!fa_acl_match_ip4_addr
+         (&pkt_5tuple->ip4_addr[0], &r->src.ip4, r->src_prefixlen))
        continue;
+      }
 
-#ifdef FA_NODE_VERBOSE_DEBUG
-      clib_warning
-       ("ACL_FA_NODE_DBG acl %d rule %d pkt src addr %U match rule addr %U/%d",
-        acl_index, i, format_ip46_address, &pkt_5tuple->addr[0],
-        r->is_ipv6 ? IP46_TYPE_IP6: IP46_TYPE_IP4, format_ip46_address,
-         &r->src, r->is_ipv6 ? IP46_TYPE_IP6: IP46_TYPE_IP4,
-        r->src_prefixlen);
-      clib_warning
-       ("ACL_FA_NODE_DBG acl %d rule %d trying to match pkt proto %d with rule %d",
-        acl_index, i, pkt_5tuple->l4.proto, r->proto);
-#endif
       if (r->proto)
        {
          if (pkt_5tuple->l4.proto != r->proto)
index 465111a..6c01643 100644 (file)
@@ -53,18 +53,26 @@ format_session_bihash_5tuple (u8 * s, va_list * args)
 {
   fa_5tuple_t *p5t = va_arg (*args, fa_5tuple_t *);
   fa_full_session_id_t *sess = (void *) &p5t->pkt;
-
-  return format (s, "l3 %U -> %U"
-                " l4 lsb_of_sw_if_index %d proto %d l4_is_input %d l4_slow_path %d l4_reserved0 %d port %d -> %d | sess id %d thread id %d epoch %04x",
-                format_ip46_address, &p5t->addr[0],
-                IP46_TYPE_ANY,
-                format_ip46_address, &p5t->addr[1],
-                IP46_TYPE_ANY,
-                p5t->l4.lsb_of_sw_if_index,
-                p5t->l4.proto, p5t->l4.is_input, p5t->l4.is_slowpath,
-                p5t->l4.reserved0, p5t->l4.port[0], p5t->l4.port[1],
-                sess->session_index, sess->thread_index,
-                sess->intf_policy_epoch);
+  if (is_ip6_5tuple (p5t))
+    return (format (s, "l3 %U -> %U"
+                   " l4 lsb_of_sw_if_index %d proto %d l4_is_input %d l4_slow_path %d l4_reserved0 %d port %d -> %d | sess id %d thread id %d epoch %04x",
+                   format_ip6_address, &p5t->ip6_addr[0],
+                   format_ip6_address, &p5t->ip6_addr[1],
+                   p5t->l4.lsb_of_sw_if_index,
+                   p5t->l4.proto, p5t->l4.is_input, p5t->l4.is_slowpath,
+                   p5t->l4.reserved0, p5t->l4.port[0], p5t->l4.port[1],
+                   sess->session_index, sess->thread_index,
+                   sess->intf_policy_epoch));
+  else
+    return (format (s, "l3 %U -> %U"
+                   " l4 lsb_of_sw_if_index %d proto %d l4_is_input %d l4_slow_path %d l4_reserved0 %d port %d -> %d | sess id %d thread id %d epoch %04x",
+                   format_ip4_address, &p5t->ip4_addr[0],
+                   format_ip4_address, &p5t->ip4_addr[1],
+                   p5t->l4.lsb_of_sw_if_index,
+                   p5t->l4.proto, p5t->l4.is_input, p5t->l4.is_slowpath,
+                   p5t->l4.reserved0, p5t->l4.port[0], p5t->l4.port[1],
+                   sess->session_index, sess->thread_index,
+                   sess->intf_policy_epoch));
 }
 
 
index 709ecc8..01dface 100644 (file)
@@ -262,6 +262,16 @@ acl_fa_restart_timer_for_session (acl_main_t * am, u64 now,
     }
 }
 
+always_inline int
+is_ip6_5tuple (fa_5tuple_t * p5t)
+{
+  return (p5t->l3_zero_pad[0] | p5t->
+         l3_zero_pad[1] | p5t->l3_zero_pad[2] | p5t->l3_zero_pad[3] | p5t->
+         l3_zero_pad[4] | p5t->l3_zero_pad[5]) != 0;
+}
+
+
+
 
 always_inline u8
 acl_fa_track_session (acl_main_t * am, int is_input, u32 sw_if_index, u64 now,
@@ -355,15 +365,24 @@ reverse_l4_u64 (u64 l4, int is_ip6)
 }
 
 always_inline void
-reverse_session_add_del (acl_main_t * am, const int is_ip6,
+reverse_session_add_del (acl_main_t * am, int is_ip6,
                         clib_bihash_kv_40_8_t * pkv, int is_add)
 {
   clib_bihash_kv_40_8_t kv2;
-  /* the first 4xu64 is two addresses, so just swap them */
-  kv2.key[0] = pkv->key[2];
-  kv2.key[1] = pkv->key[3];
-  kv2.key[2] = pkv->key[0];
-  kv2.key[3] = pkv->key[1];
+  if (is_ip6)
+    {
+      kv2.key[0] = pkv->key[2];
+      kv2.key[1] = pkv->key[3];
+      kv2.key[2] = pkv->key[0];
+      kv2.key[3] = pkv->key[1];
+    }
+  else
+    {
+      kv2.key[0] = kv2.key[1] = kv2.key[2] = 0;
+      kv2.key[3] =
+       ((pkv->key[3] & 0xffffffff) << 32) | ((pkv->key[3] >> 32) &
+                                             0xffffffff);
+    }
   /* the last u64 needs special treatment (ports, etc.) */
   kv2.key[4] = reverse_l4_u64 (pkv->key[4], is_ip6);
   kv2.value = pkv->value;
@@ -379,7 +398,7 @@ acl_fa_deactivate_session (acl_main_t * am, u32 sw_if_index,
   ASSERT (sess->thread_index == os_get_thread_index ());
   clib_bihash_add_del_40_8 (&am->fa_sessions_hash, &sess->info.kv, 0);
 
-  reverse_session_add_del (am, sess->info.pkt.is_ip6, &sess->info.kv, 0);
+  reverse_session_add_del (am, sess->is_ip6, &sess->info.kv, 0);
   sess->deleted = 1;
   clib_smp_atomic_add (&am->fa_session_total_deactivations, 1);
 }
@@ -513,6 +532,7 @@ acl_fa_add_session (acl_main_t * am, int is_input, int is_ip6,
   sess->link_prev_idx = FA_SESSION_BOGUS_INDEX;
   sess->link_next_idx = FA_SESSION_BOGUS_INDEX;
   sess->deleted = 0;
+  sess->is_ip6 = is_ip6;
 
   acl_fa_conn_list_add_session (am, f_sess_id, now);