Re: [PATCH v2.44 1/5] odp: Allow VLAN actions after MPLS actions

[Simon Horman <horms@verge.net.au>]

On Tue, Oct 22, 2013 at 01:55:41PM -0700, Ben Pfaff wrote:
> On Tue, Oct 22, 2013 at 11:30:26AM -0700, Joe Stringer wrote:
> > You're quite right. I think for OF1.2, this is similar to existing
> > behaviour, but for OF1.3 it's just incorrect. There is an additional issue
> > with the LOAD action.
> > 
> > OF1.2:
> > "push_vlan(A),push_mpls,push_vlan(B)"
> > In OF1.2, this has the same result as:-
> > "push_mpls,push_vlan(A),push_vlan(B)"
> > 
> > When translated, this boils down to "(pop_vlan,)push_mpls,push_vlan(B)".
> > Correct me if I am wrong, but I think this is similar to the existing
> > behaviour, as we currently only support one layer of VLAN. It doesn't
> > matter if A == B.
> > 
> > OF1.3:
> > "mod_vlan_vid:A,push_mpls:0x8847,mod_vlan_vid:A" should work, but the
> > second mod_vlan is being dropped as it has the same VID as the first. This
> > is incorrect, as you point out.
> > 
> > LOAD:
> > "load:A->OXM_OF_VLAN_VID,push_mpls:0x8847,load:A->OXM_OF_VLAN_VID" doesn't
> > result in the correct vlan_vid from the first load action. I'm not sure
> > that vlan_tci_restore() is clear or correct---I believe its original
> > purpose was to properly handle the case where MPLS changes are made from
> > REG_LOAD and friends, in the way that the PUSH_MPLS case works. Instead, it
> > is handling all cases where MPLS actions have been applied in the past,
> > whether the current action modifies MPLS or not.
> > 
> > It's probably also worthwhile to make use of the ovs-ofctl monitor "-m"
> > option in the tests to actually verify these, rather than the current tests
> > where we just check the size of the resulting packet.
> 
> Thanks for the careful analysis.

Indeed, thanks Joe, I'll fix this up.

The idea that I have at this time is to track the VLAN depth delta
in a similar way to what is done for MPLS. But it is not a particularly
well thought out idea at this stage.