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.
>
> 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').
>
> 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?
>
> 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;
> +};
>
> 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.
>
> 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
