Re: [PATCH v2.42 1/5] odp: Allow VLAN actions after MPLS actions

@@ -172,6 +172,37 @@ struct xlate_ctx {
     odp_port_t sflow_odp_port;  /* Output port for composing sFlow action. */
     uint16_t user_cookie_offset;/* Used for user_action_cookie fixup. */
     bool exit;                  /* No further actions should be processed. */
+
+    /* The final vlan_tci state.
+     *
+     * The value of the vlan TCI prior to the committing of ODP MPLS
+     * actions should be stored in 'xin->flow->vlan_tci'.
+     *
+     * The final value of the VLAN TCI should be stored in 'vlan_tci'. And
+     * is if the value of 'vlan_tci' and 'xin->flow->vlan_tci' differ then
+     * VLAN ODP actions will be committed after any MPLS actions regardless
+     * of whether VLAN actions were also committed before the MPLS actions or
+     * not.
+     *
+     * This mechanism allows a VLAN tag to be popped before pushing
+     * an MPLS LSE and then the same VLAN tag pushed after pushing
+     * the MPLS LSE. In this way it is possible to push an MPLS LSE
+     * before an existing VLAN tag. Moreover this mechanism allows
+     * the order in which VLAN tags and MPLS LSEs are pushed. */
+    ovs_be16 final_vlan_tci;
+
+    /* The next vlan_tci state.
+     *
+     * This value this variable points to updated each time an
+     * action updates the VLAN tci.
+     *
+     * This variable initially points to 'xin->flow->vlan_tci' so that ODP
+     * VLAN actions are committed before any MPLS actions. When an MPLS
+     * action is composed 'next_vlan_tci' is updated to point to
+     * 'final_vlan_tci'. This causes subsequent VLAN actions to be
+     * committed after MPLS actions. */
+    ovs_be16 *next_vlan_tci;
+
 };
 
 /* A controller may use OFPP_NONE as the ingress port to indicate that

@@ -982,7 +1013,7 @@ static void
 output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle,
               uint16_t vlan)
 {
-    ovs_be16 *flow_tci = &ctx->xin->vlan_tci;
+    ovs_be16 *flow_tci = &ctx->final_vlan_tci;
     uint16_t vid;
     ovs_be16 tci, old_tci;
     struct xport *xport;

@@ -1258,7 +1289,7 @@ xlate_normal(struct xlate_ctx *ctx)
 
     /* Drop malformed frames. */
     if (flow->dl_type == htons(ETH_TYPE_VLAN) &&
-        !(ctx->xin->vlan_tci & htons(VLAN_CFI))) {
+        !(ctx->final_vlan_tci & htons(VLAN_CFI))) {
         if (ctx->xin->packet != NULL) {
             static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
             VLOG_WARN_RL(&rl, "bridge %s: dropping packet with partial "

@@ -1282,7 +1313,7 @@ xlate_normal(struct xlate_ctx *ctx)
     }
 
     /* Check VLAN. */
-    vid = vlan_tci_to_vid(ctx->xin->vlan_tci);
+    vid = vlan_tci_to_vid(ctx->final_vlan_tci);
     if (!input_vid_is_valid(vid, in_xbundle, ctx->xin->packet != NULL)) {
         xlate_report(ctx, "disallowed VLAN VID for this input port, dropping");
         return;

@@ -1540,7 +1571,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
     const struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port);
     struct flow_wildcards *wc = &ctx->xout->wc;
     struct flow *flow = &ctx->xin->flow;
-    ovs_be16 flow_vlan_tci, xin_vlan_tci;
+    ovs_be16 flow_vlan_tci, vlan_tci;
     uint32_t flow_pkt_mark;
     uint8_t flow_nw_tos;
     odp_port_t out_port, odp_port;

@@ -1609,7 +1640,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
     }
 
     flow_vlan_tci = flow->vlan_tci;
-    xin_vlan_tci = ctx->xin->vlan_tci;
+    vlan_tci = ctx->final_vlan_tci;
     flow_pkt_mark = flow->pkt_mark;
     flow_nw_tos = flow->nw_tos;
 

@@ -1649,20 +1680,20 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
             wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
         }
         vlandev_port = vsp_realdev_to_vlandev(ctx->xbridge->ofproto, ofp_port,
-                                              ctx->xin->vlan_tci);
+                                              ctx->final_vlan_tci);
         if (vlandev_port == ofp_port) {
             out_port = odp_port;
         } else {
             out_port = ofp_port_to_odp_port(ctx->xbridge, vlandev_port);
             flow->vlan_tci = htons(0);
-            ctx->xin->vlan_tci = htons(0);
+            ctx->final_vlan_tci = htons(0);
         }
     }
 
     if (out_port != ODPP_NONE) {
         commit_odp_actions(flow, &ctx->base_flow,
                            &ctx->xout->odp_actions, &ctx->xout->wc,
-                           &ctx->mpls_depth_delta, ctx->xin->vlan_tci);
+                           &ctx->mpls_depth_delta, ctx->final_vlan_tci);
         nl_msg_put_odp_port(&ctx->xout->odp_actions, OVS_ACTION_ATTR_OUTPUT,
                             out_port);
 

@@ -1674,7 +1705,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
  out:
     /* Restore flow */
     flow->vlan_tci = flow_vlan_tci;
-    ctx->xin->vlan_tci = xin_vlan_tci;
+    ctx->final_vlan_tci = vlan_tci;
     flow->pkt_mark = flow_pkt_mark;
     flow->nw_tos = flow_nw_tos;
 }

@@ -1819,7 +1850,7 @@ execute_controller_action(struct xlate_ctx *ctx, int len,
 
     commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
                        &ctx->xout->odp_actions, &ctx->xout->wc,
-                       &ctx->mpls_depth_delta, ctx->xin->vlan_tci);
+                       &ctx->mpls_depth_delta, ctx->final_vlan_tci);
 
     odp_execute_actions(NULL, packet, &key, ctx->xout->odp_actions.data,
                         ctx->xout->odp_actions.size, NULL, NULL);

@@ -2207,7 +2238,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
 
   commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
                      &ctx->xout->odp_actions, &ctx->xout->wc,
-                     &ctx->mpls_depth_delta, ctx->xin->vlan_tci);
+                     &ctx->mpls_depth_delta, ctx->final_vlan_tci);
 
   compose_flow_sample_cookie(os->probability, os->collector_set_id,
                              os->obs_domain_id, os->obs_point_id, &cookie);

@@ -2236,13 +2267,14 @@ may_receive(const struct xport *xport, struct xlate_ctx *ctx)
 }
 
 static void
-vlan_tci_restore(struct xlate_in *xin, ovs_be16 *tci_ptr, ovs_be16 orig_tci)
+vlan_tci_restore(struct xlate_ctx *ctx, ovs_be16 orig_tci)
 {
     /* If MPLS actions were executed after vlan actions then
      * copy the final vlan_tci out and restore the intermediate VLAN state. */
-    if (xin->flow.vlan_tci != orig_tci && tci_ptr == &xin->vlan_tci) {
-        xin->vlan_tci = xin->flow.vlan_tci;
-        xin->flow.vlan_tci = orig_tci;
+    if (ctx->xin->flow.vlan_tci != orig_tci &&
+        ctx->next_vlan_tci == &ctx->final_vlan_tci) {
+        ctx->final_vlan_tci = ctx->xin->flow.vlan_tci;
+        ctx->xin->flow.vlan_tci = orig_tci;
     }
 }
 

@@ -2252,19 +2284,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 {
     struct flow_wildcards *wc = &ctx->xout->wc;
     struct flow *flow = &ctx->xin->flow;
-    ovs_be16 *vlan_tci;
     const struct ofpact *a;
 
-
-    /* VLAN actions are stored in '*vlan_tci'. This variable initially
-     * points to 'xin->flow->vlan_tci', so that VLAN actions are applied
-     * before any MPLS actions. When an MPLS action is translated,
-     * 'vlan_tci' is updated to point to 'xin->vlan_tci'. This causes later
-     * VLAN actions to be applied after MPLS actions.  For each iteration
-     * of the loop 'xin->vlan_tci' is updated to reflect the final VLAN
-     * state of the flow. */
-    vlan_tci = &ctx->xin->flow.vlan_tci;
-
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
         struct ofpact_controller *controller;
         const struct ofpact_metadata *metadata;

@@ -2274,7 +2295,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         }
 
         /* Update the final vlan state to match the current state. */
-        ctx->xin->vlan_tci = *vlan_tci;
+        ctx->final_vlan_tci = *ctx->next_vlan_tci;
 
         switch (a->type) {
         case OFPACT_OUTPUT:

@@ -2299,28 +2320,28 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
         case OFPACT_SET_VLAN_VID:
             wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
-            *vlan_tci &= ~htons(VLAN_VID_MASK);
-            *vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
+            *ctx->next_vlan_tci &= ~htons(VLAN_VID_MASK);
+            *ctx->next_vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
                           | htons(VLAN_CFI));
             break;
 
         case OFPACT_SET_VLAN_PCP:
             wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
-            *vlan_tci &= ~htons(VLAN_PCP_MASK);
-            *vlan_tci |=
+            *ctx->next_vlan_tci &= ~htons(VLAN_PCP_MASK);
+            *ctx->next_vlan_tci |=
                 htons((ofpact_get_SET_VLAN_PCP(a)->vlan_pcp << VLAN_PCP_SHIFT)
                       | VLAN_CFI);
             break;
 
         case OFPACT_STRIP_VLAN:
             memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
-            *vlan_tci = htons(0);
+            *ctx->next_vlan_tci = htons(0);
             break;
 
         case OFPACT_PUSH_VLAN:
             /* XXX 802.1AD(QinQ) */
             memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
-            *vlan_tci = htons(VLAN_CFI);
+            *ctx->next_vlan_tci = htons(VLAN_CFI);
             break;
 
         case OFPACT_SET_ETH_SRC:

@@ -2391,20 +2412,20 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         case OFPACT_REG_MOVE: {
             ovs_be16 orig_tci = flow->vlan_tci;
             nxm_execute_reg_move(ofpact_get_REG_MOVE(a), flow, wc);
-            vlan_tci_restore(ctx->xin, vlan_tci, orig_tci);
+            vlan_tci_restore(ctx, orig_tci);
             break;
         }
 
         case OFPACT_REG_LOAD: {
             ovs_be16 orig_tci = flow->vlan_tci;
             nxm_execute_reg_load(ofpact_get_REG_LOAD(a), flow);
-            vlan_tci_restore(ctx->xin, vlan_tci, orig_tci);
+            vlan_tci_restore(ctx, orig_tci);
             break;
         }
 
         case OFPACT_STACK_PUSH: {
             ovs_be16 orig_tci = flow->vlan_tci;
-            flow->vlan_tci = *vlan_tci;
+            flow->vlan_tci = *ctx->next_vlan_tci;
             nxm_execute_stack_push(ofpact_get_STACK_PUSH(a), flow, wc,
                                    &ctx->stack);
             flow->vlan_tci = orig_tci;

@@ -2415,7 +2436,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             ovs_be16 orig_tci = flow->vlan_tci;
             nxm_execute_stack_pop(ofpact_get_STACK_POP(a), flow, wc,
                                   &ctx->stack);
-            vlan_tci_restore(ctx->xin, vlan_tci, orig_tci);
+            vlan_tci_restore(ctx, orig_tci);
             break;
         }
 

@@ -2433,11 +2454,11 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
              * Do not save and therefore pop the VLAN tags if the MPLS LSE
              * should be pushed before any VLAN tags that are present.
              * This is the behaviour described for OpenFlow 1.3. */
-            ctx->xin->vlan_tci = *vlan_tci;
+            ctx->final_vlan_tci = *ctx->next_vlan_tci;
             if (!oam->mpls_before_vlan) {
                 flow->vlan_tci = htons(0);
             }
-            vlan_tci = &ctx->xin->vlan_tci;
+            ctx->next_vlan_tci = &ctx->final_vlan_tci;
             break;
         }
 

@@ -2540,7 +2561,6 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto,
 {
     xin->ofproto = ofproto;
     xin->flow = *flow;
-    xin->vlan_tci = flow->vlan_tci;
     xin->packet = packet;
     xin->may_learn = packet != NULL;
     xin->rule = rule;

@@ -2770,6 +2790,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     ctx.table_id = 0;
     ctx.exit = false;
     ctx.mpls_depth_delta = 0;
+    ctx.final_vlan_tci = ctx->xin->flow.vlan_tci;
+    ctx.next_vlan_tci = &ctx->xin->flow.vlan_tci;
 
     if (xin->ofpacts) {
         ofpacts = xin->ofpacts;

@@ -60,11 +60,6 @@ struct xlate_in {
      * this flow when actions change header fields. */
     struct flow flow;
 
-    /* If MPLS and VLAN actions were both present in the translation, and VLAN
-     * actions should occur after the MPLS actions, then this field is used
-     * to store the final vlan_tci state. */
-    ovs_be16 vlan_tci;
-
     /* The packet corresponding to 'flow', or a null pointer if we are
      * revalidating without a packet to refer to. */
     const struct ofpbuf *packet;