[ovs-dev] MPLS and VLAN QinQ patch

Ben Pfaff blp at nicira.com
Thu May 24 17:05:59 UTC 2012


I've fixed your quoting.

On Thu, May 24, 2012 at 06:38:58PM +0200, Ravi.Kerur at telekom.com
wrote:
> Ben Pfaff writes:
> > OF1.3 adds the ability to match on the MPLS "BOS" bit.  Should we add that here too?
>
> My plan was to first get to 1.1 and later on to 1.2/1.3. I
> think it is good to have a reference code for 1.1 just in case there
> is a request for that. I haven't followed the discussion initiated
> by Zoltan on this, will look into it. Do you think match capability
> for "mpls ttl" should be added as well since it provides similar
> purpose as layer-3?

OK, I'm fine with deferring this.  It should only be a small
incremental change.

As for the TTL, I don't think that any OpenFlow version allows for
matching on an IP TTL, so there is no support-by-analogy for matching
on the MPLS TTL.  Is there another reason to support matching on the
MPLS TTL?

> > It occurs to me that we could drop the NXAST_SET_MPLS_LABEL and
> > NXAST_SET_MPLS_TC actions because these can be implemented through
> > "register load" actions.  OpenFlow 1.2 also dropped the similar
> > OFPAT_ actions, for the same reason, so maybe we shouldn't bother
> > with NXAST_ actions for them as they don't have any additional
> > benefit for OpenFlow 1.0 with Nicira extensions.
> 
> Same argument as above. In this case it isn't quite clear to me how
> set_mpls_label and set_mpls_tc be implemented via reg_load. Any
> pointers?

In ovs-ofctl syntax, the "load" actions would look like this:
        load:123->NXM_OF_MPLS_TC[]
        load:5->NXM_OF_MPLS_LABEL[]

> > I think that you can greatly simplify parse_remaining_mpls() to
> > just this (not tested):
> > 
> >     while (b->size >= sizeof(struct mpls_hdr)) {
> >         struct mpls_hdr *mh = ofpbuf_pull(b, sizeof *mh);
> >         if (mh->mpls_lse & htonl(MPLS_STACK_MASK)) {
> >             break;
> >         }
> >     }
> 
> I quite understand vlan implementation part, though mpls parsing
> implementation is similar to vlan, the reason I had it was for ovs
> to reject packets if there is nothing following mpls
> header. Initially I checked for ipv4/ipv6 header version in
> parse_mpls and later removed after discussion on mpls and non-IP
> packets. Though 2 bytes isn't enough for sanity check, I just left
> it as it is.

OK, as-is it looks like a mistake, could you do something to indicate
that it is not?  For example, if there's a real minimum MPLS payload
size, you could use that.  Or otherwise add a comment.

> > I don't think we should consider the MPLS label in
> > flow_hash_symmetric_l4(), because there's no assurance that the MPLS
> > labels would be the same in both directions.  (Bruce Davie tells me
> > that in fact that's unlikely.)
> 
> I don't have strong opinion either way.

Can we go with Bruce's opinion, then, and drop it?  Thanks.

> > In commit_mpls_push_action() and commit_mpls_pop_action(), I'm
> > surprised that we can possibly log errors at this late stage.
> > Shouldn't we catch these errors earlier?
> 
> checks are performed in validate_action as well.

OK, so we know at this point that it's correct?  Can we drop the
check, then, or switch it to an assertion? 



More information about the dev mailing list