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

[Joe Stringer <joestringernz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1401 bytes --]

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.

[-- Attachment #1.2: Type: text/html, Size: 1626 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]