Message ID | 1512557728-31050-3-git-send-email-zoltan.balogh@ericsson.com |
---|---|
State | Not Applicable |
Headers | show |
Series | xlate: optimize dp flow action in case of error in multi-bridge setup | expand |
Subject is incorrect, missing v2. Please use this instead: https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341658.html > -----Original Message----- > From: Zoltán Balogh > Sent: Wednesday, December 06, 2017 11:55 AM > To: dev@openvswitch.org > Cc: Zoltán Balogh <zoltan.balogh@ericsson.com>; Sugesh Chandran <sugesh.chandran@intel.com>; Ben Pfaff <blp@ovn.org> > Subject: [PATCH 2/2] xlate: normalize the actions after translation > > When all OF actions have been translated, there could be actions at > the end of list of odp actions which are not needed to be executed. > So, the list can be normalized at the end of xlate_actions(). > > Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com> > Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com> > Co-authored-by: Sugesh Chandran <sugesh.chandran@intel.com> > CC: Ben Pfaff <blp@ovn.org> > --- > ofproto/ofproto-dpif-xlate.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ > tests/ofproto-dpif.at | 48 +++++++++++++++++++++++++++++++ > 2 files changed, 115 insertions(+) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 36d0a0e1f..2af1ec1e8 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -6954,6 +6954,70 @@ xlate_wc_finish(struct xlate_ctx *ctx) > } > } > > +/* Returns true if the action stored in 'nla' can be a valid last action of a > + * datapath flow. */ > +static bool > +is_valid_last_action(const struct nlattr *nla) > +{ > + enum ovs_action_attr action_type = nl_attr_type(nla); > + > + switch (action_type) { > + case OVS_ACTION_ATTR_USERSPACE: > + case OVS_ACTION_ATTR_SAMPLE: > + case OVS_ACTION_ATTR_TRUNC: > + case OVS_ACTION_ATTR_RECIRC: > + case OVS_ACTION_ATTR_TUNNEL_PUSH: > + case OVS_ACTION_ATTR_TUNNEL_POP: > + case OVS_ACTION_ATTR_OUTPUT: > + case OVS_ACTION_ATTR_CLONE: > + case OVS_ACTION_ATTR_CT: > + return true; > + case OVS_ACTION_ATTR_UNSPEC: > + case OVS_ACTION_ATTR_SET: > + case OVS_ACTION_ATTR_PUSH_VLAN: > + case OVS_ACTION_ATTR_POP_VLAN: > + case OVS_ACTION_ATTR_HASH: > + case OVS_ACTION_ATTR_PUSH_MPLS: > + case OVS_ACTION_ATTR_POP_MPLS: > + case OVS_ACTION_ATTR_SET_MASKED: > + case OVS_ACTION_ATTR_PUSH_ETH: > + case OVS_ACTION_ATTR_POP_ETH: > + case OVS_ACTION_ATTR_METER: > + case OVS_ACTION_ATTR_ENCAP_NSH: > + case OVS_ACTION_ATTR_DECAP_NSH: > + case __OVS_ACTION_ATTR_MAX: > + default: > + return false; > + } > +} > + > +/* Returns offset of last netlink attribute storing valid action in array > + * 'data'. Execution of actions beyond this last attribute does not make sense. > + */ > +static size_t > +last_action_offset(const struct nlattr *data, const size_t data_len) > +{ > + const struct nlattr *last = data; > + > + uint16_t left; > + const struct nlattr *a; > + NL_ATTR_FOR_EACH (a, left, data, data_len) { > + if (is_valid_last_action(a)) { > + last = nl_attr_next(a); > + } > + } > + > + return (char *) last - (char *) data; > +} > + > +/* Get rid of any unneeded actions at the tail end. */ > +static void > +normalize_odp_actions(struct xlate_ctx *ctx) > +{ > + struct ofpbuf *oa = ctx->odp_actions; > + oa->size = last_action_offset(oa->data, oa->size); > +} > + > /* Translates the flow, actions, or rule in 'xin' into datapath actions in > * 'xout'. > * The caller must take responsibility for eventually freeing 'xout', with > @@ -7364,7 +7428,10 @@ exit: > if (xin->odp_actions) { > ofpbuf_clear(xin->odp_actions); > } > + } else { > + normalize_odp_actions(&ctx); > } > + > return ctx.error; > } > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index e7df1504a..5bcd8bb46 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -10133,3 +10133,51 @@ AT_CHECK([grep "Datapath actions" stdout], [0], > > OVS_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([ofproto-dpif - xlate error - normalize actions]) > + > +# ->-+ > +# | p1 > +# +-o-------+ > +# | br0 | > +# +-o-----o-+ > +# patch0| |patch1 > +# +-->--+ > + > +OVS_VSWITCHD_START([dnl > + -- add-port br0 patch0 \ > + -- set interface patch0 type=patch options:peer=patch1 ofport_request=10 \ > + -- add-port br0 patch1 \ > + -- set interface patch1 type=patch options:peer=patch0 ofport_request=20 > +]) > + > +AT_CHECK([ > + ovs-vsctl add-port br0 p1 -- set interface p1 type=dummy ofport_request=1 > +]) > + > +AT_CHECK([ > + ovs-appctl dpif/show | grep -v hit | sed "s/$(printf \\t)/ /g" | sed 's./[[0-9]]\{1,\}..' > +], [0], [dnl > + br0: > + br0 65534: (dummy-internal) > + p1 1: (dummy) > + patch0 10/none: (patch: peer=patch1) > + patch1 20/none: (patch: peer=patch0) > +]) > + > +# Error due to pushing too many MPLS labels. > +AT_CHECK([ > + ovs-ofctl del-flows br0 > + ovs-ofctl add-flow br0 "table=0,in_port=1,actions=mod_dl_src:aa:aa:aa:bb:bb:00,goto_table:1" -OOpenFlow13 > + ovs-ofctl add-flow br0 "table=0,in_port=patch1,actions=goto_table:2" -OOpenFlow13 > + ovs-ofctl add-flow br0 "table=1,actions=push_vlan:0x8100,set_field:4196->vlan_vid,output:patch0" -OOpenFlow13 > + ovs-ofctl add-flow br0 "table=2,actions=push_mpls:0x8847,output:patch0" > +], [0]) > + > +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,ip'], [0], [stdout]) > +AT_CHECK([tail -1 stdout], [0], > + [Datapath actions: drop > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > -- > 2.14.1
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 36d0a0e1f..2af1ec1e8 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -6954,6 +6954,70 @@ xlate_wc_finish(struct xlate_ctx *ctx) } } +/* Returns true if the action stored in 'nla' can be a valid last action of a + * datapath flow. */ +static bool +is_valid_last_action(const struct nlattr *nla) +{ + enum ovs_action_attr action_type = nl_attr_type(nla); + + switch (action_type) { + case OVS_ACTION_ATTR_USERSPACE: + case OVS_ACTION_ATTR_SAMPLE: + case OVS_ACTION_ATTR_TRUNC: + case OVS_ACTION_ATTR_RECIRC: + case OVS_ACTION_ATTR_TUNNEL_PUSH: + case OVS_ACTION_ATTR_TUNNEL_POP: + case OVS_ACTION_ATTR_OUTPUT: + case OVS_ACTION_ATTR_CLONE: + case OVS_ACTION_ATTR_CT: + return true; + case OVS_ACTION_ATTR_UNSPEC: + case OVS_ACTION_ATTR_SET: + case OVS_ACTION_ATTR_PUSH_VLAN: + case OVS_ACTION_ATTR_POP_VLAN: + case OVS_ACTION_ATTR_HASH: + case OVS_ACTION_ATTR_PUSH_MPLS: + case OVS_ACTION_ATTR_POP_MPLS: + case OVS_ACTION_ATTR_SET_MASKED: + case OVS_ACTION_ATTR_PUSH_ETH: + case OVS_ACTION_ATTR_POP_ETH: + case OVS_ACTION_ATTR_METER: + case OVS_ACTION_ATTR_ENCAP_NSH: + case OVS_ACTION_ATTR_DECAP_NSH: + case __OVS_ACTION_ATTR_MAX: + default: + return false; + } +} + +/* Returns offset of last netlink attribute storing valid action in array + * 'data'. Execution of actions beyond this last attribute does not make sense. + */ +static size_t +last_action_offset(const struct nlattr *data, const size_t data_len) +{ + const struct nlattr *last = data; + + uint16_t left; + const struct nlattr *a; + NL_ATTR_FOR_EACH (a, left, data, data_len) { + if (is_valid_last_action(a)) { + last = nl_attr_next(a); + } + } + + return (char *) last - (char *) data; +} + +/* Get rid of any unneeded actions at the tail end. */ +static void +normalize_odp_actions(struct xlate_ctx *ctx) +{ + struct ofpbuf *oa = ctx->odp_actions; + oa->size = last_action_offset(oa->data, oa->size); +} + /* Translates the flow, actions, or rule in 'xin' into datapath actions in * 'xout'. * The caller must take responsibility for eventually freeing 'xout', with @@ -7364,7 +7428,10 @@ exit: if (xin->odp_actions) { ofpbuf_clear(xin->odp_actions); } + } else { + normalize_odp_actions(&ctx); } + return ctx.error; } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index e7df1504a..5bcd8bb46 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -10133,3 +10133,51 @@ AT_CHECK([grep "Datapath actions" stdout], [0], OVS_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([ofproto-dpif - xlate error - normalize actions]) + +# ->-+ +# | p1 +# +-o-------+ +# | br0 | +# +-o-----o-+ +# patch0| |patch1 +# +-->--+ + +OVS_VSWITCHD_START([dnl + -- add-port br0 patch0 \ + -- set interface patch0 type=patch options:peer=patch1 ofport_request=10 \ + -- add-port br0 patch1 \ + -- set interface patch1 type=patch options:peer=patch0 ofport_request=20 +]) + +AT_CHECK([ + ovs-vsctl add-port br0 p1 -- set interface p1 type=dummy ofport_request=1 +]) + +AT_CHECK([ + ovs-appctl dpif/show | grep -v hit | sed "s/$(printf \\t)/ /g" | sed 's./[[0-9]]\{1,\}..' +], [0], [dnl + br0: + br0 65534: (dummy-internal) + p1 1: (dummy) + patch0 10/none: (patch: peer=patch1) + patch1 20/none: (patch: peer=patch0) +]) + +# Error due to pushing too many MPLS labels. +AT_CHECK([ + ovs-ofctl del-flows br0 + ovs-ofctl add-flow br0 "table=0,in_port=1,actions=mod_dl_src:aa:aa:aa:bb:bb:00,goto_table:1" -OOpenFlow13 + ovs-ofctl add-flow br0 "table=0,in_port=patch1,actions=goto_table:2" -OOpenFlow13 + ovs-ofctl add-flow br0 "table=1,actions=push_vlan:0x8100,set_field:4196->vlan_vid,output:patch0" -OOpenFlow13 + ovs-ofctl add-flow br0 "table=2,actions=push_mpls:0x8847,output:patch0" +], [0]) + +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,ip'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], + [Datapath actions: drop +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP