Thanks for working on it! Can we have nested clone?
actions=clone(push_vlan(1), resubmit(1,2), clone(mod_dl_src:<mac1>, output:5) ) BTW, I believe the port 3 will get a 104byte size packet with this action, right? actions= clone(truncate(100), push_vlan, output:3) Thanks Zhenyu Gao 2016-12-01 5:35 GMT+08:00 William Tu <[email protected]>: > This patch adds OpenFlow clone action with syntax as below: > "clone([action][,action...])". The clone() action makes a copy of the > current packet and executes the list of actions against the packet, > without affecting the packet after the "clone(...)" action. In other > word, the packet before the clone() and after the clone() is the same, > no matter what actions executed inside the clone(). > > The patch reuses the dapatah SAMPLE action, with probability of 100%. > The kernel datapath 'sample()' clones the skb and its flow key under this > circumstance, and actions specified in the clone() are executed against > the cloned skb. Note that there is no performance overhead of clone, since > if eventually the packet is output, it will also clone the packet. Here we > simply move the copy at the beginning. > > The complexity of adding clone might outweight its use cases. I'm looking > for comments as well as listing some cases below: > Use case 1: > Set different fields and output to different ports without unset > actions= > clone(mod_dl_src:<mac1>, output:1), clone(mod_dl_dst:<mac2>, output:2), > output:3 > Since each clone() has independent packet, output:1 has only dl_src > modified, > output:2 has only dl_dst modified, output:3 has original packet. > > Similar to case1 > actions= > push_vlan(...), output:2, pop_vlan, push_vlan(...), output:3 > can be changed to > actions= > clone(push_vlan(...), output:2),clone(push_vlan(...), output:3) > without having to add pop_vlan. > > case 2: resubmit to another table without worrying packet being modified > actions=clone(resubmit(1,2)), ... > > case 3: truncate in the clone action > Currently OVS can truncate packet by 'output(port=1,max_len=100)', which > ties truncate and output together. However, sometimes the layer decides > to truncate is separate from the layer to output. One proposal is to > introduce actions s.t like truncate_set() and truncate_unset(), where > only the action in between sees truncated packet. Another approach is > to use clone() as below: > actions= > clone(truncate(100), push_vlan, resubmit, ...) > where we don't need to worry about missing the truncate_unset() because > truncated packet is not visible outside the clone(). > > We definitely should put some limit on the action types available inside > clone(). For this patch, there is no restriction. > > Signed-off-by: William Tu <[email protected]> > --- > v1->v2 > - rebase and change NX1.0+(41) to 42 > --- > include/openvswitch/ofp-actions.h | 23 +++++++++ > lib/ofp-actions.c | 101 ++++++++++++++++++++++++++++++ > +++++++- > ofproto/ofproto-dpif-xlate.c | 35 +++++++++++++ > tests/ofproto-dpif.at | 19 +++++++ > tests/system-traffic.at | 29 +++++++++++ > 5 files changed, 206 insertions(+), 1 deletion(-) > > diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp- > actions.h > index 2999261..be4a991 100644 > --- a/include/openvswitch/ofp-actions.h > +++ b/include/openvswitch/ofp-actions.h > @@ -109,6 +109,7 @@ > OFPACT(CT, ofpact_conntrack, ofpact, "ct") \ > OFPACT(NAT, ofpact_nat, ofpact, "nat") \ > OFPACT(OUTPUT_TRUNC, ofpact_output_trunc,ofpact, "output_trunc") \ > + OFPACT(CLONE, ofpact_clone, ofpact, "clone") \ > \ > /* Debugging actions. \ > * \ > @@ -868,6 +869,28 @@ struct ofpact_sample { > enum nx_action_sample_direction direction; > }; > > +/* OFPACT_CLONE. > + * > + * Used for cloning the packet and execute actions > + * in the clone envelope. */ > +struct ofpact_clone { > + OFPACT_PADDED_MEMBERS( > + struct ofpact ofpact; > + ); > + struct ofpact actions[0]; > +}; > + > +BUILD_ASSERT_DECL(offsetof(struct ofpact_clone, actions) > + % OFPACT_ALIGNTO == 0); > +BUILD_ASSERT_DECL(offsetof(struct ofpact_clone, actions) > + == sizeof(struct ofpact_clone)); > + > +static inline size_t > +ofpact_clone_get_action_len(const struct ofpact_clone *oc) > +{ > + return oc->ofpact.len - offsetof(struct ofpact_clone, actions); > +} > + > /* OFPACT_DEC_TTL. > * > * Used for OFPAT11_DEC_NW_TTL, NXAST_DEC_TTL and NXAST_DEC_TTL_CNT_IDS. > */ > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index 7507558..227ecb2 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -306,6 +306,9 @@ enum ofp_raw_action_type { > /* NX1.0+(39): struct nx_action_output_trunc. */ > NXAST_RAW_OUTPUT_TRUNC, > > + /* NX1.0+(42): struct nx_action_clone, ... */ > + NXAST_RAW_CLONE, > + > /* ## ------------------ ## */ > /* ## Debugging actions. ## */ > /* ## ------------------ ## */ > @@ -386,6 +389,7 @@ ofpact_next_flattened(const struct ofpact *ofpact) > switch (ofpact->type) { > case OFPACT_OUTPUT: > case OFPACT_GROUP: > + case OFPACT_CLONE: > case OFPACT_CONTROLLER: > case OFPACT_ENQUEUE: > case OFPACT_OUTPUT_REG: > @@ -4801,6 +4805,90 @@ format_UNROLL_XLATE(const struct > ofpact_unroll_xlate *a, struct ds *s) > colors.paren, colors.end); > } > > + > +struct nx_action_clone { > + ovs_be16 type; /* OFPAT_VENDOR. */ > + ovs_be16 len; /* Length is at least 16. */ > + ovs_be32 vendor; /* NX_VENDOR_ID. */ > + ovs_be16 subtype; /* NXAST_CLONE. */ > + ovs_be16 pad0; > + ovs_be32 pad1; > + /* Followed by a sequence of zero or more OpenFlow actions. The > length > + * of these is included in 'len'. */ > +}; > +OFP_ASSERT(sizeof(struct nx_action_clone) == 16); > + > +static enum ofperr > +decode_NXAST_RAW_CLONE(const struct nx_action_clone *nac, > + enum ofp_version ofp_version, > + struct ofpbuf *out) > +{ > + int error; > + struct ofpbuf openflow; > + const size_t clone_offset = ofpacts_pull(out); > + struct ofpact_clone *clone = ofpact_put_CLONE(out); > + > + /* decode action list */ > + ofpbuf_pull(out, sizeof(*clone)); > + openflow = ofpbuf_const_initializer( > + nac + 1, ntohs(nac->len) - sizeof(*nac)); > + error = ofpacts_pull_openflow_actions__(&openflow, openflow.size, > + ofp_version, > + 1u << > OVSINST_OFPIT11_APPLY_ACTIONS, > + out, OFPACT_CLONE); > + clone = ofpbuf_push_uninit(out, sizeof(*clone)); > + out->header = &clone->ofpact; > + ofpact_finish_CLONE(out, &clone); > + ofpbuf_push_uninit(out, clone_offset); > + return error; > +} > + > +static void > +encode_CLONE(const struct ofpact_clone *clone, > + enum ofp_version ofp_version, struct ofpbuf *out) > +{ > + size_t len; > + const size_t ofs = out->size; > + struct nx_action_clone *nac; > + > + nac = put_NXAST_CLONE(out); > + len = ofpacts_put_openflow_actions(clone->actions, > + ofpact_clone_get_action_len(clone), > + out, ofp_version); > + len += sizeof(*nac); > + nac = ofpbuf_at(out, ofs, sizeof(*nac)); > + nac->len = htons(len); > +} > + > +static char * OVS_WARN_UNUSED_RESULT > +parse_CLONE(char *arg, struct ofpbuf *ofpacts, > + enum ofputil_protocol *usable_protocols) > +{ > + > + const size_t ct_offset = ofpacts_pull(ofpacts); > + struct ofpact_clone *clone = ofpact_put_CLONE(ofpacts); > + char *error; > + > + ofpbuf_pull(ofpacts, sizeof(*clone)); > + error = ofpacts_parse_copy(arg, ofpacts, usable_protocols, > + false, OFPACT_CLONE); > + /* header points to the action list */ > + ofpacts->header = ofpbuf_push_uninit(ofpacts, sizeof(*clone)); > + clone = ofpacts->header; > + > + ofpact_finish_CLONE(ofpacts, &clone); > + ofpbuf_push_uninit(ofpacts, ct_offset); > + return error; > +} > + > +static void > +format_CLONE(const struct ofpact_clone *a, struct ds *s) > +{ > + ds_put_format(s, "%sclone(%s", colors.paren, colors.end); > + ofpacts_format(a->actions, ofpact_clone_get_action_len(a), s); > + ds_put_format(s, "%s)%s", colors.paren, colors.end); > +} > + > /* Action structure for NXAST_SAMPLE. > * > * Samples matching packets with the given probability and sends them > @@ -6212,6 +6300,7 @@ ofpact_is_set_or_move_action(const struct ofpact *a) > case OFPACT_BUNDLE: > case OFPACT_CLEAR_ACTIONS: > case OFPACT_CT: > + case OFPACT_CLONE: > case OFPACT_NAT: > case OFPACT_CONTROLLER: > case OFPACT_DEC_MPLS_TTL: > @@ -6288,6 +6377,7 @@ ofpact_is_allowed_in_actions_set(const struct > ofpact *a) > * the specification. Thus the order in which they should be applied > * in the action set is undefined. */ > case OFPACT_BUNDLE: > + case OFPACT_CLONE: > case OFPACT_CONTROLLER: > case OFPACT_CT: > case OFPACT_NAT: > @@ -6480,6 +6570,7 @@ ovs_instruction_type_from_ofpact_type(enum > ofpact_type type) > return OVSINST_OFPIT11_GOTO_TABLE; > case OFPACT_OUTPUT: > case OFPACT_GROUP: > + case OFPACT_CLONE: > case OFPACT_CONTROLLER: > case OFPACT_ENQUEUE: > case OFPACT_OUTPUT_REG: > @@ -7080,6 +7171,9 @@ ofpact_check__(enum ofputil_protocol > *usable_protocols, struct ofpact *a, > case OFPACT_SAMPLE: > return 0; > > + case OFPACT_CLONE: > + return 0; > + > case OFPACT_CT: { > struct ofpact_conntrack *oc = ofpact_get_CT(a); > > @@ -7271,7 +7365,7 @@ ofpacts_verify_nested(const struct ofpact *a, enum > ofpact_type outer_action) > > if (outer_action) { > ovs_assert(outer_action == OFPACT_WRITE_ACTIONS > - || outer_action == OFPACT_CT); > + || outer_action == OFPACT_CT || outer_action == > OFPACT_CLONE); > > if (outer_action == OFPACT_CT) { > if (!field) { > @@ -7282,6 +7376,10 @@ ofpacts_verify_nested(const struct ofpact *a, enum > ofpact_type outer_action) > return OFPERR_OFPBAC_BAD_ARGUMENT; > } > } > + if (outer_action == OFPACT_CLONE) { > + /* add some constraints. */ > + return 0; > + } > } > > return 0; > @@ -7635,6 +7733,7 @@ ofpact_outputs_to_port(const struct ofpact *ofpact, > ofp_port_t port) > case OFPACT_POP_MPLS: > case OFPACT_SAMPLE: > case OFPACT_CLEAR_ACTIONS: > + case OFPACT_CLONE: > case OFPACT_WRITE_ACTIONS: > case OFPACT_GOTO_TABLE: > case OFPACT_METER: > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 55ac371..b534b31 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -4289,6 +4289,35 @@ xlate_sample_action(struct xlate_ctx *ctx, > tunnel_out_port, false); > } > > +static void > +compose_clone_action(struct xlate_ctx *ctx, > + const struct ofpact_clone *oc) > +{ > + struct flow old_flow, old_base_flow; > + size_t actions_offset; > + size_t sample_offset = nl_msg_start_nested(ctx->odp_actions, > + OVS_ACTION_ATTR_SAMPLE); > + > + /* Ensure that any prior actions are applied. */ > + xlate_commit_actions(ctx); > + > + /* clone might change the flow key, store the original. */ > + old_flow = ctx->xin->flow; > + old_base_flow = ctx->base_flow; > + > + /* Sample with 100% Probability */ > + nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, > UINT32_MAX); > + actions_offset = nl_msg_start_nested(ctx->odp_actions, > + OVS_SAMPLE_ATTR_ACTIONS); > + do_xlate_actions(oc->actions, ofpact_clone_get_action_len(oc), ctx); > + nl_msg_end_nested(ctx->odp_actions, actions_offset); > + nl_msg_end_nested(ctx->odp_actions, sample_offset); > + > + /* restore flow key */ > + ctx->xin->flow = old_flow; > + ctx->base_flow = old_base_flow; > +} > + > static bool > may_receive(const struct xport *xport, struct xlate_ctx *ctx) > { > @@ -4447,6 +4476,7 @@ freeze_unroll_actions(const struct ofpact *a, const > struct ofpact *end, > case OFPACT_WRITE_ACTIONS: > case OFPACT_METER: > case OFPACT_SAMPLE: > + case OFPACT_CLONE: > case OFPACT_DEBUG_RECIRC: > case OFPACT_CT: > case OFPACT_NAT: > @@ -4695,6 +4725,7 @@ recirc_for_mpls(const struct ofpact *a, struct > xlate_ctx *ctx) > case OFPACT_NOTE: > case OFPACT_EXIT: > case OFPACT_SAMPLE: > + case OFPACT_CLONE: > case OFPACT_UNROLL_XLATE: > case OFPACT_CT: > case OFPACT_NAT: > @@ -5054,6 +5085,10 @@ do_xlate_actions(const struct ofpact *ofpacts, > size_t ofpacts_len, > xlate_sample_action(ctx, ofpact_get_SAMPLE(a)); > break; > > + case OFPACT_CLONE: > + compose_clone_action(ctx, ofpact_get_CLONE(a)); > + break; > + > case OFPACT_CT: > compose_conntrack_action(ctx, ofpact_get_CT(a)); > break; > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index ec7bd60..999f9fd 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -6388,6 +6388,25 @@ AT_CHECK([ovs-vsctl destroy > Flow_Sample_Collector_Set 1], [0], [ignore]) > OVS_VSWITCHD_STOP > AT_CLEANUP > > +dnl OpenFlow Clone action using Sample datapath action > +AT_SETUP([ofproto-dpif - clone action]) > +OVS_VSWITCHD_START > +add_of_ports br0 1 2 3 4 > + > +AT_DATA([flows.txt], [dnl > +in_port=1, actions=clone(set_field:192.168.3.3->ip_src),clone(set_ > field:192.168.4.4->ip_dst,output:2),clone(mod_dl_src:80: > 81:81:81:81:81,set_field:192.168.5.5->ip_dst,output:3),output:4 > +]) > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00: > 00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src= > 10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], > [0], [stdout]) > + > +AT_CHECK([tail -1 stdout], [0], [dnl > +Datapath actions: sample(sample=100.0%,actions( > drop)),sample(sample=100.0%,actions(set(ipv4(src=10.10.10. > 2,dst=192.168.4.4)),2)),sample(sample=100.0%,actions( > set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst= > 192.168.5.5)),3)),4 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > dnl Flow based IPFIX statistics check > AT_SETUP([ofproto-dpif - Flow IPFIX statistics check]) > OVS_VSWITCHD_START > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 801dfe3..31e5a16 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -337,6 +337,35 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 > -w 2 10.1.1.100 | FORMAT_PI > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([datapath - clone action]) > +OVS_TRAFFIC_VSWITCHD_START() > + > +ADD_NAMESPACES(at_ns0, at_ns1, at_ns2) > + > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") > +ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24") > + > +AT_CHECK([ovs-vsctl -- set interface ovs-p0 ofport_request=1]) > +AT_CHECK([ovs-vsctl -- set interface ovs-p1 ofport_request=2]) > +AT_CHECK([ovs-vsctl -- set interface ovs-p2 ofport_request=3]) > + > +dnl verify that the clone(...) won't affect the original packet, so ping > still works OK > +dnl without 'output' in 'clone()' > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=1,actions=clone(mod_ > dl_dst(50:54:00:00:00:0a),set_field:192.168.3.3->ip_dst), output:2"]) > +dnl with 'output' in 'clone()' > +AT_CHECK([ovs-ofctl add-flow br0 "in_port=2,actions=clone(mod_ > dl_dst(50:54:00:00:00:0b),set_field:192.168.4.4->ip_dst, output:3), > output:1"]) > + > +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | > FORMAT_PING], [0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py]], [http0.pid]) > +NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v > -o wget0.log]) > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([datapath - basic truncate action]) > OVS_TRAFFIC_VSWITCHD_START() > AT_CHECK([ovs-ofctl del-flows br0]) > -- > 2.5.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
