I am late in commenting on Ben's comments on Dave's patch but I thought
I would add my two cents.
On 10/10/14, 6:43 PM, Ben Pfaff wrote:
On Fri, Oct 10, 2014 at 10:06:29AM -0400, Dave Benson wrote:
One direction that we will probably have to take at some point is to
allow matching and setting the DEI bit.
I think this will be near impossible at least for now at least in the
Linux datapath. The upstream kernel also uses the DEI (CFI) bit to
indicate VLAN present and uses it to trigger HW acceleration and vlan
offloading.
OVS may be wrong in OVS to use this bit but we are consistent with
upstream Linux 802.1q/ad implementation. Any way, my understanding is
that this bit is rarely used in real-world networks.
Until now this has been
somewhat difficult because of the special use we made of that bit (to
indicate that a VLAN was present). With a separate TPID member in the
flow, we can move away from that, making it easier to add DEI support.
I think that match_format() should include the tpid when it is
relevant.
With this change, I think that an OpenFlow flow table entry that
matches on a VLAN will now match both 0x88a8 and 0x8100 tags for that
VLAN. I doubt that that is desirable behavior, because my
understanding is that 0x88a8 and 0x8100 tags are generally in
different namespaces and are not interchangeable. We also need to
decide how to handle 0x88a8 tags in the OpenFlow normal action (I see
that you made a change in xlate_normal() in ofproto-dpif-xlate.c
already, but it does not seem to address that issue).
In flow_get_vlan_tpid(), this:
if (!flow->vlan_tci & htons(VLAN_CFI)) {
should likely be:
if (!(flow->vlan_tci & htons(VLAN_CFI))) {
Is flow_get_vlan_tpid() useful? That is, is there a way for an
invalid tpid value to get into a flow's vlan_tpid member?
In contrast to Dave's patch, my patch uses the inner and outer tci in
the flow. I only use the tpid to be consistent with the OF specification
but the tpid's of 8100 or 88a8 is inferred by whether it is double or
single tagged. I believe that this is consistent with the 802.1ad
specification. Although OF 1.1+ specifies that the TPID can be
specified as an argument to push vlan. It actually should only be used
to differentiate whether you are pushing on top of an already existing tag.
odp_execute_actions() should use the TPID provided in the push_vlan
action, e.g. 'vlan->vlan_tpid'.
My patch does this.
I think that commit_vlan_action() overlooks the possibility that the
TCI stays the same but the TPID changes. To handle that, I think that
you need to pop off the old VLAN header and then push on a new one.
My patch includes code to determine from the flow and the base flow
whether the attempt is to replace an existing tag, or push a new tag on
top of the existing tag. Until I figured out that this was necessary, I
found that the existing tests failed for single vlans.
For compatibility with old datapaths, userspace needs to be able to
detect whether the datapath supports non-0x8100 VLANs and fall back to
userspace processing for them if it doesn't.
What datapaths support non 0x8100 vlans? I am not sure what Dave did,
but I only tested my patch with 0x8100 and 0x88a8. I am not sure how to
test for this.
The new struct ofpact_push is used only for pushing VLANs. I don't
understand why the comment mentions MPLS and PBB:
/* OFPACT_PUSH_VLAN/MPLS/PBB (But MPLS has it's own version right now...)
*
* Used for OFPAT11_PUSH_VLAN */
struct ofpact_push {
struct ofpact ofpact;
ovs_be16 ethertype;
};
I did not review the datapath changes.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
--
Thomas F. Herbert
Network Implementation Engineer
Entry Point LLC
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
