| Message ID | 79BBBFE6CB6C9B488C1A45ACD284F51961C14DB3@SHSMSX103.ccr.corp.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Aug 01, 2017 at 12:04:55AM +0000, Yang, Yi Y wrote: > Hi, Ben > > Thank you so much for your great review and great comments, I'll do changes per your comments and post next version because Zoltan and Jan are taking vacation. I explained your concerns > > [Ben] 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. > > [Yi] because encap is very generic action, you can add a new header/encap type and some new properties for it very easily by this kind of very generic abstraction and mustn't change generic encap implementation. I make an example, for NSH we have > > prop(class=nsh,type=md_type,val=1) > > If we add new encap/header type gtpu, we can have the below property for GTP-u > > prop(class=gtpu,type=teid,val=1234) > > Generic encap action can accept this kind of new prop without any change against generic encap action implementation, this is the magic of very generic " prop(class=<class>,type=<type>,val=<val>)". To me, this argument would make sense if it meant that we could add gtpu without having to modify OVS. But we'll still need to do that. At the very least, OVS isn't going to know class "gtpu" or type "teid" means without modifications. Here's a more user friendly syntax that seems just as generic to me: class(type=value) e.g.: nsh(md_type=1) gtpu(teid=1234) > [Ben] 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. > > [Yi] Actually header_size isn't used at all now, it is reserved for later enhancement. For NSH MD type 2, NSH header is length-variable, ranged from 8 to 264, number of props and size of each prop are also very flexible (like Geneve tunnel metadata) OK. > [Ben] 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. > > [Yi] As I said above, prop is length-variable and unpredictable, especially for NSH MD type 2, so a length-fixed ofpact_ed_prop impracticable and will waste too much space, n_props can indicate number of props, actually props is a pointer, we can use > > uint8 props[] > > to replace > > struct ofp_ed_prop_header props[]; > > But I think it doesn't bring any substantial benefit from my perspective. > > ofpact_ed_prop is header of each peoperty, it just helps parse properties better. > > In addition, code is completely controllable, abuse of props[index] won't happen because you guys check every commit very carefully :-) OK. I still think this seems to add more complexity than is probably needed, but I'll leave it alone for now.
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; }