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 <u9012...@gmail.com>:

> 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 <u9012...@gmail.com>
> ---
> 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to