On 11/25/15 12:54 AM, Ben Pfaff wrote:
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" <[email protected]>
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.
Yes, thanks for the comment. I will update patch to encode and decode
both tpid's.
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.
There was a separate comment by Pravin discussing eliminating the DEI.
Now that we are encoding tpid's explicitly we could check tpid ethertype
to determine if a vlan is present but probably that should wait for
another patch.
Maybe an example would help. Suppose we define:
struct flow_vlan {
ovs_be32 tpid;
ovs_be32 tci;
};
Yes, I did this in the kernel patch.
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).
Yes, this will make for cleaner code to calculate depth of encapsulation
vlan. I think that since vlans are never nested more then 2 deep, I
could encode it the same way I do in the kernel flow key with two
structs instead of an array of structs.
--
Thomas F Herbert Red Hat
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
