session: delete and add application namespace do not create the global session table

When an application namespace is added, we call session_table_is_alloced
to see if we need to allocate a new session table. That check returns true
even if we removed the session table.
The fix is when we delete an application's global session table,
we need to invalidate fib_index_to_table_index.

Fixed test_vcl test script to run two tests back to back.
The 1st test deletes the application namespace at the end.
The 2nd test adds the application namespace in the beginning.

Type: fix
Fixes: 67bae20b05cb46e5f6d19afeaf1f7a52a5309d59

Change-Id: I67f5cc1b726a07659597a9479df011717db08d0a
Signed-off-by: Steven Luong <sluong@cisco.com>

5 files changed, 56 insertions, 93 deletions

diff --git a/src/vnet/session/application_namespace.c b/src/vnet/session/application_namespace.c
index 2520188d88c..dbc6a6be80f 100644
--- a/src/vnet/session/application_namespace.c
+++ b/src/vnet/session/application_namespace.c
@@ -22,12 +22,6 @@
 #include <vppinfra/format_table.h>
 #include <vlib/unix/unix.h>
 
-/*
- * fib source when locking the fib table
- */
-static fib_source_t app_namespace_fib_src = FIB_SOURCE_INVALID;
-static u32 *fib_index_to_lock_count[FIB_PROTOCOL_IP6 + 1];
-
 /**
  * Hash table of application namespaces by app ns ids
  */
@@ -87,50 +81,6 @@ app_namespace_alloc (const u8 *ns_id)
   return app_ns;
 }
 
-static void
-app_namespace_fib_table_lock (u32 fib_index, u32 protocol)
-{
-  fib_table_lock (fib_index, protocol, app_namespace_fib_src);
-  vec_validate (fib_index_to_lock_count[protocol], fib_index);
-  fib_index_to_lock_count[protocol][fib_index]++;
-  ASSERT (fib_index_to_lock_count[protocol][fib_index] > 0);
-}
-
-static void
-app_namespace_fib_table_unlock (u32 fib_index, u32 protocol)
-{
-  fib_table_unlock (fib_index, protocol, app_namespace_fib_src);
-  ASSERT (fib_index_to_lock_count[protocol][fib_index] > 0);
-  fib_index_to_lock_count[protocol][fib_index]--;
-}
-
-static void
-app_namespace_del_global_table (app_namespace_t *app_ns)
-{
-  session_table_t *st;
-  u32 table_index;
-
-  app_namespace_fib_table_unlock (app_ns->ip4_fib_index, FIB_PROTOCOL_IP4);
-  if (fib_index_to_lock_count[FIB_PROTOCOL_IP4][app_ns->ip4_fib_index] == 0)
-    {
-      table_index = session_lookup_get_index_for_fib (FIB_PROTOCOL_IP4,
-						      app_ns->ip4_fib_index);
-      st = session_table_get (table_index);
-      if (st)
-	session_table_free (st, FIB_PROTOCOL_IP4);
-    }
-
-  app_namespace_fib_table_unlock (app_ns->ip6_fib_index, FIB_PROTOCOL_IP6);
-  if (fib_index_to_lock_count[FIB_PROTOCOL_IP6][app_ns->ip6_fib_index] == 0)
-    {
-      table_index = session_lookup_get_index_for_fib (FIB_PROTOCOL_IP6,
-						      app_ns->ip6_fib_index);
-      st = session_table_get (table_index);
-      if (st)
-	session_table_free (st, FIB_PROTOCOL_IP6);
-    }
-}
-
 session_error_t
 vnet_app_namespace_add_del (vnet_app_namespace_add_del_args_t *a)
 {
@@ -188,11 +138,7 @@ vnet_app_namespace_add_del (vnet_app_namespace_add_del_args_t *a)
       app_ns->sw_if_index = a->sw_if_index;
 
       app_ns->ip4_fib_index = fib_table_find (FIB_PROTOCOL_IP4, a->ip4_fib_id);
-      app_namespace_fib_table_lock (app_ns->ip4_fib_index, FIB_PROTOCOL_IP4);
-
       app_ns->ip6_fib_index = fib_table_find (FIB_PROTOCOL_IP6, a->ip6_fib_id);
-      app_namespace_fib_table_lock (app_ns->ip6_fib_index, FIB_PROTOCOL_IP6);
-
       session_lookup_set_tables_appns (app_ns);
     }
   else
@@ -216,7 +162,8 @@ vnet_app_namespace_add_del (vnet_app_namespace_add_del_args_t *a)
       if (app_ns->sock_name)
 	vec_free (app_ns->sock_name);
 
-      app_namespace_del_global_table (app_ns);
+      session_lookup_table_cleanup (FIB_PROTOCOL_IP4, app_ns->ip4_fib_index);
+      session_lookup_table_cleanup (FIB_PROTOCOL_IP6, app_ns->ip6_fib_index);
 
       app_namespace_free (app_ns);
     }
@@ -290,15 +237,6 @@ app_namespaces_init (void)
 {
   u8 *ns_id = format (0, "default");
 
-  /* We are not contributing any route to the fib. But we allocate a fib source
-   * so that when we lock the fib table, we can view that we have a lock on the
-   * particular fib table in case we wonder why the fib table is not free after
-   * "ip table del"
-   */
-  if (app_namespace_fib_src == FIB_SOURCE_INVALID)
-    app_namespace_fib_src = fib_source_allocate (
-      "application namespace", FIB_SOURCE_PRIORITY_LOW, FIB_SOURCE_BH_SIMPLE);
-
   if (!app_namespace_lookup_table)
     app_namespace_lookup_table =
       hash_create_vec (0, sizeof (u8), sizeof (uword));
diff --git a/src/vnet/session/session_lookup.c b/src/vnet/session/session_lookup.c
index ff6e5818ce5..1e58e5e7806 100644
--- a/src/vnet/session/session_lookup.c
+++ b/src/vnet/session/session_lookup.c
@@ -37,6 +37,8 @@ static session_lookup_main_t sl_main;
  */
 static u32 *fib_index_to_table_index[2];
 
+static u32 *fib_index_to_lock_count[2];
+
 /* 16 octets */
 typedef CLIB_PACKED (struct {
   union
@@ -1392,6 +1394,23 @@ vnet_session_rule_add_del (session_rule_add_del_args_t *args)
   return rv;
 }
 
+static void
+session_lookup_fib_table_lock (u32 fib_index, u32 protocol)
+{
+  fib_table_lock (fib_index, protocol, sl_main.fib_src);
+  vec_validate (fib_index_to_lock_count[protocol], fib_index);
+  fib_index_to_lock_count[protocol][fib_index]++;
+  ASSERT (fib_index_to_lock_count[protocol][fib_index] > 0);
+}
+
+static void
+session_lookup_fib_table_unlock (u32 fib_index, u32 protocol)
+{
+  fib_table_unlock (fib_index, protocol, sl_main.fib_src);
+  ASSERT (fib_index_to_lock_count[protocol][fib_index] > 0);
+  fib_index_to_lock_count[protocol][fib_index]--;
+}
+
 /**
  * Mark (global) tables as pertaining to app ns
  */
@@ -1407,7 +1426,10 @@ session_lookup_set_tables_appns (app_namespace_t * app_ns)
       fib_index = app_namespace_get_fib_index (app_ns, fp);
       st = session_table_get_or_alloc (fp, fib_index);
       if (st)
-	st->appns_index = app_namespace_index (app_ns);
+	{
+	  st->appns_index = app_namespace_index (app_ns);
+	  session_lookup_fib_table_lock (fib_index, fp);
+	}
     }
 }
 
@@ -1859,6 +1881,14 @@ session_lookup_init (void)
 
   clib_spinlock_init (&slm->st_alloc_lock);
 
+  /* We are not contributing any route to the fib. But we allocate a fib source
+   * so that when we lock the fib table, we can view that we have a lock on the
+   * particular fib table in case we wonder why the fib table is not free after
+   * "ip table del"
+   */
+  slm->fib_src = fib_source_allocate (
+    "session lookup", FIB_SOURCE_PRIORITY_LOW, FIB_SOURCE_BH_SIMPLE);
+
   /*
    * Allocate default table and map it to fib_index 0
    */
@@ -1874,6 +1904,26 @@ session_lookup_init (void)
   session_table_init (st, FIB_PROTOCOL_IP6);
 }
 
+void
+session_lookup_table_cleanup (u32 fib_proto, u32 fib_index)
+{
+  session_table_t *st;
+  u32 table_index;
+
+  session_lookup_fib_table_unlock (fib_index, fib_proto);
+  if (fib_index_to_lock_count[fib_proto][fib_index] == 0)
+    {
+      table_index = session_lookup_get_index_for_fib (fib_proto, fib_index);
+      st = session_table_get (table_index);
+      if (st)
+	{
+	  session_table_free (st, fib_proto);
+	  if (vec_len (fib_index_to_table_index[fib_proto]) > fib_index)
+	    fib_index_to_table_index[fib_proto][fib_index] = ~0;
+	}
+    }
+}
+
 /*
  * fd.io coding-style-patch-verification: ON
  *
diff --git a/src/vnet/session/session_lookup.h b/src/vnet/session/session_lookup.h
index f9ffc15165a..95fcca6ff69 100644
--- a/src/vnet/session/session_lookup.h
+++ b/src/vnet/session/session_lookup.h
@@ -32,6 +32,7 @@ typedef enum session_lookup_result_
 typedef struct session_lookup_main_
 {
   clib_spinlock_t st_alloc_lock;
+  fib_source_t fib_src;
 } session_lookup_main_t;
 
 session_t *session_lookup_safe4 (u32 fib_index, ip4_address_t * lcl,
diff --git a/src/vnet/session/session_table.h b/src/vnet/session/session_table.h
index 636b8d77bee..bcbb6ec3037 100644
--- a/src/vnet/session/session_table.h
+++ b/src/vnet/session/session_table.h
@@ -78,6 +78,8 @@ session_table_t *_get_session_tables ();
 #define session_table_foreach(VAR, BODY)		\
   pool_foreach (VAR, _get_session_tables ()) BODY
 
+void session_lookup_table_cleanup (u32 fib_proto, u32 fib_index);
+
 #endif /* SRC_VNET_SESSION_SESSION_TABLE_H_ */
 /*
  * fd.io coding-style-patch-verification: ON
diff --git a/test/asf/test_vcl.py b/test/asf/test_vcl.py
index a186c6fe8c5..11b77aeb0cc 100644
--- a/test/asf/test_vcl.py
+++ b/test/asf/test_vcl.py
@@ -1006,34 +1006,6 @@ class LDPThruHostStackIperf(VCLTestCase):
             iperf3, self.server_iperf3_args, iperf3, self.client_iperf3_args
         )
 
-
-class LDPThruHostStackIperfMss(VCLTestCase):
-    """LDP Thru Host Stack Iperf with MSS"""
-
-    @classmethod
-    def setUpClass(cls):
-        super(LDPThruHostStackIperfMss, cls).setUpClass()
-
-    @classmethod
-    def tearDownClass(cls):
-        super(LDPThruHostStackIperfMss, cls).tearDownClass()
-
-    def setUp(self):
-        super(LDPThruHostStackIperfMss, self).setUp()
-
-        self.thru_host_stack_setup()
-        self.client_iperf3_timeout = 20
-        self.client_iperf3_args = ["-4", "-t 2", "-c", self.loop0.local_ip4]
-        self.server_iperf3_args = ["-4", "-s"]
-
-    def tearDown(self):
-        self.thru_host_stack_tear_down()
-        super(LDPThruHostStackIperfMss, self).tearDown()
-
-    def show_commands_at_teardown(self):
-        self.logger.debug(self.vapi.cli("show session verbose 2"))
-        self.logger.debug(self.vapi.cli("show app mq"))
-
     @unittest.skipUnless(_have_iperf3, "'%s' not found, Skipping.")
     def test_ldp_thru_host_stack_iperf3_mss(self):
         """run LDP thru host stack iperf3 test with mss option"""