[ovs-dev] [PATCH v2] ofproto-dpif-xlate: Fix packet_in reason for Table-miss rule

Keshav Gupta keshav.gupta at ericsson.com
Wed Jul 4 15:10:53 UTC 2018


Currently in OvS if we hit "Table-miss" rules (associated with Controller
action) then we send PACKET_IN message to controller with reason as
OFPR_NO_MATCH.

“Table-miss” rule is one whose priority is 0 and its catch all rule.

But if we hit same "Table-miss" rule after executing group entry we will
send the reason as OFPR_ACTION (for OF1.3 and below) and OFPR_GROUP
(for OF1.4 and above).

This is because once we execute group entry we set ctx->in_group and later
when we hit the "Table-miss" rule, Since ctx->in_group  is set we send
reason as OFPR_ACTION (for OF1.3) and OFPR_GROUP (for OF1.4 and above).

For eg: for the following pipeline, we will send the reason as OFPR_ACTION
even if we hit The “Table-miss” rule.

cookie=0x8000000, duration=761.189s, table=0, n_packets=1401, n_bytes=67954,
  priority=4,in_port=9,vlan_tci=0x0000/0x1fff
  actions=write_metadata:0x67870000000000/0xffffff0000000001,goto_table:17

cookie=0x6800001, duration=768.848s, table=17, n_packets=1418, n_bytes=68776,
  priority=10,metadata=0x67870000000000/0xffffff0000000000
  actions=write_metadata:0xe067870000000000/0xfffffffffffffffe,goto_table:60

cookie=0x6800000, duration=24944.312s, table=60, n_packets=58244,
  n_bytes=2519520, priority=0 actions=resubmit(,17)

cookie=0x8040000, duration=785.733s, table=17, n_packets=1450, n_bytes=69724,
  priority=10,metadata=0xe067870000000000/0xffffff0000000000
  actions=write_metadata:0x67871d4d000000/0xfffffffffffffffe,goto_table:43

cookie=0x822002d, duration=24960.795s, table=43, n_packets=53097,
  n_bytes=2230074, priority=100,arp,arp_op=1 actions=group:6000

group_id=6000,type=all,bucket=actions=CONTROLLER:65535,
  bucket=actions=resubmit(,48), bucket=actions=resubmit(,81)

cookie=0x8500000, duration=24977.323s, table=48, n_packets=58309, n_bytes=2522634,
  priority=0 actions=resubmit(,49),resubmit(,50)

cookie=0x8050000, duration=24984.679s, table=50, n_packets=6, n_bytes=264,
  priority=0 actions=CONTROLLER:65535

Currently we are sending table_id as 50 and packet_in reason as OFPR_ACTION.
Instead of sending packet_in reason as OFPR_NO_MATCH.

Signed-off-by: Keshav Gupta <keshav.gupta at ericsson.com>
Co-authored-by: Rohith Basavaraja <rohith.basavaraja at gmail.com>
Signed-off-by: Rohith Basavaraja <rohith.basavaraja at gmail.com>
---
 ofproto/ofproto-dpif-xlate.c | 82 +++++++++++++++++++++++++-------------------
 tests/ofproto-dpif.at        | 38 ++++++++++++++++++++
 2 files changed, 84 insertions(+), 36 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index c02a032..0215793 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -249,7 +249,6 @@ struct xlate_ctx {
      */
     int depth;                  /* Current resubmit nesting depth. */
     int resubmits;              /* Total number of resubmits. */
-    bool in_group;              /* Currently translating ofgroup, if true. */
     bool in_action_set;         /* Currently translating action_set, if true. */
     bool in_packet_out;         /* Currently translating a packet_out msg, if
                                  * true. */
@@ -528,12 +527,12 @@ static OVSRCU_TYPE(struct xlate_cfg *) xcfgp = OVSRCU_INITIALIZER(NULL);
 static struct xlate_cfg *new_xcfg = NULL;
 
 typedef void xlate_actions_handler(const struct ofpact *, size_t ofpacts_len,
-                                   struct xlate_ctx *, bool);
+                                   struct xlate_ctx *, bool, bool);
 static bool may_receive(const struct xport *, struct xlate_ctx *);
 static void do_xlate_actions(const struct ofpact *, size_t ofpacts_len,
-                             struct xlate_ctx *, bool);
+                             struct xlate_ctx *, bool, bool);
 static void clone_xlate_actions(const struct ofpact *, size_t ofpacts_len,
-                                struct xlate_ctx *, bool);
+                                struct xlate_ctx *, bool, bool);
 static void xlate_normal(struct xlate_ctx *);
 static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port,
                                uint8_t table_id, bool may_packet_in,
@@ -4104,7 +4103,7 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule,
     ctx->rule_cookie = rule->up.flow_cookie;
     actions = rule_get_actions(&rule->up);
     actions_xlator(actions->ofpacts, actions->ofpacts_len, ctx,
-                   is_last_action);
+                   is_last_action, false);
     ctx->rule_cookie = old_cookie;
     ctx->rule = old_rule;
     ctx->depth -= deepens;
@@ -4279,7 +4278,8 @@ xlate_group_bucket(struct xlate_ctx *ctx, struct ofputil_bucket *bucket,
 
     ofpacts_execute_action_set(&action_list, &action_set);
     ctx->depth++;
-    do_xlate_actions(action_list.data, action_list.size, ctx, is_last_action);
+    do_xlate_actions(action_list.data, action_list.size, ctx, is_last_action,
+                     true);
     ctx->depth--;
 
     ofpbuf_uninit(&action_list);
@@ -4459,9 +4459,6 @@ static void
 xlate_group_action__(struct xlate_ctx *ctx, struct group_dpif *group,
                      bool is_last_action)
 {
-    bool was_in_group = ctx->in_group;
-    ctx->in_group = true;
-
     if (group->up.type == OFPGT11_ALL || group->up.type == OFPGT11_INDIRECT) {
         struct ovs_list *last_bucket = ovs_list_back(&group->up.buckets);
         struct ofputil_bucket *bucket;
@@ -4492,8 +4489,6 @@ xlate_group_action__(struct xlate_ctx *ctx, struct group_dpif *group,
             }
         }
     }
-
-    ctx->in_group = was_in_group;
 }
 
 static bool
@@ -4958,7 +4953,8 @@ compose_dec_mpls_ttl_action(struct xlate_ctx *ctx)
 static void
 xlate_output_action(struct xlate_ctx *ctx, ofp_port_t port,
                     uint16_t controller_len, bool may_packet_in,
-                    bool is_last_action, bool truncate)
+                    bool is_last_action, bool truncate,
+                    bool group_bucket_action)
 {
     ofp_port_t prev_nf_output_iface = ctx->nf_output_iface;
 
@@ -4986,7 +4982,7 @@ xlate_output_action(struct xlate_ctx *ctx, ofp_port_t port,
     case OFPP_CONTROLLER:
         xlate_controller_action(ctx, controller_len,
                                 (ctx->in_packet_out ? OFPR_PACKET_OUT
-                                 : ctx->in_group ? OFPR_GROUP
+                                 : group_bucket_action ? OFPR_GROUP
                                  : ctx->in_action_set ? OFPR_ACTION_SET
                                  : OFPR_ACTION),
                                 0, NULL, 0);
@@ -5016,7 +5012,8 @@ xlate_output_action(struct xlate_ctx *ctx, ofp_port_t port,
 static void
 xlate_output_reg_action(struct xlate_ctx *ctx,
                         const struct ofpact_output_reg *or,
-                        bool is_last_action)
+                        bool is_last_action,
+                        bool group_bucket_action)
 {
     uint64_t port = mf_get_subfield(&or->src, &ctx->xin->flow);
     if (port <= UINT16_MAX) {
@@ -5027,7 +5024,8 @@ xlate_output_reg_action(struct xlate_ctx *ctx,
         memset(&value, 0xff, sizeof value);
         mf_write_subfield_flow(&or->src, &value, &ctx->wc->masks);
         xlate_output_action(ctx, u16_to_ofp(port), or->max_len,
-                            false, is_last_action, false);
+                            false, is_last_action, false,
+                            group_bucket_action);
     } else {
         xlate_report(ctx, OFT_WARN, "output port %"PRIu64" is out of range",
                      port);
@@ -5037,7 +5035,8 @@ xlate_output_reg_action(struct xlate_ctx *ctx,
 static void
 xlate_output_trunc_action(struct xlate_ctx *ctx,
                     ofp_port_t port, uint32_t max_len,
-                    bool is_last_action)
+                    bool is_last_action,
+                    bool group_bucket_action)
 {
     bool support_trunc = ctx->xbridge->support.trunc;
     struct ovs_action_trunc *trunc;
@@ -5074,7 +5073,8 @@ xlate_output_trunc_action(struct xlate_ctx *ctx,
                                 OVS_ACTION_ATTR_TRUNC,
                                 sizeof *trunc);
             trunc->max_len = max_len;
-            xlate_output_action(ctx, port, 0, false, is_last_action, true);
+            xlate_output_action(ctx, port, 0, false, is_last_action, true,
+                                group_bucket_action);
             if (!support_trunc) {
                 ctx->xout->slow |= SLOW_ACTION;
             }
@@ -5088,7 +5088,8 @@ xlate_output_trunc_action(struct xlate_ctx *ctx,
 static void
 xlate_enqueue_action(struct xlate_ctx *ctx,
                      const struct ofpact_enqueue *enqueue,
-                     bool is_last_action)
+                     bool is_last_action,
+                     bool group_bucket_action)
 {
     ofp_port_t ofp_port = enqueue->port;
     uint32_t queue_id = enqueue->queue;
@@ -5100,7 +5101,8 @@ xlate_enqueue_action(struct xlate_ctx *ctx,
     if (error) {
         /* Fall back to ordinary output action. */
         xlate_output_action(ctx, enqueue->port, 0, false,
-                            is_last_action, false);
+                            is_last_action, false,
+                            group_bucket_action);
         return;
     }
 
@@ -5163,7 +5165,8 @@ slave_enabled_cb(ofp_port_t ofp_port, void *xbridge_)
 static void
 xlate_bundle_action(struct xlate_ctx *ctx,
                     const struct ofpact_bundle *bundle,
-                    bool is_last_action)
+                    bool is_last_action,
+                    bool group_bucket_action)
 {
     ofp_port_t port;
 
@@ -5173,7 +5176,8 @@ xlate_bundle_action(struct xlate_ctx *ctx,
         nxm_reg_load(&bundle->dst, ofp_to_u16(port), &ctx->xin->flow, ctx->wc);
         xlate_report_subfield(ctx, &bundle->dst);
     } else {
-        xlate_output_action(ctx, port, 0, false, is_last_action, false);
+        xlate_output_action(ctx, port, 0, false, is_last_action, false,
+                            group_bucket_action);
     }
 }
 
@@ -5472,7 +5476,8 @@ reversible_actions(const struct ofpact *ofpacts, size_t ofpacts_len)
 
 static void
 clone_xlate_actions(const struct ofpact *actions, size_t actions_len,
-                    struct xlate_ctx *ctx, bool is_last_action)
+                    struct xlate_ctx *ctx, bool is_last_action,
+                    bool group_bucket_action OVS_UNUSED)
 {
     struct ofpbuf old_stack = ctx->stack;
     union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
@@ -5489,7 +5494,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len,
 
     if (reversible_actions(actions, actions_len) || is_last_action) {
         old_flow = ctx->xin->flow;
-        do_xlate_actions(actions, actions_len, ctx, is_last_action);
+        do_xlate_actions(actions, actions_len, ctx, is_last_action, false);
         if (!ctx->freezing) {
             xlate_action_set(ctx);
         }
@@ -5513,7 +5518,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len,
     if (ctx->xbridge->support.clone) { /* Use clone action */
         /* Use clone action as datapath clone. */
         offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
-        do_xlate_actions(actions, actions_len, ctx, true);
+        do_xlate_actions(actions, actions_len, ctx, true, false);
         if (!ctx->freezing) {
             xlate_action_set(ctx);
         }
@@ -5529,7 +5534,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len,
         offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE);
         ac_offset = nl_msg_start_nested(ctx->odp_actions,
                                         OVS_SAMPLE_ATTR_ACTIONS);
-        do_xlate_actions(actions, actions_len, ctx, true);
+        do_xlate_actions(actions, actions_len, ctx, true, false);
         if (!ctx->freezing) {
             xlate_action_set(ctx);
         }
@@ -5576,7 +5581,8 @@ compose_clone(struct xlate_ctx *ctx, const struct ofpact_nest *oc,
 {
     size_t oc_actions_len = ofpact_nest_get_action_len(oc);
 
-    clone_xlate_actions(oc->actions, oc_actions_len, ctx, is_last_action);
+    clone_xlate_actions(oc->actions, oc_actions_len, ctx, is_last_action,
+                        false);
 }
 
 static void
@@ -5658,7 +5664,7 @@ xlate_action_set(struct xlate_ctx *ctx)
         struct ovs_list *old_trace = ctx->xin->trace;
         ctx->xin->trace = xlate_report(ctx, OFT_TABLE,
                                        "--. Executing action set:");
-        do_xlate_actions(action_list.data, action_list.size, ctx, true);
+        do_xlate_actions(action_list.data, action_list.size, ctx, true, false);
         ctx->xin->trace = old_trace;
 
         ctx->in_action_set = false;
@@ -5901,7 +5907,7 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc,
     ctx->wc->masks.ct_mark = 0;
     ctx->wc->masks.ct_label = OVS_U128_ZERO;
     do_xlate_actions(ofc->actions, ofpact_ct_get_action_len(ofc), ctx,
-                     is_last_action);
+                     is_last_action, false);
 
     if (ofc->zone_src.field) {
         zone = mf_get_subfield(&ofc->zone_src, &ctx->xin->flow);
@@ -6313,7 +6319,8 @@ xlate_ofpact_unroll_xlate(struct xlate_ctx *ctx,
 
 static void
 do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
-                 struct xlate_ctx *ctx, bool is_last_action)
+                 struct xlate_ctx *ctx, bool is_last_action,
+                 bool group_bucket_action)
 {
     struct flow_wildcards *wc = ctx->wc;
     struct flow *flow = &ctx->xin->flow;
@@ -6362,7 +6369,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         case OFPACT_OUTPUT:
             xlate_output_action(ctx, ofpact_get_OUTPUT(a)->port,
                                 ofpact_get_OUTPUT(a)->max_len, true, last,
-                                false);
+                                false, group_bucket_action);
             break;
 
         case OFPACT_GROUP:
@@ -6393,7 +6400,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         case OFPACT_ENQUEUE:
             memset(&wc->masks.skb_priority, 0xff,
                    sizeof wc->masks.skb_priority);
-            xlate_enqueue_action(ctx, ofpact_get_ENQUEUE(a), last);
+            xlate_enqueue_action(ctx, ofpact_get_ENQUEUE(a), last,
+                                 group_bucket_action);
             break;
 
         case OFPACT_SET_VLAN_VID:
@@ -6609,16 +6617,19 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_BUNDLE:
-            xlate_bundle_action(ctx, ofpact_get_BUNDLE(a), last);
+            xlate_bundle_action(ctx, ofpact_get_BUNDLE(a), last,
+                                group_bucket_action);
             break;
 
         case OFPACT_OUTPUT_REG:
-            xlate_output_reg_action(ctx, ofpact_get_OUTPUT_REG(a), last);
+            xlate_output_reg_action(ctx, ofpact_get_OUTPUT_REG(a), last,
+                    group_bucket_action);
             break;
 
         case OFPACT_OUTPUT_TRUNC:
             xlate_output_trunc_action(ctx, ofpact_get_OUTPUT_TRUNC(a)->port,
-                                ofpact_get_OUTPUT_TRUNC(a)->max_len, last);
+                                ofpact_get_OUTPUT_TRUNC(a)->max_len, last,
+                                group_bucket_action);
             break;
 
         case OFPACT_LEARN:
@@ -7036,7 +7047,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
 
         .depth = xin->depth,
         .resubmits = xin->resubmits,
-        .in_group = false,
         .in_action_set = false,
         .in_packet_out = xin->in_packet_out,
         .pending_encap = false,
@@ -7285,7 +7295,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
             }
 
             mirror_ingress_packet(&ctx);
-            do_xlate_actions(ofpacts, ofpacts_len, &ctx, true);
+            do_xlate_actions(ofpacts, ofpacts_len, &ctx, true, false);
             if (ctx.error) {
                 goto exit;
             }
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index d798925..caa9b43 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -3421,6 +3421,44 @@ OFPST_FLOW reply (OF1.4):
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([ofproto-dpif - packet-in reasons (Openflow 1.3)])
+
+OVS_VSWITCHD_START([dnl
+    set bridge br0 datapath_type=dummy \
+        protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
+    add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
+
+AT_CHECK([
+    ovs-ofctl -OOpenFlow13 del-flows br0
+    ovs-ofctl -OOpenFlow13 add-group br0 "group_id=6000,type=all,bucket=actions=controller,bucket=actions=resubmit(,48),bucket=actions=resubmit(,81)"
+    ovs-ofctl -OOpenFlow13 add-flow br0 "table=0, in_port=1, vlan_tci=0x0000/0x1fff actions=write_metadata:0x67870000000000/0xffffff0000000001,goto_table:17"
+    ovs-ofctl -OOpenFlow13 add-flow br0 "table=17,  priority=10,metadata=0x67870000000000/0xffffff0000000000 actions=write_metadata:0xe067870000000000/0xfffffffffffffffe,goto_table:60"
+    ovs-ofctl -OOpenFlow13 add-flow br0 "table=60,  priority=0 actions=resubmit(,17)"
+    ovs-ofctl -OOpenFlow13 add-flow br0 "table=17,  priority=10,metadata=0xe067870000000000/0xffffff0000000000 actions=write_metadata:0x67871d4d000000/0xfffffffffffffffe,goto_table:43"
+    ovs-ofctl -OOpenFlow13 add-flow br0 "table=43,  priority=100,icmp actions=group:6000"
+    ovs-ofctl -OOpenFlow13 add-flow br0 "table=48, priority=0 actions=resubmit(,49),resubmit(,50)"
+    ovs-ofctl -OOpenFlow13 add-flow br0 "table=50, priority=0 actions=controller"
+], [0], [ignore])
+
+AT_CHECK([ovs-ofctl monitor -OOpenFlow13 -P standard br0 65534 --detach --no-chdir --pidfile 2> ofctl_monitor.log])
+
+AT_CHECK([
+    ovs-appctl netdev-dummy/receive p1 1e2ce92a669e3a6dd2099cab0800450000548a53400040011addc0a80a0ac0a80a1e08006f200a4d0001fc509a58000000002715020000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637
+], [0], [ignore])
+
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 1])
+OVS_APP_EXIT_AND_WAIT(ovs-ofctl)
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+OFPT_PACKET_IN (OF1.3) (xid=0x0): table_id=43 cookie=0x0 total_len=98 metadata=0x67871d4d000000,in_port=1 (via action) data_len=98 (unbuffered)
+icmp,vlan_tci=0x0000,dl_src=3a:6d:d2:09:9c:ab,dl_dst=1e:2c:e9:2a:66:9e,nw_src=192.168.10.10,nw_dst=192.168.10.30,nw_tos=0,nw_ecn=0,nw_ttl=64,icmp_type=8,icmp_code=0 icmp_csum:6f20
+dnl
+OFPT_PACKET_IN (OF1.3) (xid=0x0): table_id=50 cookie=0x0 total_len=98 metadata=0x67871d4d000000,in_port=1 (via no_match) data_len=98 (unbuffered)
+icmp,vlan_tci=0x0000,dl_src=3a:6d:d2:09:9c:ab,dl_dst=1e:2c:e9:2a:66:9e,nw_src=192.168.10.10,nw_dst=192.168.10.30,nw_tos=0,nw_ecn=0,nw_ttl=64,icmp_type=8,icmp_code=0 icmp_csum:6f20
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
 
 AT_SETUP([ofproto-dpif - ARP modification slow-path])
 OVS_VSWITCHD_START
-- 
1.9.1



More information about the dev mailing list