acl-plugin: change the src/dst L3 info in 5tuple struct to be always contiguous with L4 data

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>

6 files changed, 156 insertions, 97 deletions

diff --git a/src/plugins/acl/dataplane_node.c b/src/plugins/acl/dataplane_node.c
index f1ed4c28b99..dead2ec131e 100644
--- a/src/plugins/acl/dataplane_node.c
+++ b/src/plugins/acl/dataplane_node.c
@@ -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 *
diff --git a/src/plugins/acl/fa_node.h b/src/plugins/acl/fa_node.h
index 5c55cb995a1..ba080446613 100644
--- a/src/plugins/acl/fa_node.h
+++ b/src/plugins/acl/fa_node.h
@@ -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;
 
diff --git a/src/plugins/acl/hash_lookup.c b/src/plugins/acl/hash_lookup.c
index 9a280031fc8..4bcd9050c73 100644
--- a/src/plugins/acl/hash_lookup.c
+++ b/src/plugins/acl/hash_lookup.c
@@ -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 */
diff --git a/src/plugins/acl/public_inlines.h b/src/plugins/acl/public_inlines.h
index e7b085271f6..6b464b36b7a 100644
--- a/src/plugins/acl/public_inlines.h
+++ b/src/plugins/acl/public_inlines.h
@@ -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)
diff --git a/src/plugins/acl/sess_mgmt_node.c b/src/plugins/acl/sess_mgmt_node.c
index 465111a380f..6c01643aca2 100644
--- a/src/plugins/acl/sess_mgmt_node.c
+++ b/src/plugins/acl/sess_mgmt_node.c
@@ -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));
 }
 
 
diff --git a/src/plugins/acl/session_inlines.h b/src/plugins/acl/session_inlines.h
index 709ecc8cae1..01dface323e 100644
--- a/src/plugins/acl/session_inlines.h
+++ b/src/plugins/acl/session_inlines.h
@@ -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);