[ovs-dev] [PATCH v3 1/2] OF support and translation of generic encap and decap
Ben Pfaff
blp at ovn.org
Mon Jul 31 15:43:11 UTC 2017
On Fri, Jul 21, 2017 at 07:08:25PM +0200, Zoltán Balogh wrote:
> From: Jan Scheurich <jan.scheurich at ericsson.com>
>
> This commit adds support for the OpenFlow actions generic encap
> and decap (as specified in ONF EXT-382) to the OVS control plane.
>
> CLI syntax for encap action with properties:
> encap(hdr=<header>)
> encap(hdr=<header>,
> prop(class=<class>,type=<type>,val=<simple>),
> prop(class=<class>,type=<type>,val(<complex>)))
>
> CLI syntax for decap action:
> decap()
> decap(packet_type(ns=<pt_ns>,type=<pt_type>))
>
> The first header supported for encap and decap is "ethernet" to convert
> packets between packet_type (1,Ethertype) and (0,0).
>
> This commit also implements a skeleton for the translation of generic
> encap and decap actions in ofproto-dpif and adds support to encap and
> decap an Ethernet header.
>
> In general translation of encap commits pending actions and then rewrites
> struct flow in accordance with the new packet type and header. In the
> case of encap(ethernet) it suffices to change the packet type from
> (1, Ethertype) to (0,0) and set the dl_type accordingly. A new
> pending_encap flag in xlate ctx is set to mark that an corresponding
> datapath encap action must be triggered at the next commit. In the
> case of encap(ethernet) ofproto generetas a push_eth action.
>
> The general case for translation of decap() is to emit a datapath action
> to decap the current outermost header and then recirculate the packet
> to reparse the inner headers. In the special case of an Ethernet packet,
> decap() just changes the packet type from (0,0) to (1, dl_type) without
> a need to recirculate. The emission of the pop_eth action for the
> datapath is postponed to the next commit.
>
> Hence encap(ethernet) and decap() on an Ethernet packet are OF octions
> that only incur a cost in the dataplane when a modifed packet is
> actually committed, e.g. because it is sent out. They can freely be
> used for normalizing the packet type in the OF pipeline without
> degrading performance.
>
> Signed-off-by: Jan Scheurich <jan.scheurich at ericsson.com>
> Signed-off-by: Yi Yang <yi.y.yang at intel.com>
> Signed-off-by: Zoltan Balogh <zoltan.balogh at ericsson.com>
> Co-authored-by: Zoltan Balogh <zoltan.balogh at ericsson.com>
Thanks for working on this. I think that it will be a valuable feature.
First, I'm having a really hard time figuring out what we actually mean
when we say EXT-382. The ONF repo has 8 branches with 382 in the name:
remotes/extensibility/ext/mszczesn/ext-382
remotes/extensibility/spec/benmc/EXT-382-566
remotes/extensibility/spec/benmc/ext-382
remotes/extensibility/spec/benmc/ext-382-112
remotes/extensibility/spec/jean/ext-382
remotes/extensibility/spec/jean/ext-382-112
remotes/extensibility/spec/jean/ext-382-112-mark-ben
remotes/extensibility/spec/mszczesn/ext-382-112
The EXT-382 ticket in the ONF JIRA has over 20 attachments. All of them
are over a year old.
There doesn't seem to be anything that's actually going to be released
as EXT-382, nor is any progress being made toward OpenFlow 1.6, nor was
EXT-382 incorporated into OpenFlow before progress stopped. I think
that we need to be more specific about what spec we're implementing
here. Possibly, we need to document in detail what OVS is actually
implementing, since it's hard for users to find the spec.
Given that the spec isn't clear, we need to be really clear in the
source code. One place we aren't is padding. The comment on struct
ofp_ed_prop_header says that "These must be padded to a multiple of 4
bytes." but decode_ed_prop(), format_ed_props(), and probably other code
actually uses multiples of 8 bytes.
I don't see anything in decode_ofp_prop() or its caller that checks that
the property fits in the available space. It looks like an integer
overflow error.
I still get a pair of alignment errors (use ALIGNED_CAST, please):
../lib/ofp-ed-props.c:42:17: error: cast from 'char *' to 'const struct ofp_ed_prop_header *' increases required alignment from 1 to 2 [-Werror,-Wcast-align]
../lib/ofp-ed-props.c:58:13: error: cast from 'char *' to 'const struct ofpact_ed_prop *' increases required alignment from 1 to 2 [-Werror,-Wcast-align]
In struct ofpact_decap, thanks for the detailed comments. For long
comments on individual members, please put them above the member instead
of next to it, because it is easier to read that way. Also, we don't
use the abbreviation "OvS": please say "OVS" instead.
Similar feedback for the long comment in enum ofp_ed_prop_class.
It's not clear what OFPACT_ENCAP_MAX_PROP_SIZE is for. It's not used
anywhere.
The new actions need to be documented in the ovs-ofctl manpage.
The string format of the encap actions seems rather user unfriendly. Is
there a good reason why it should be the very generic
prop(class=<class>,type=<type>,val=<val>) rather than something more
tailored to the actual properties that users will want to set? It is
hard to tell without any actual examples.
In decode_NXAST_RAW_ENCAP() and parse_ENCAP(), shouldn't there be
validation of the header size value? Most header types will only have a
few acceptable header sizes.
The internal representation of ofpact_ed_prop, has no examples so far.
How confident are you that it actually needs to be variable-length? If
it does, then using a member 'n_props' in struct ofpact_encap to count
the number of properties seems risky: it implicitly encourages
programmers to try to index the props[] array from 0 to n_props-1. I'd
encourage, in that case, switching to a length member, or just
eliminating the member if ofpact_ed_props can be padded to 8-byte
multiples. But it's easier if we can just make ofpact_ed_prop
fixed-length for now; then we can just index props[] as an array.
However, I question whether the properties actually need to be
internally represented as properties at all. Presumably, OVS is only
going to support a specific set of properties. Probably, it's easy to
just add specific members to struct ofpact_encap that represent the
values of those properties. Did you consider that? That approach
usually simplifies code greatly; properties are usually needed only for
external representations.
I suspect that decode_NXAST_RAW_DECAP() should report an error if
properties are present, since it doesn't support properties.
In encode_DECAP(), please use sizeof instead of hard-coding a size of
16.
I can't find anything in the EXT-382 draft that talks about where encap
and decap go in the action set execution order. How did you decide?
The description of the action set execution order in ovs-ofctl(8) should
be updated (mentioning that encap and decap are nonstandard).
Thanks again, and I'll look forward to the next version.
I'm appending an incremental that implements a few of the more stylistic
requests above plus a few other minor things I noticed.
--8<--------------------------cut here-------------------------->8--
diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
index 7b9f6c199968..1bab42976482 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -976,9 +976,7 @@ struct ofpact_unroll_xlate {
/* OFPACT_ENCAP.
*
- * Used for OFPAT_ENCAP. */
-
-#define OFPACT_ENCAP_MAX_PROP_SIZE 256
+ * Used for NXAST_ENCAP. */
struct ofpact_encap {
struct ofpact ofpact;
@@ -990,14 +988,16 @@ struct ofpact_encap {
/* OFPACT_DECAP.
*
- * Used for OFPAT_DECAP. */
+ * Used for NXAST_DECAP. */
struct ofpact_decap {
struct ofpact ofpact;
- ovs_be32 new_pkt_type; /* New packet type. The special value
- (0,0xFFFE) "Use next proto" is used to
- request OvS to automatically set the
- new packet type based on the decap'ed
- header's next protocol.*/
+
+ /* New packet type.
+ *
+ * The special value (0,0xFFFE) "Use next proto" is used to request OVS to
+ * automatically set the new packet type based on the decap'ed header's
+ * next protocol. */
+ ovs_be32 new_pkt_type;
};
/* Converting OpenFlow to ofpacts. */
diff --git a/include/openvswitch/ofp-ed-props.h b/include/openvswitch/ofp-ed-props.h
index 1db7e56dfa4b..d45aa51a71a9 100644
--- a/include/openvswitch/ofp-ed-props.h
+++ b/include/openvswitch/ofp-ed-props.h
@@ -23,21 +23,21 @@
enum ofp_ed_prop_class {
OFPPPC_BASIC = 0, /* ONF Basic class. */
- OFPPPC_MPLS = 1, /* MPLS property class. */
- OFPPPC_GRE = 2, /* GRE property class. */
- OFPPPC_GTP = 3, /* GTP property class. */
- /* new values go here */
- OFPPPC_EXPERIMENTER=0xffff, /* Experimenter property class.
- * First 32 bits of property data is exp
- * id after that is the experimenter
- * property data. */
+ OFPPPC_MPLS = 1, /* MPLS property class. */
+ OFPPPC_GRE = 2, /* GRE property class. */
+ OFPPPC_GTP = 3, /* GTP property class. */
+
+ /* Experimenter property class.
+ *
+ * First 32 bits of property data is experimenter ID, after that is the
+ * experimenter property data. */
+ OFPPPC_EXPERIMENTER = 0xffff,
};
/*
* External representation of encap/decap properties.
* These must be padded to a multiple of 4 bytes.
*/
-
struct ofp_ed_prop_header {
ovs_be16 prop_class;
uint8_t type;
@@ -47,7 +47,6 @@ struct ofp_ed_prop_header {
/*
* Internal representation of encap/decap properties
*/
-
struct ofpact_ed_prop {
uint16_t prop_class;
uint8_t type;
diff --git a/lib/odp-util.c b/lib/odp-util.c
index a9a99486a9f1..84ca8ed6e04f 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6339,8 +6339,7 @@ commit_packet_type_change(const struct flow *flow,
switch (ntohl(flow->packet_type)) {
case PT_ETH: {
/* push_eth */
- odp_put_push_eth_action(odp_actions, &flow->dl_src,
- &flow->dl_dst);
+ odp_put_push_eth_action(odp_actions, &flow->dl_src, &flow->dl_dst);
base_flow->packet_type = flow->packet_type;
base_flow->dl_src = flow->dl_src;
base_flow->dl_dst = flow->dl_dst;
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index e3e84100e0f2..94f2d0ba760b 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -4028,9 +4028,9 @@ format_FIN_TIMEOUT(const struct ofpact_fin_timeout *a,
ds_put_format(s, "%s)%s", colors.paren, colors.end);
}
-/* Action structure for OFPAT_ENCAP */
+/* Action structure for NXAST_ENCAP */
struct nx_action_encap {
- ovs_be16 type; /* OFPAT_ENCAP */
+ ovs_be16 type; /* OFPAT_VENDOR. */
ovs_be16 len; /* Total size including any property TLVs. */
ovs_be32 vendor; /* NX_VENDOR_ID. */
ovs_be16 subtype; /* NXAST_ENCAP. */
@@ -4235,9 +4235,9 @@ format_ENCAP(const struct ofpact_encap *a,
ds_put_format(s, "%s)%s", colors.paren, colors.end);
}
-/* Action structure for OFPAT_DECAP */
+/* Action structure for NXAST_DECAP */
struct nx_action_decap {
- ovs_be16 type; /* OFPAT_DECAP */
+ ovs_be16 type; /* OFPAT_VENDOR */
ovs_be16 len; /* Total size including any property TLVs. */
ovs_be32 vendor; /* NX_VENDOR_ID. */
ovs_be16 subtype; /* NXAST_DECAP. */
diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c
index d2d91bebfb9a..3b9f5b923605 100644
--- a/lib/ofp-ed-props.c
+++ b/lib/ofp-ed-props.c
@@ -39,8 +39,8 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop,
}
*remaining -= pad_len;
- *ofp_prop = (const struct ofp_ed_prop_header *)
- ((char *)(*ofp_prop) + pad_len);
+ *ofp_prop = ALIGNED_CAST(const struct ofp_ed_prop_header *,
+ (char *) *ofp_prop + pad_len);
return 0;
}
@@ -55,7 +55,8 @@ encode_ed_prop(const struct ofpact_ed_prop **prop,
return OFPERR_OFPBAC_BAD_ARGUMENT;
}
- *prop = (const struct ofpact_ed_prop *) ((char *)(*prop) + prop_len);
+ *prop = ALIGNED_CAST(const struct ofpact_ed_prop *,
+ (char *) *prop + prop_len);
return 0;
}
More information about the dev
mailing list