Hi Yi-Hung, I read through the patch and have some questions and comments.
On Fri, Apr 10, 2020 at 01:05:24PM -0700, Yi-Hung Wei wrote: > This patch adds a new OpenFlow action, delete field, to delete a > field in packets. Currently, only the tun_metadata fields are > supported. > > One use case to add this action is to support multiple versions > of geneve tunnel metadatas to be exchanged among different versions > of networks. For example, we may introduce tun_metadata2 to > replace old tun_metadata1, but still want to provide backward > compatibility to the older release. In this case, in the new > OpenFlow pipeline, we would like to support the case to receive a > packet with tun_metadata1, do some processing. And if the packet > is going to a switch in the newer release, we would like to delete > the value in tun_metadata1 and set a value into tun_metadata2. > > Currently, ovs does not provide an action to remove a value in > tun_metadata if the value is present. This patch fulfills the gap > by adding the delete_field action. The OpenFlow syntax is: > > actions: delete_field:tun_metadata1 actions=delete_field:tun_metadata > > Signed-off-by: Yi-Hung Wei <[email protected]> > --- > Travis CI: https://travis-ci.org/github/YiHungWei/ovs/builds/673081110 > --- > NEWS | 1 + > include/openvswitch/ofp-actions.h | 13 +++++- > lib/nx-match.c | 20 ++++++++- > lib/nx-match.h | 4 +- > lib/ofp-actions.c | 90 > ++++++++++++++++++++++++++++++++++++++- > lib/ovs-actions.xml | 16 +++++++ > lib/tun-metadata.c | 17 ++++++++ > lib/tun-metadata.h | 1 + > ofproto/ofproto-dpif-xlate.c | 24 ++++++++++- > tests/ofp-actions.at | 3 ++ > tests/tunnel.at | 30 +++++++++++++ > 11 files changed, 214 insertions(+), 5 deletions(-) > > diff --git a/NEWS b/NEWS > index 70bd17584594..58fbcedcb9bf 100644 > --- a/NEWS > +++ b/NEWS > @@ -5,6 +5,7 @@ Post-v2.13.0 > * The OpenFlow ofp_desc/serial_num may now be configured by setting the > value of other-config:dp-sn in the Bridge table. > * Added support to watch CONTROLLER port status in fast failover group. > + * New action "delete_field". > - DPDK: > * Deprecated DPDK pdump packet capture support removed. > * Deprecated DPDK ring ports (dpdkr) are no longer supported. > diff --git a/include/openvswitch/ofp-actions.h > b/include/openvswitch/ofp-actions.h > index c8948e0d694f..226e86d0b0a5 100644 > --- a/include/openvswitch/ofp-actions.h > +++ b/include/openvswitch/ofp-actions.h > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2012, 2013, 2014, 2015, 2016, 2017, 2019 Nicira, Inc. > + * Copyright (c) 2012-2017, 2019-2020 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -94,6 +94,7 @@ struct vl_mff_map; > OFPACT(PUSH_MPLS, ofpact_push_mpls, ofpact, "push_mpls") \ > OFPACT(POP_MPLS, ofpact_pop_mpls, ofpact, "pop_mpls") \ > OFPACT(DEC_NSH_TTL, ofpact_null, ofpact, "dec_nsh_ttl") \ > + OFPACT(DELETE_FIELD, ofpact_delete_field, ofpact, "delete_field") \ > \ > /* Generic encap & decap */ \ > OFPACT(ENCAP, ofpact_encap, props, "encap") \ > @@ -576,6 +577,16 @@ struct ofpact_pop_mpls { > ); > }; > > +/* OFPACT_DELETE_FIELD. > + * > + * Used for NXAST_DELETE_FIELD. */ > +struct ofpact_delete_field { > + OFPACT_PADDED_MEMBERS( > + struct ofpact ofpact; > + const struct mf_field *field; > + ); > +}; > + > /* OFPACT_SET_TUNNEL. > * > * Used for NXAST_SET_TUNNEL, NXAST_SET_TUNNEL64. */ > diff --git a/lib/nx-match.c b/lib/nx-match.c > index 058816c7b88f..3ffd7d9d7711 100644 > --- a/lib/nx-match.c > +++ b/lib/nx-match.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc. > + * Copyright (c) 2010-2017, 2020 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -1994,6 +1994,24 @@ nxm_execute_stack_pop(const struct ofpact_stack *pop, > } > } > > +/* Parses a field from '*s' into '*field'. If successful, stores the > + * reference to the field in '*field', and returns NULL. On failure, > + * returns a malloc()'ed error message. > + */ > +char * OVS_WARN_UNUSED_RESULT > +mf_parse_field(const struct mf_field **field, const char *s) > +{ > + const struct nxm_field *f; > + int s_len = strlen(s); > + > + f = nxm_field_by_name(s, s_len); > + (*field) = f ? mf_from_id(f->id) : mf_from_name_len(s, s_len); > + if (!*field) { > + return xasprintf("unknown field `%s'", s); > + } > + return NULL; > +} > + > /* Formats 'sf' into 's' in a format normally acceptable to > * mf_parse_subfield(). (It won't be acceptable if sf->field is NULL or if > * sf->field has no NXM name.) */ > diff --git a/lib/nx-match.h b/lib/nx-match.h > index 9be40a98150e..3120ac0a0d5f 100644 > --- a/lib/nx-match.h > +++ b/lib/nx-match.h > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2010-2017 Nicira, Inc. > + * Copyright (c) 2010-2017, 2020 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -42,6 +42,8 @@ struct vl_mff_map; > * See include/openflow/nicira-ext.h for NXM specification. > */ > > +char * mf_parse_field(const struct mf_field **field, const char *s) > + OVS_WARN_UNUSED_RESULT; > void mf_format_subfield(const struct mf_subfield *, struct ds *); > char *mf_parse_subfield__(struct mf_subfield *sf, const char **s) > OVS_WARN_UNUSED_RESULT; > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index ef8b2b4527f9..9e76148ba8b0 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2008-2017, 2019 Nicira, Inc. > + * Copyright (c) 2008-2017, 2019-2020 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -361,6 +361,9 @@ enum ofp_raw_action_type { > /* NX1.0+(49): struct nx_action_check_pkt_larger, ... VLMFF */ > NXAST_RAW_CHECK_PKT_LARGER, > > + /* NX1.0+(50): struct nx_action_delete_field. VLMFF */ > + NXAST_RAW_DELETE_FIELD, > + > /* ## ------------------ ## */ > /* ## Debugging actions. ## */ > /* ## ------------------ ## */ > @@ -500,6 +503,7 @@ ofpact_next_flattened(const struct ofpact *ofpact) > case OFPACT_DECAP: > case OFPACT_DEC_NSH_TTL: > case OFPACT_CHECK_PKT_LARGER: > + case OFPACT_DELETE_FIELD: > return ofpact_next(ofpact); > > case OFPACT_CLONE: > @@ -4140,6 +4144,87 @@ check_SET_TUNNEL(const struct ofpact_tunnel *a > OVS_UNUSED, > return 0; > } > > +/* Delete field action. */ > + > +/* Action structure for DELETE_FIELD */ > +struct nx_action_delete_field { > + ovs_be16 type; /* OFPAT_VENDOR */ > + ovs_be16 len; /* Length is 24. */ > + ovs_be32 vendor; /* NX_VENDOR_ID. */ > + ovs_be16 subtype; /* NXAST_DELETE_FIELD. */ > + /* Followed by: > + * - OXM/NXM header for field to delete (4 or 8 bytes). > + * - Enough 0-bytes to pad out the action to 24 bytes. */ > + uint8_t pad[14]; > +}; > +OFP_ASSERT(sizeof(struct nx_action_delete_field ) == 24); > + > +static enum ofperr > +decode_NXAST_RAW_DELETE_FIELD(const struct nx_action_delete_field *nadf, > + enum ofp_version ofp_version OVS_UNUSED, > + const struct vl_mff_map *vl_mff_map, > + uint64_t *tlv_bitmap, struct ofpbuf *out) > +{ > + struct ofpact_delete_field *delete_field; > + enum ofperr err; > + > + delete_field = ofpact_put_DELETE_FIELD(out); > + delete_field->ofpact.raw = NXAST_RAW_DELETE_FIELD; > + > + struct ofpbuf b = ofpbuf_const_initializer(nadf, ntohs(nadf->len)); > + ofpbuf_pull(&b, OBJECT_OFFSETOF(nadf, pad)); > + > + err = mf_vl_mff_nx_pull_header(&b, vl_mff_map, &delete_field->field, > + NULL, tlv_bitmap); > + if (err) { > + return err; > + } > + > + return 0; > +} > + > +static void > +encode_DELETE_FIELD(const struct ofpact_delete_field *delete_field, > + enum ofp_version ofp_version OVS_UNUSED, > + struct ofpbuf *out) alignment, missing one space > +{ > + struct nx_action_delete_field *nadf = put_NXAST_DELETE_FIELD(out); > + size_t size = out->size; > + > + out->size = size - sizeof nadf->pad; > + nx_put_mff_header(out, delete_field->field, 0, false); > + out->size = size; question: why setting the size back? > +} > + > +static char * OVS_WARN_UNUSED_RESULT > +parse_DELETE_FIELD(char *arg, const struct ofpact_parse_params *pp) > +{ > + struct ofpact_delete_field *delete_field; > + > + delete_field = ofpact_put_DELETE_FIELD(pp->ofpacts); > + return mf_parse_field(&delete_field->field, arg); > +} > + > +static void > +format_DELETE_FIELD(const struct ofpact_delete_field *odf, > + const struct ofpact_format_params *fp) > +{ > + ds_put_format(fp->s, "%sdelete_field:%s", colors.param, > + colors.end); > + ds_put_format(fp->s, "%s", odf->field->name); > +} > + > +static enum ofperr > +check_DELETE_FIELD(const struct ofpact_delete_field *odf, > + struct ofpact_check_params *cp OVS_UNUSED) > +{ > + if (odf->field->id < MFF_TUN_METADATA0 || > + odf->field->id > MFF_TUN_METADATA63) { > + return OFPERR_OFPBAC_BAD_ARGUMENT; > + } > + return 0; > +} > + > /* Set queue action. */ > > static enum ofperr > @@ -7869,6 +7954,7 @@ action_set_classify(const struct ofpact *a) > case OFPACT_DEBUG_RECIRC: > case OFPACT_DEBUG_SLOW: > case OFPACT_CHECK_PKT_LARGER: > + case OFPACT_DELETE_FIELD: > return ACTION_SLOT_INVALID; > > default: > @@ -8072,6 +8158,7 @@ ovs_instruction_type_from_ofpact_type(enum ofpact_type > type, > case OFPACT_DECAP: > case OFPACT_DEC_NSH_TTL: > case OFPACT_CHECK_PKT_LARGER: > + case OFPACT_DELETE_FIELD: > default: > return OVSINST_OFPIT11_APPLY_ACTIONS; > } > @@ -8983,6 +9070,7 @@ ofpact_outputs_to_port(const struct ofpact *ofpact, > ofp_port_t port) > case OFPACT_DECAP: > case OFPACT_DEC_NSH_TTL: > case OFPACT_CHECK_PKT_LARGER: > + case OFPACT_DELETE_FIELD: > default: > return false; > } > diff --git a/lib/ovs-actions.xml b/lib/ovs-actions.xml > index ab8e08b84d8b..5acf6fe64fc5 100644 > --- a/lib/ovs-actions.xml > +++ b/lib/ovs-actions.xml > @@ -1552,6 +1552,22 @@ for <var>i</var> in [1,<var>n_slaves</var>]: > This action was added in Open vSwitch 2.11.90. > </p> > </action> > + > + <action name="DELETE_FIELD"> > + <h2>The <code>delete_field</code>action</h2> missing a space before 'action'? > + <syntax><code>delete_field:</code><var>field</var></syntax> > + > + <p> > + The <code>delete_field</code> action deletes a field in the syntax > + described under ``Field Specifications'' above. Currently, only > + the tun_metadta fields are supported. > + </p> > + > + <p> > + This action was added in Open vSwitch 2.13.90. > + </p> > + </action> > + > </group> > > <group title="Metadata Actions"> > diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c > index f8a0e19524e9..c0b0ae044897 100644 > --- a/lib/tun-metadata.c > +++ b/lib/tun-metadata.c > @@ -261,6 +261,23 @@ tun_metadata_write(struct flow_tnl *tnl, > value->tun_metadata + mf->n_bytes - loc->len, loc, > idx); > } > > +/* Deletes field 'mf' in 'tnl' (in non-UDPIF format). > + * 'mf' must be an MFF_TUN_METADATA* field. > + */ > +void > +tun_metadata_delete(struct flow_tnl *tnl, const struct mf_field *mf) > +{ > + unsigned int idx; > + > + if (tnl->flags & FLOW_TNL_F_UDPIF) { > + return; > + } > + > + idx = mf->id - MFF_TUN_METADATA0; > + ovs_assert(idx < TUN_METADATA_NUM_OPTS); > + ULLONG_SET0(tnl->metadata.present.map, idx); question: why only set the present bit to 0? why not delete the entry, ex: tun_metadata_del_entry()? > +} > + > static const struct tun_metadata_loc * > metadata_loc_from_match(const struct tun_table *map, struct match *match, > const char *name, unsigned int idx, > diff --git a/lib/tun-metadata.h b/lib/tun-metadata.h > index 7dad9504b8da..67dedae2522b 100644 > --- a/lib/tun-metadata.h > +++ b/lib/tun-metadata.h > @@ -47,6 +47,7 @@ void tun_metadata_read(const struct flow_tnl *, > const struct mf_field *, union mf_value *); > void tun_metadata_write(struct flow_tnl *, > const struct mf_field *, const union mf_value *); > +void tun_metadata_delete(struct flow_tnl *, const struct mf_field *); > void tun_metadata_set_match(const struct mf_field *, > const union mf_value *value, > const union mf_value *mask, struct match *, > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 042c50a6346c..009961863093 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2019 > Nicira, Inc. > +/* Copyright (c) 2009-2017, 2019-2020 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -5159,6 +5159,21 @@ compose_dec_mpls_ttl_action(struct xlate_ctx *ctx) > return true; > } > > +static void > +xlate_delete_field(struct xlate_ctx *ctx, > + struct flow *flow, > + const struct ofpact_delete_field *odf) > +{ > + struct ds s = DS_EMPTY_INITIALIZER; > + > + /* Currently, only tun_metadata is allowed for delete_field action. */ > + tun_metadata_delete(&flow->tunnel, odf->field); > + > + ds_put_format(&s, "delete %s", odf->field->name); > + xlate_report(ctx, OFT_DETAIL, "%s", ds_cstr(&s)); > + ds_destroy(&s); > +} > + > /* Emits an action that outputs to 'port', within 'ctx'. > * > * 'controller_len' affects only packets sent to an OpenFlow controller. It > @@ -5684,6 +5699,7 @@ reversible_actions(const struct ofpact *ofpacts, size_t > ofpacts_len) > case OFPACT_WRITE_ACTIONS: > case OFPACT_WRITE_METADATA: > case OFPACT_CHECK_PKT_LARGER: > + case OFPACT_DELETE_FIELD: > break; > > case OFPACT_CT: > @@ -5993,6 +6009,7 @@ freeze_unroll_actions(const struct ofpact *a, const > struct ofpact *end, > case OFPACT_CT_CLEAR: > case OFPACT_NAT: > case OFPACT_CHECK_PKT_LARGER: > + case OFPACT_DELETE_FIELD: > /* These may not generate PACKET INs. */ > break; > > @@ -6653,6 +6670,7 @@ recirc_for_mpls(const struct ofpact *a, struct > xlate_ctx *ctx) > case OFPACT_WRITE_METADATA: > case OFPACT_GOTO_TABLE: > case OFPACT_CHECK_PKT_LARGER: > + case OFPACT_DELETE_FIELD: > default: > break; > } > @@ -7030,6 +7048,10 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t > ofpacts_len, > xlate_fin_timeout(ctx, ofpact_get_FIN_TIMEOUT(a)); > break; > > + case OFPACT_DELETE_FIELD: > + xlate_delete_field(ctx, flow, ofpact_get_DELETE_FIELD(a)); > + break; > + > case OFPACT_CLEAR_ACTIONS: > xlate_report_action_set(ctx, "was"); > ofpbuf_clear(&ctx->action_set); > diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at > index 4893280a998f..28b2099a0c71 100644 > --- a/tests/ofp-actions.at > +++ b/tests/ofp-actions.at > @@ -316,6 +316,9 @@ ffff 0018 00002320 0031 05dc 000000010004000000000000 > # actions=check_pkt_larger(1000)->NXM_NX_XXREG1[4] > ffff 0018 00002320 0031 03e8 00040001e010000000000000 > > +# actions=delete_field:tun_metadata10 > +ffff 0018 00002320 0032 00 01 64 7c 00 00 00 00 000000000000 > + > ]) > sed '/^[[#&]]/d' < test-data > input.txt > sed -n 's/^# //p; /^$/p' < test-data > expout > diff --git a/tests/tunnel.at b/tests/tunnel.at > index d65bf4412aa9..ae38ca92c689 100644 > --- a/tests/tunnel.at > +++ b/tests/tunnel.at > @@ -890,6 +890,36 @@ Datapath actions: > set(tunnel(dst=1.1.1.1,ttl=64,tp_dst=6081,geneve({class=0xffff > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([tunnel - Delete Geneve option]) > +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=geneve \ > + options:remote_ip=1.1.1.1 ofport_request=1 \ > + -- add-port br0 p2 -- set Interface p2 type=dummy \ > + ofport_request=2 ofport_request=2]) > +OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP > + > +AT_CHECK([ovs-ofctl add-tlv-map br0 > "{class=0xffff,type=0,len=4}->tun_metadata0,{class=0xffff,type=1,len=4}->tun_metadata1"]) > + > +AT_DATA([flows.txt], [dnl > +table=0,tun_metadata0=0x11112222,actions=set_field:0x55556666->tun_metadata1,resubmit(,1) > +table=0,tun_metadata0=0x33334444,actions=delete_field:tun_metadata0,set_field:0x77778888->tun_metadata1,resubmit(,1) maybe also add a negative case? ex: actions=delete_field:tun_metadata30 (where 30 is not set) Thanks William > +table=1,actions=IN_PORT > +]) > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy > 'recirc_id(0),tunnel(tun_id=0x0,src=1.1.1.1,dst=1.1.1.2,ttl=64,geneve({class=0xffff,type=0,len=4,0x11112222}),flags(df|key)),in_port(6081),skb_mark(0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(frag=no)'], > [0], [stdout]) > +AT_CHECK([tail -2 stdout], [0], > + [Megaflow: > recirc_id=0,eth,ip,tun_id=0,tun_src=1.1.1.1,tun_dst=1.1.1.2,tun_tos=0,tun_flags=+df-csum+key,tun_metadata0=0x11112222,tun_metadata1=NP,in_port=1,nw_ecn=0,nw_frag=no > +Datapath actions: > set(tunnel(dst=1.1.1.1,ttl=64,tp_dst=6081,geneve({class=0xffff,type=0,len=4,0x11112222}{class=0xffff,type=0x1,len=4,0x55556666}),flags(df))),6081 > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy > 'recirc_id(0),tunnel(tun_id=0x0,src=1.1.1.1,dst=1.1.1.2,ttl=64,geneve({class=0xffff,type=0,len=4,0x33334444}),flags(df|key)),in_port(6081),skb_mark(0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(frag=no)'], > [0], [stdout]) > +AT_CHECK([tail -2 stdout], [0], > + [Megaflow: > recirc_id=0,eth,ip,tun_id=0,tun_src=1.1.1.1,tun_dst=1.1.1.2,tun_tos=0,tun_flags=+df-csum+key,tun_metadata0=0x33334444,tun_metadata1=NP,in_port=1,nw_ecn=0,nw_frag=no > +Datapath actions: > set(tunnel(dst=1.1.1.1,ttl=64,tp_dst=6081,geneve({class=0xffff,type=0x1,len=4,0x77778888}),flags(df))),6081 > +]) > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([tunnel - concomitant IPv6 and IPv4 tunnels]) > OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \ > options:remote_ip=1.1.1.1 ofport_request=1 \ > -- > 2.7.4 > > _______________________________________________ > 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
