[ovs-dev] [PATCH 01/16] User-Space MPLS actions and matches

[Simon Horman]

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.