Inline <rk>
-----Original Message-----
From: Ben Pfaff [mailto:b...@nicira.com]
Sent: Thursday, May 24, 2012 10:06 AM
To: Kerur, Ravi
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] MPLS and VLAN QinQ patch
I've fixed your quoting.
On Thu, May 24, 2012 at 06:38:58PM +0200, ravi.ke...@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?
<rk> I see there is a capability to match on nw_ttl so thought it would make
sense to add a match for mpls_ttl. When IP packets traverse MPLS tunnels, IP
TTL is copied into MPLS TTL and it is copied back into IP when they exit. Hence
thought of providing a match for that.
> > 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[]
<rk> Thanks for the information. I will work on it after 1.1 is ready.
> > 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.
<rk> I will add a comment referring to some data to be available after MPLS
header.
> > 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.
<rk> sure, will remove it.
> > 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?
<rk> I will change it to assert call.
Thanks,
Ravi
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
