On Wed, Jan 16, 2013 at 05:38:08PM +0900, Simon Horman wrote: > On Tue, Jan 15, 2013 at 11:05:45PM -0800, Ben Pfaff wrote: > > On Tue, Jan 08, 2013 at 02:46:02PM +0900, Simon Horman wrote: > > > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h > > > index 5e32965..b421753 100644 > > > --- a/include/linux/openvswitch.h > > > +++ b/include/linux/openvswitch.h > > > @@ -282,6 +282,7 @@ enum ovs_key_attr { > > > OVS_KEY_ATTR_ARP, /* struct ovs_key_arp */ > > > OVS_KEY_ATTR_ND, /* struct ovs_key_nd */ > > > OVS_KEY_ATTR_SKB_MARK, /* u32 skb mark */ > > > + OVS_KEY_ATTR_MPLS, /* struct ovs_key_mpls */ > > > OVS_KEY_ATTR_IPV4_TUNNEL, /* struct ovs_key_ipv4_tunnel */ > > > OVS_KEY_ATTR_TUN_ID = 63, /* be64 tunnel ID */ > > > __OVS_KEY_ATTR_MAX > > > > I'm pretty sure we shouldn't be inserting OVS_KEY_ATTR_MPLS in front of > > another value that we're already aiming to get upstream. I'd suggest a > > value of 62. > > > > Jesse, does that sound reasonable to you? Another alternative would be > > to avoid modifying <linux/openvswitch.h> entirely, one way or another, > > until we have real kernel support, which would be a little extra work. > > I'll use 62 unless I hear otherwise from Jesse. > > > I'm not sure why we have OVS_ACTION_ATTR_SET_MPLS instead of using > > OVS_ACTION_SET to set OVS_KEY_ATTR_MPLS. > > I'll switch over to using OVS_ACTION_SET+OVS_KEY_ATTR_MPLS, > it seems to be in keeping with the existing code. > > > > > > +/* Action structure for NXAST_PUSH_VLAN/MPLS. */ > > > +struct nx_action_push { > > > + ovs_be16 type; /* OFPAT_PUSH_VLAN/MPLS. */ > > > > The above two comments are wrong, there's no NXAST_PUSH_VLAN. And the > > struct should be named nx_action_push_mpls, not more generically. (This > > must be a leftover from Ravi's initial patch that also added QinQ.) > > > > > + ovs_be16 len; /* Length is 8. */ > > > + ovs_be32 vendor; /* NX_VENDOR_ID. */ > > > + ovs_be16 subtype; /* NXAST_PUSH_MPLS. */ > > > + ovs_be16 ethertype; /* Ethertype */ > > > + uint8_t pad[4]; > > > +}; > > > +OFP_ASSERT(sizeof(struct nx_action_push) == 16); > > > > > @@ -1271,6 +1272,20 @@ dp_netdev_execute_actions(struct dp_netdev *dp, > > > eth_pop_vlan(packet); > > > break; > > > > > > + case OVS_ACTION_ATTR_PUSH_MPLS: { > > > + const struct ovs_action_push_mpls *mpls = nl_attr_get(a); > > > + push_mpls(packet, mpls->mpls_ethertype, mpls->mpls_label); > > > + break; > > > > The similar OVS_ACTION_ATTR_PUSH_VLAN case declares its variable at the > > outer block. Please make the code consistent one way or another on this > > score.
I will supply a patch to localise the vlan variable for OVS_ACTION_ATTR_PUSH_VLAN as this approach reduces clutter at the top of the loop and keeps variables close to where they are used. > > The breakdown of code between parse_mpls() and parse_remaining_mpls() > > seems odd. I'd just write one function. > > > > In mf_is_value_valid(), I think that the MFF_MPLS_TC and MFF_MPLS_BOS > > cases should check the u8 member, not be32. Same for > > mf_random_value(). Also htonl won't be needed for u8. > > > > Most of the meta-flow functions present the cases in the order label, > > tc, bos, but mf_set_value() uses a different order. Can you make it > > consistent? > > > > In enum mf_prereqs, it might be best to add an "L2.5" category for MPLS. > > > > This adds a double blank line in nx_put_raw(). (Oh the horror!) > > > > Also in nx_put_raw(), missing space after the comma here: > > + nxm_put_8(b,OXM_OF_MPLS_TC, mpls_lse_to_tc(flow->mpls_lse)); > > > > parse_l3_onward() has a ";;": > > + ovs_be16 dl_type = flow->dl_type;; > > > > parse_l3_onward() could use flow_innermost_dl_type() since that's what > > it's effectively calculating as 'dl_type' (maybe it should be > > 'inner_dl_type' or 'innermost_dl_type'). I'm not so sure about this. flow_innermost_dl_type(), in its current incantation, is used to obtain the innermost dl_type of a struct flow. In other words, it reads flow->encap_dl_type. On the other hand, parse_l3_onward() may write flow->encap_dl_type and never reads it. > > Is there any sense in handling cases in commit_mpls_action() where the > > mpls_depth has to change by more than 1? Should we log an error at > > least? I think that logging is a good approach. I have update the code as follows: if (flow->mpls_depth < base->mpls_depth) { if (base->mpls_depth - flow->mpls_depth > 1) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(10, 10); VLOG_WARN_RL(&rl, "Multiple mpls_pop actions reduced to " " a single mpls_pop action"); } nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_POP_MPLS, flow->dl_type); } else if (flow->mpls_depth > base->mpls_depth) { struct ovs_action_push_mpls *mpls; if (flow->mpls_depth - base->mpls_depth > 1) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(10, 10); VLOG_WARN_RL(&rl, "Multiple mpls_push actions reduced to " " a single mpls_push action"); } ... > > The comment on struct ofpact_push is getting ahead of ourselves, since > > we have an OFPACT_PUSH_VLAN but it doesn't use this structure and no one > > has even mentioned PBB for OVS yet, at least not to me: > > > > +/* OFPACT_PUSH_VLAN/MPLS/PBB > > + * > > + * used for NXAST_PUSH_MPLS, OFPAT13_PUSH_VLAN/MPLS/PBB */ > > +struct ofpact_push { > > + struct ofpact ofpact; > > + ovs_be16 ethertype; > > +}; Thanks. I'm not even sure that I know what PBB is. I will also rename ofpact_push as ofpact_push_mpls. > > > > You can remove the bit about "negotiation of an OF1.3 session is not yet > > supported" from this comment in ofputil_usable_protocols(): > > + /* NXM and OF1.3+ support matching MPLS label */ > > + /* Allow for OF1.2 as there doesn't seem to be a > > + * particularly good reason not to and negotiation > > + * of an OF1.3 session is not yet supported. */ > > > > In ofputil_normalize_match__() should we also mask off mpls_depth if > > !MAY_MPLS? I guess so. Yes, I think so. > > It's getting late here so I'll resume looking at this patch starting at > > lib/packets.c next chance I get. > > All the above seems reasonable. I've started working on making it so. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev