[ovs-dev] [PATCH V14 1/4] Flow and action changes for 802.1AD

[Ben Pfaff]

On Mon, Nov 09, 2015 at 08:30:19PM -0800, Thomas F Herbert wrote:
> 
> 
> On 11/9/15 12:58 PM, Ben Pfaff wrote:
> >On Fri, Oct 02, 2015 at 05:31:58PM -0400, Thomas F Herbert wrote:
> >>From: "Thomas F. Herbert" <thomasfherbert at gmail.com>
> >>
> >>ToDo: The flow structure needs to be updated to hold both the inner and outer
> >>tpid since these are encoded by the kernel module.
> >I think I'd like to see the version with both TPIDs encoded.  That's the
> >way I've always envisioned this working.
> From an architectural view, it sounds indeed quite sensible. Also, it would
> get rid of the vlan depth calculation in the above code which is working but
> ugly. The $10 question is: are you implying that we should change the ABI by
> adding new OVS_KEY_ATTRIBs for the two TPIDs? This would raise backward
> compatibility and ABI issues.

I think that we covered this in-person at ovscon last week, but just in
case...  I don't think that this requires a kernel ABI change, because
the kernel ABI has always included the TPID.  I'm only suggesting that
"struct flow" consistently include the TPID; struct flow always goes
through some kind of translation whenever it passes out of OVS
userspace, so that shouldn't in itself break anything.

> >Also, once we have the TPIDs in place in struct flow, I am not sure
> >whether it is worthwhile to keep adding the VLAN_CFI bit to the TCI
> >value to indicate that a header is present.
> I assume you are suggesting that we determining a VLAN is present by
> checking eth_type_vlan(). With this we could allow any value for the VLAN
> including a 1 in the DEI bit. Maybe this should be a separate patch to
> eliminate the redundancy in openvswitch once the above change was made.

Maybe an example would help.  Suppose we define:
        struct flow_vlan {
            ovs_be32 tpid;
            ovs_be32 tci;
        };

and then in struct flow replace vlan_tci by "struct flow_vlan
vlans[2];".  (I'm not saying this is the way to go but it's a
possibility.)

Then, we know that there's at least one VLAN in 'flow' if
'flow->vlans[0].tpid != 0' and that there are two VLANs in 'flow' if
'flow->vlans[1].tpid != 0' (presumably they'd be left-justified so you
wouldn't get just the latter).