gbp: Ownership of dynamically created vxlan-gbp tunnels managed via gbp_itf

Type: fix

This solves the ownership of vxlan-gbp tunnels. When the last reference of these goes away they need to be deleted. Currently there are two owners; gbp_itf via gef_itf and the lock held by the gbp_endpoint_location_t. The problem is that the
loc removes its reference whilst the fwd still holds the gbp_itf, and things go wrong.
This change moves the lifecycle management of the vxlan-gbp tunnel to the gbp_itf. When the last lock of the gbp_itf goes, so does the tunnel. now both the EP's loc and fwd can hold a lock on the gbp_itf and it's only removed when required.
The other change is the management of the 'user' of the gbp_itf. Since each user can enable and disable different features, it's the job of the gbp_itf to apply the combined set. determining a unique 'uesr' from the caller was near impossible, so I moved that to the gbp_itf, and return the allocated user, hence the 'handle' that encodes both user and interface.

The hash table maps from sw_if_index to pool index.

Change-Id: I4c7bf4c0e5dcf33d1c545f262365e69151febcf4
Signed-off-by: Neale Ranns <nranns@cisco.com>

1 files changed, 37 insertions, 46 deletions

diff --git a/src/plugins/gbp/gbp_endpoint.c b/src/plugins/gbp/gbp_endpoint.c
index d759da2bece..836930accbe 100644
--- a/src/plugins/gbp/gbp_endpoint.c
+++ b/src/plugins/gbp/gbp_endpoint.c
@@ -264,8 +264,8 @@ gbp_endpoint_alloc (const ip46_address_t * ips,
   fib_node_init (&ge->ge_node, gbp_endpoint_fib_type);
   gei = gbp_endpoint_index (ge);
   ge->ge_key.gek_gbd =
-    ge->ge_key.gek_grd =
-    ge->ge_fwd.gef_itf = ge->ge_fwd.gef_fib_index = INDEX_INVALID;
+    ge->ge_key.gek_grd = ge->ge_fwd.gef_fib_index = INDEX_INVALID;
+  gbp_itf_hdl_reset (&ge->ge_fwd.gef_itf);
   ge->ge_last_time = vlib_time_now (vlib_get_main ());
   ge->ge_key.gek_gbd = gbp_bridge_domain_index (gbd);
 
@@ -336,11 +336,7 @@ static void
 gbp_endpoint_loc_destroy (gbp_endpoint_loc_t * gel)
 {
   gbp_endpoint_group_unlock (gel->gel_epg);
-
-  if (gel->gel_flags & GBP_ENDPOINT_FLAG_REMOTE)
-    {
-      vxlan_gbp_tunnel_unlock (gel->gel_sw_if_index);
-    }
+  gbp_itf_unlock (&gel->gel_itf);
 }
 
 static gbp_endpoint_loc_t *
@@ -349,7 +345,7 @@ gbp_endpoint_loc_find_or_add (gbp_endpoint_t * ge, gbp_endpoint_src_t src)
   gbp_endpoint_loc_t gel = {
     .gel_src = src,
     .gel_epg = INDEX_INVALID,
-    .gel_sw_if_index = INDEX_INVALID,
+    .gel_itf = GBP_ITF_HDL_INVALID,
     .gel_locks = 0,
   };
   u32 pos;
@@ -484,7 +480,8 @@ gbp_endpoint_n_learned (int n)
 }
 
 static void
-gbp_endpoint_loc_update (gbp_endpoint_loc_t * gel,
+gbp_endpoint_loc_update (const gbp_endpoint_t * ge,
+			 gbp_endpoint_loc_t * gel,
 			 const gbp_bridge_domain_t * gb,
 			 u32 sw_if_index,
 			 index_t ggi,
@@ -544,29 +541,29 @@ gbp_endpoint_loc_update (gbp_endpoint_loc_t * gel,
        * create a child vlxan-gbp tunnel and use that as the endpoint's
        * interface.
        */
-      if (~0 != gel->gel_sw_if_index)
-	vxlan_gbp_tunnel_unlock (gel->gel_sw_if_index);
+      gbp_itf_hdl_t old = gel->gel_itf;
 
       switch (gbp_vxlan_tunnel_get_type (sw_if_index))
 	{
 	case GBP_VXLAN_TEMPLATE_TUNNEL:
 	  gel->tun.gel_parent_sw_if_index = sw_if_index;
-	  gel->gel_sw_if_index =
-	    gbp_vxlan_tunnel_clone_and_lock (sw_if_index,
-					     &gel->tun.gel_src,
-					     &gel->tun.gel_dst);
+	  gel->gel_itf = gbp_vxlan_tunnel_clone_and_lock (sw_if_index,
+							  &gel->tun.gel_src,
+							  &gel->tun.gel_dst);
 	  break;
 	case VXLAN_GBP_TUNNEL:
 	  gel->tun.gel_parent_sw_if_index =
 	    vxlan_gbp_tunnel_get_parent (sw_if_index);
-	  gel->gel_sw_if_index = sw_if_index;
-	  vxlan_gbp_tunnel_lock (gel->gel_sw_if_index);
+	  gel->gel_itf = vxlan_gbp_tunnel_lock_itf (sw_if_index);
 	  break;
 	}
+
+      gbp_itf_unlock (&old);
     }
   else
     {
-      gel->gel_sw_if_index = sw_if_index;
+      gel->gel_itf = gbp_itf_l2_add_and_lock (sw_if_index,
+					      ge->ge_key.gek_gbd);
     }
 }
 
@@ -578,9 +575,7 @@ gbb_endpoint_fwd_reset (gbp_endpoint_t * ge)
   gbp_endpoint_fwd_t *gef;
   const fib_prefix_t *pfx;
   index_t *ai;
-  index_t gei;
 
-  gei = gbp_endpoint_index (ge);
   gbd = gbp_bridge_domain_get (ge->ge_key.gek_gbd);
   gef = &ge->ge_fwd;
 
@@ -609,26 +604,23 @@ gbb_endpoint_fwd_reset (gbp_endpoint_t * ge)
     adj_unlock (*ai);
   }
 
-  if (INDEX_INVALID != gef->gef_itf)
+  if (gbp_itf_hdl_is_valid (gef->gef_itf))
     {
       l2fib_del_entry (ge->ge_key.gek_mac.bytes,
-		       gbd->gb_bd_index, gef->gef_itf);
-      gbp_itf_set_l2_input_feature (gef->gef_itf, gei, L2INPUT_FEAT_NONE);
-      gbp_itf_set_l2_output_feature (gef->gef_itf, gei, L2OUTPUT_FEAT_NONE);
-
-      gbp_itf_unlock (gef->gef_itf);
-      gef->gef_itf = INDEX_INVALID;
+		       gbd->gb_bd_index,
+		       gbp_itf_get_sw_if_index (gef->gef_itf));
     }
 
+  gbp_itf_unlock (&gef->gef_itf);
   vec_free (gef->gef_adjs);
 }
 
 static void
 gbb_endpoint_fwd_recalc (gbp_endpoint_t * ge)
 {
-  const gbp_route_domain_t *grd;
   const gbp_bridge_domain_t *gbd;
   const gbp_endpoint_group_t *gg;
+  const gbp_route_domain_t *grd;
   gbp_endpoint_loc_t *gel;
   gbp_endpoint_fwd_t *gef;
   const fib_prefix_t *pfx;
@@ -654,12 +646,11 @@ gbb_endpoint_fwd_recalc (gbp_endpoint_t * ge)
       gg = NULL;
     }
 
-  gef->gef_itf = gbp_itf_add_and_lock (gel->gel_sw_if_index,
-				       gbd->gb_bd_index);
+  gef->gef_itf = gbp_itf_clone_and_lock (gel->gel_itf);
 
   if (!mac_address_is_zero (&ge->ge_key.gek_mac))
     {
-      gbp_itf_set_l2_input_feature (gef->gef_itf, gei, L2INPUT_FEAT_GBP_FWD);
+      gbp_itf_l2_set_input_feature (gef->gef_itf, L2INPUT_FEAT_GBP_FWD);
 
       if (gbp_endpoint_is_remote (ge) || gbp_endpoint_is_external (ge))
 	{
@@ -667,18 +658,19 @@ gbb_endpoint_fwd_recalc (gbp_endpoint_t * ge)
 	   * bridged packets to external endpoints should be classifed
 	   * based on the EP's/BD's EPG
 	   */
-	  gbp_itf_set_l2_output_feature (gef->gef_itf, gei,
+	  gbp_itf_l2_set_output_feature (gef->gef_itf,
 					 L2OUTPUT_FEAT_GBP_POLICY_MAC);
 	}
       else
 	{
-	  gbp_endpoint_add_itf (gef->gef_itf, gei);
-	  gbp_itf_set_l2_output_feature (gef->gef_itf, gei,
+	  gbp_endpoint_add_itf (gbp_itf_get_sw_if_index (gef->gef_itf), gei);
+	  gbp_itf_l2_set_output_feature (gef->gef_itf,
 					 L2OUTPUT_FEAT_GBP_POLICY_PORT);
 	}
       l2fib_add_entry (ge->ge_key.gek_mac.bytes,
 		       gbd->gb_bd_index,
-		       gef->gef_itf, L2FIB_ENTRY_RESULT_FLAG_STATIC);
+		       gbp_itf_get_sw_if_index (gef->gef_itf),
+		       L2FIB_ENTRY_RESULT_FLAG_STATIC);
     }
 
   vec_foreach (pfx, ge->ge_key.gek_ips)
@@ -716,7 +708,7 @@ gbb_endpoint_fwd_recalc (gbp_endpoint_t * ge)
 	 * tunnel since the BD will not contain the EP's MAC since it was
 	 * L3 learned. The dst MAC address used is the 'BD's MAC'.
 	 */
-	ip_sw_if_index = gef->gef_itf;
+	ip_sw_if_index = gbp_itf_get_sw_if_index (gef->gef_itf);
 
 	mac_address_to_bytes (gbp_route_domain_get_local_mac (),
 			      eth->src_address);
@@ -786,7 +778,8 @@ gbb_endpoint_fwd_recalc (gbp_endpoint_t * ge)
 	 */
 	if (gbp_endpoint_is_local (ge) && ~0 != gg->gg_uplink_sw_if_index)
 	  {
-	    gbp_endpoint_add_itf (gef->gef_itf, gei);
+	    gbp_endpoint_add_itf (gbp_itf_get_sw_if_index (gef->gef_itf),
+				  gei);
 	    if (FIB_PROTOCOL_IP4 == pfx->fp_proto)
 	      send_ip4_garp_w_addr (vlib_get_main (),
 				    &pfx->fp_addr.ip4,
@@ -801,7 +794,7 @@ gbb_endpoint_fwd_recalc (gbp_endpoint_t * ge)
 
   if (gbp_endpoint_is_external (ge))
     {
-      gbp_itf_set_l2_input_feature (gef->gef_itf, gei,
+      gbp_itf_l2_set_input_feature (gef->gef_itf,
 				    L2INPUT_FEAT_GBP_LPM_CLASSIFY);
     }
   else if (gbp_endpoint_is_local (ge))
@@ -817,7 +810,7 @@ gbb_endpoint_fwd_recalc (gbp_endpoint_t * ge)
 
       if (NULL != gg && ~0 != gg->gg_uplink_sw_if_index)
 	feats |= L2INPUT_FEAT_GBP_FWD;
-      gbp_itf_set_l2_input_feature (gef->gef_itf, gei, feats);
+      gbp_itf_l2_set_input_feature (gef->gef_itf, feats);
     }
 
   /*
@@ -901,8 +894,8 @@ gbp_endpoint_update_and_lock (gbp_endpoint_src_t src,
   gei = gbp_endpoint_index (ge);
   gel = gbp_endpoint_loc_find_or_add (ge, src);
 
-  gbp_endpoint_loc_update (gel, gbd, sw_if_index, ggi, flags, tun_src,
-			   tun_dst);
+  gbp_endpoint_loc_update (ge, gel, gbd, sw_if_index, ggi, flags,
+			   tun_src, tun_dst);
 
   if (src <= best)
     {
@@ -1193,7 +1186,7 @@ format_gbp_endpoint_fwd (u8 * s, va_list * args)
   gbp_endpoint_fwd_t *gef = va_arg (*args, gbp_endpoint_fwd_t *);
 
   s = format (s, "fwd:");
-  s = format (s, "\n   itf:[%U]", format_gbp_itf, gef->gef_itf);
+  s = format (s, "\n   itf:[%U]", format_gbp_itf_hdl, gef->gef_itf);
   if (GBP_ENDPOINT_FLAG_NONE != gef->gef_flags)
     {
       s = format (s, " flags:%U", format_gbp_endpoint_flags, gef->gef_flags);
@@ -1227,10 +1220,8 @@ format_gbp_endpoint_loc (u8 * s, va_list * args)
   gbp_endpoint_loc_t *gel = va_arg (*args, gbp_endpoint_loc_t *);
 
   s = format (s, "%U", format_gbp_endpoint_src, gel->gel_src);
-  s =
-    format (s, "\n    %U", format_vnet_sw_if_index_name, vnet_get_main (),
-	    gel->gel_sw_if_index);
-  s = format (s, " EPG:%d", gel->gel_epg);
+  s = format (s, "\n    EPG:%d [%U]", gel->gel_epg,
+	      format_gbp_itf_hdl, gel->gel_itf);
 
   if (GBP_ENDPOINT_FLAG_NONE != gel->gel_flags)
     {