VPP-651: Ensure sw_if_index to node mapping for L2 output path is only done via l2output_main.next_nodes

Before this commit, several output features that happen to be the
last in the list of features to be executed, send the packets directly
to <interfaceName>-output. To do this, they use l2_output_dispatch,
which builds a list of sw_if_index to next index mappings.

When interfaces are deleted and the new interfaces are created,
these mappings become stale, and cause the packets being sent to wrong
interface output nodes.

This patch (thanks John Lo for the brilliant idea!) adds a feature node "output",
whose sole purpose is dispatching the packets to the correct interface output
nodes. To do that, it uses the l2output_main.next_nodes, which is already
taken care of for the case of the sw_if_index reuse, so this makes the dependent
features all work correctly.

Since this changes the packet path, for the features that were always the last ones
it has triggered a side problem of the output feat_next_node_index not being properly
initalized. These two users are l2-output-classify node and the output nodes belonging
to the acl-plugin.

For the first one the less invasive fix is just to initialize that field.
For the acl-plugin nodes, rewrite the affected part of the code to use
feat_bitmap_get_next_node_index since this is essentially what the conditional
in l2_output_dispatch does, and fix the compiler warnings generated.

This fix was first made in stable/1701 under commit e7dcee4027854b0ad076101471afdfff67eb9011.

Change-Id: I32e876ab1e1d498cf0854c19c6318dcf59a93805
Signed-off-by: Andrew Yourtchenko <ayourtch@gmail.com>

10 files changed, 53 insertions, 88 deletions

diff --git a/src/plugins/acl/acl.c b/src/plugins/acl/acl.c
index 85c9113b25b..3fe084b49b9 100644
--- a/src/plugins/acl/acl.c
+++ b/src/plugins/acl/acl.c
@@ -1883,7 +1883,11 @@ acl_setup_nodes (void)
 
   feat_bitmap_init_next_nodes (vm, acl_in_node.index, L2INPUT_N_FEAT,
 			       l2input_get_feat_names (),
-			       am->acl_in_node_input_next_node_index);
+			       am->acl_in_node_feat_next_node_index);
+
+  feat_bitmap_init_next_nodes (vm, acl_out_node.index, L2OUTPUT_N_FEAT,
+			       l2output_get_feat_names (),
+			       am->acl_out_node_feat_next_node_index);
 
   memset (&am->acl_in_ip4_match_next[0], 0,
 	  sizeof (am->acl_in_ip4_match_next));
diff --git a/src/plugins/acl/acl.h b/src/plugins/acl/acl.h
index 62046788660..0252ff388d8 100644
--- a/src/plugins/acl/acl.h
+++ b/src/plugins/acl/acl.h
@@ -122,9 +122,8 @@ typedef struct {
   u32 l2_output_classify_next_acl;
 
   /* next node indices for feature bitmap */
-  u32 acl_in_node_input_next_node_index[32];
-  /* the respective thing for the output feature */
-  l2_output_next_nodes_st acl_out_output_next_nodes;
+  u32 acl_in_node_feat_next_node_index[32];
+  u32 acl_out_node_feat_next_node_index[32];
 
   /* ACL match actions (must be coherent across in/out ACLs to next indices (can differ) */
 
diff --git a/src/plugins/acl/l2sess.c b/src/plugins/acl/l2sess.c
index b0385be13df..7a1567fbd1b 100644
--- a/src/plugins/acl/l2sess.c
+++ b/src/plugins/acl/l2sess.c
@@ -31,10 +31,18 @@
 #include <vnet/l2/l2_input.h>
 
 void
-l2sess_init_next_features_input (vlib_main_t * vm, l2sess_main_t * sm)
+l2sess_init_next_features (vlib_main_t * vm, l2sess_main_t * sm)
 {
-#define _(node_name, node_var, is_out, is_ip6, is_track) \
-  if (!is_out) feat_bitmap_init_next_nodes(vm, node_var.index, L2INPUT_N_FEAT,  l2input_get_feat_names (), sm->node_var ## _input_next_node_index);
+#define _(node_name, node_var, is_out, is_ip6, is_track)                 \
+  if (is_out)                                                            \
+    feat_bitmap_init_next_nodes(vm, node_var.index, L2OUTPUT_N_FEAT,      \
+                                l2output_get_feat_names (),               \
+                                sm->node_var ## _feat_next_node_index); \
+  else                                                                   \
+    feat_bitmap_init_next_nodes(vm, node_var.index, L2INPUT_N_FEAT,      \
+                                l2input_get_feat_names (),               \
+                                sm->node_var ## _feat_next_node_index);
+
   foreach_l2sess_node
 #undef _
 }
@@ -62,7 +70,7 @@ l2sess_setup_nodes (void)
   vlib_main_t *vm = vlib_get_main ();
   l2sess_main_t *sm = &l2sess_main;
 
-  l2sess_init_next_features_input (vm, sm);
+  l2sess_init_next_features (vm, sm);
 
   l2sess_add_our_next_nodes (vm, sm, (u8 *) "l2-input-classify", 0);
   l2sess_add_our_next_nodes (vm, sm, (u8 *) "l2-output-classify", 1);
diff --git a/src/plugins/acl/l2sess.h b/src/plugins/acl/l2sess.h
index 888b53011dd..961c08c8749 100644
--- a/src/plugins/acl/l2sess.h
+++ b/src/plugins/acl/l2sess.h
@@ -95,8 +95,7 @@ typedef struct {
      * on whether the node is an input or output one.
      */
 #define _(node_name, node_var, is_out, is_ip6, is_track) \
-    u32 node_var ## _input_next_node_index[32]; \
-    l2_output_next_nodes_st node_var ## _next_nodes;
+    u32 node_var ## _feat_next_node_index[32];
 foreach_l2sess_node
 #undef _
     l2_output_next_nodes_st output_next_nodes;
diff --git a/src/plugins/acl/l2sess_node.c b/src/plugins/acl/l2sess_node.c
index 520e5929b4b..689d216dea1 100644
--- a/src/plugins/acl/l2sess_node.c
+++ b/src/plugins/acl/l2sess_node.c
@@ -526,13 +526,13 @@ check_idle_sessions (l2sess_main_t * sm, u32 sw_if_index, u64 now)
 
 static uword
 l2sess_node_fn (vlib_main_t * vm,
-		vlib_node_runtime_t * node, vlib_frame_t * frame)
+		vlib_node_runtime_t * node, vlib_frame_t * frame,
+                int node_is_out, int node_is_ip6, int node_is_track,
+                u32 *feat_next_node_index)
 {
   u32 n_left_from, *from, *to_next;
   l2sess_next_t next_index;
   u32 pkts_swapped = 0;
-  u32 cached_sw_if_index = (u32) ~ 0;
-  u32 cached_next_index = (u32) ~ 0;
   u32 feature_bitmap0;
   u32 trace_flags0;
 
@@ -570,45 +570,19 @@ l2sess_node_fn (vlib_main_t * vm,
 	  //en0 = vlib_buffer_get_current (b0);
 
 /*
- * The non-boilerplate is in the block below.
- * Note first a magic macro block that sets up the behavior qualifiers:
  *     node_is_out : 1 = is output, 0 = is input
  *     node_is_ip6 : 1 = is ip6, 0 = is ip4
  *     node_is_track : 1 = is a state tracking node, 0 - is a session addition node
  *
- * Subsequently the code adjusts its behavior depending on these variables.
- * It's most probably not great performance wise but much easier to work with.
- *
+ *     The below code adjust the behavior according to these parameters.
  */
 	  {
-	    int node_is_out = -1;
-	    CLIB_UNUSED (int node_is_ip6) = -1;
-	    CLIB_UNUSED (int node_is_track) = -1;
-	    u32 node_index = 0;
 	    u32 session_tables[2] = { ~0, ~0 };
 	    u32 session_nexts[2] = { ~0, ~0 };
-	    l2_output_next_nodes_st *next_nodes = 0;
-	    u32 *input_feat_next_node_index;
 	    u8 l4_proto;
 	    u64 now = clib_cpu_time_now ();
 
-/* 
- * Set the variables according to which of the 8 nodes we are.
- * Hopefully the compiler is smart enough to eliminate the extraneous.
- */
-#define _(node_name, node_var, is_out, is_ip6, is_track)                 \
-if(node_var.index == node->node_index)                                   \
-  {                                                                      \
-    node_is_out = is_out;                                                \
-    node_is_ip6 = is_ip6;                                                \
-    node_is_track = is_track;                                            \
-    node_index = node_var.index;                                         \
-    next_nodes = &sm->node_var ## _next_nodes;                           \
-    input_feat_next_node_index = sm->node_var ## _input_next_node_index; \
-  }
-	    foreach_l2sess_node
-#undef _
-	      trace_flags0 = 0;
+	    trace_flags0 = 0;
 	    if (node_is_out)
 	      {
 		sw_if_index0 = vnet_buffer (b0)->sw_if_index[VLIB_TX];
@@ -715,38 +689,8 @@ if(node_var.index == node->node_index)                                   \
 		check_idle_sessions (sm, sw_if_index0, now);
 	      }
 
-	    if (node_is_out)
-	      {
-		if (feature_bitmap0)
-		  {
-		    trace_flags0 |= 0x10;
-		  }
-		if (sw_if_index0 == cached_sw_if_index)
-		  {
-		    trace_flags0 |= 0x20;
-		  }
-		l2_output_dispatch (sm->vlib_main,
-				    sm->vnet_main,
-				    node,
-				    node_index,
-				    &cached_sw_if_index,
-				    &cached_next_index,
-				    next_nodes,
-				    b0, sw_if_index0, feature_bitmap0,
-				    &next0);
-		trace_flags0 |= 2;
-
-	      }
-	    else
-	      {
-		next0 =
-		  feat_bitmap_get_next_node_index (input_feat_next_node_index,
+	    next0 = feat_bitmap_get_next_node_index (feat_next_node_index,
 						   feature_bitmap0);
-		trace_flags0 |= 4;
-
-	      }
-
-
 
 	    if (next0 >= node->n_next_nodes)
 	      {
@@ -795,7 +739,10 @@ node_var ## node_fn (vlib_main_t * vm,                   \
                   vlib_node_runtime_t * node,            \
                   vlib_frame_t * frame)                  \
 {                                                        \
-  return l2sess_node_fn(vm, node, frame);                \
+  l2sess_main_t *sm = &l2sess_main;                      \
+  return l2sess_node_fn(vm, node, frame,                 \
+                        is_out, is_ip6, is_track,        \
+                        sm->node_var ## _feat_next_node_index);  \
 }                                                        \
 VLIB_REGISTER_NODE (node_var) = {                        \
   .function = node_var ## node_fn,                       \
diff --git a/src/plugins/acl/node_in.c b/src/plugins/acl/node_in.c
index 2a5199a9ab8..95802df5fbc 100644
--- a/src/plugins/acl/node_in.c
+++ b/src/plugins/acl/node_in.c
@@ -73,7 +73,7 @@ acl_in_node_fn (vlib_main_t * vm,
   u32 feature_bitmap0;
   u32 trace_bitmap = 0;
   u32 *input_feat_next_node_index =
-    acl_main.acl_in_node_input_next_node_index;
+    acl_main.acl_in_node_feat_next_node_index;
 
   from = vlib_frame_vector_args (frame);
   n_left_from = frame->n_vectors;
diff --git a/src/plugins/acl/node_out.c b/src/plugins/acl/node_out.c
index 50af3679b6e..cbec3b9a89d 100644
--- a/src/plugins/acl/node_out.c
+++ b/src/plugins/acl/node_out.c
@@ -68,13 +68,12 @@ acl_out_node_fn (vlib_main_t * vm,
 		 vlib_node_runtime_t * node, vlib_frame_t * frame)
 {
   acl_main_t *am = &acl_main;
-  l2_output_next_nodes_st *next_nodes = &am->acl_out_output_next_nodes;
+  u32 *output_feat_next_node_index =
+    am->acl_out_node_feat_next_node_index;
   u32 n_left_from, *from, *to_next;
   acl_out_next_t next_index;
   u32 pkts_acl_checked = 0;
   u32 feature_bitmap0;
-  u32 cached_sw_if_index = (u32) ~ 0;
-  u32 cached_next_index = (u32) ~ 0;
   u32 match_acl_index = ~0;
   u32 match_rule_index = ~0;
   u32 trace_bitmap = 0;
@@ -119,14 +118,9 @@ acl_out_node_fn (vlib_main_t * vm,
 	    }
 	  if (next0 == ~0)
 	    {
-	      l2_output_dispatch (vm,
-				  am->vnet_main,
-				  node,
-				  acl_out_node.index,
-				  &cached_sw_if_index,
-				  &cached_next_index,
-				  next_nodes,
-				  b0, sw_if_index0, feature_bitmap0, &next0);
+              next0 =
+                feat_bitmap_get_next_node_index (output_feat_next_node_index,
+                                                 feature_bitmap0);
 	    }
 
 
diff --git a/src/vnet/l2/l2_input.c b/src/vnet/l2/l2_input.c
index fbd75f222e0..3aa5331bf7a 100644
--- a/src/vnet/l2/l2_input.c
+++ b/src/vnet/l2/l2_input.c
@@ -703,10 +703,11 @@ set_int_l2_mode (vlib_main_t * vm, vnet_main_t * vnet_main,	/*           */
 	  shg = 0;		/* not used in xconnect */
 	}
 
-      /* set up split-horizon group */
+      /* set up split-horizon group and set output feature bit */
       config->shg = shg;
       out_config = l2output_intf_config (sw_if_index);
       out_config->shg = shg;
+      out_config->feature_bitmap |= L2OUTPUT_FEAT_OUTPUT;
 
       /*
        * Test: remove this when non-IP features can be configured.
diff --git a/src/vnet/l2/l2_output.h b/src/vnet/l2/l2_output.h
index c683b1ade73..9597205caed 100644
--- a/src/vnet/l2/l2_output.h
+++ b/src/vnet/l2/l2_output.h
@@ -94,6 +94,7 @@ l2output_main_t l2output_main;
 
 /* Mappings from feature ID to graph node name */
 #define foreach_l2output_feat \
+ _(OUTPUT,            "interface-output")           \
  _(SPAN,              "feature-bitmap-drop")        \
  _(CFM,               "feature-bitmap-drop")        \
  _(QOS,               "feature-bitmap-drop")        \
@@ -217,9 +218,21 @@ l2_output_dispatch (vlib_main_t * vlib_main,
 		    vlib_buffer_t * b0,
 		    u32 sw_if_index, u32 feature_bitmap, u32 * next0)
 {
-  if (feature_bitmap)
+  /*
+   * The output feature bitmap always have at least the output feature bit set
+   * for a normal L2 interface (or all 0's if the interface is changed from L2
+   * to L3 mode). So if next_nodes specified is that from the l2-output node and
+   * the bitmap is all clear except output feature bit, we know there is no more
+   * feature and will fall through to output packet. If next_nodes is from a L2
+   * output feature node (and not l2-output), we always want to get the node for
+   * the next L2 output feature, including the last feature being interface-
+   * output node to output packet.
+   */
+  if ((next_nodes != &l2output_main.next_nodes)
+      || ((feature_bitmap & ~L2OUTPUT_FEAT_OUTPUT) != 0))
     {
       /* There are some features to execute */
+      ASSERT (feature_bitmap != 0);
 
       /* Save bitmap for the next feature graph nodes */
       vnet_buffer (b0)->l2.feature_bitmap = feature_bitmap;
diff --git a/src/vnet/l2/l2_output_classify.c b/src/vnet/l2/l2_output_classify.c
index 27d5eb39514..832be1a11c0 100644
--- a/src/vnet/l2/l2_output_classify.c
+++ b/src/vnet/l2/l2_output_classify.c
@@ -493,7 +493,7 @@ l2_output_classify_init (vlib_main_t * vm)
 			       l2_output_classify_node.index,
 			       L2OUTPUT_N_FEAT,
 			       l2output_get_feat_names (),
-			       cm->feat_next_node_index);
+			       cm->next_nodes.feat_next_node_index);
   rt->l2cm = cm;
   rt->vcm = cm->vnet_classify_main;