On Sat, Apr 11, 2020 at 8:10 AM William Tu <[email protected]> wrote:
>
> 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
Thanks for William's review. I will address your comment and send out v2.
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
> > @@ -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
Yes, I will fix the alignment issue in v2.
> > +{
> > + 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?
>
There is 14 bytes of pad in struct nx_action_delete_field, and
put_NXAST_DELETE_FIELD() would allocate the whole structure with
padding in the buffer. Therefore, we move the data pointer in the
buffer back to put the OXM/NXM header in the correct position.
> > 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'?
Thanks, I will add the missing space back.
> > --- 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()?
>
In the non-UDPIF format, unset the present map indicating the
corresponding tun_metadata is removed (not present).
tun_metadata_del_entry() is used to delete a TLV mapping entry in the
tun_table, i. e. when using ovs-ofctl del-tlv-map .....
> > 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
Sure, I will add a negative case in v2.
Thanks for the review.
-Yi-Hung
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
