On Fri, Oct 10, 2014 at 10:06:29AM -0400, Dave Benson wrote:
> Previous behavior forced the use of the 802.1Q Ethertype for all VLAN headers.
> In order to open the door for future support of VLAN stacking or Provider
> Based
> Bridging, support of the 802.1AD Ethertype is a logical first step.
>
> This patch adds support for OpenFlow flows to specify push_vlan actions which
> may now specify the 0x88a8 Ethertype. Userspace and fastpath forwarding are
> supported. The various mechanisms to dump flows correctly show the two
> possible Ethertypes.
>
> Bridging (aka normal mode) has not been addressed with this patch.
>
> Testing:
> 1 Added a test to ofproto-dpif.at.
> 2 Tested on Xubuntu 14.04 (kernel version 3.13). Created two OVS bridges with
> a physical ethernet between them. Attached VM clients and passed ping and
> iperf traffic between the VMs. Tested configurations using both possible
> VLAN Ethertypes. Verified correct packet formats with wireshark.
> Verified the various dump-flow commands (ofctl, dpif and dpctl) returned the
> correct information.
> 3 Prior to the rebase to latest, testing was also done on Xubuntu 13.10
> (kernel
> version 3.11).
>
> This work was done independantly, although I read with interest the patches
> from
> Thomas Herbert and Avinash Andrew and the related comments. I believe this
> work
> will be complementary to further efforts in this area.
Thank you for your contribution. I have some comments.
One direction that we will probably have to take at some point is to
allow matching and setting the DEI bit. 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?
odp_execute_actions() should use the TPID provided in the push_vlan
action, e.g. 'vlan->vlan_tpid'.
In odp_flow_key_from_flow__(), I imagine that the code here should use
the VLAN type from the flow instead:
/* If flow->dl_type is not already VLAN, then use typical VLAN eth
type */
if (!eth_type_vlan(flow->dl_type)) {
nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE,
htons(ETH_TYPE_VLAN));
} else {
nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, flow->dl_type);
}
which would look something like:
nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE,
flow->vlan_tpid ? flow->vlan_tpid : flow->dl_type);
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.
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.
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
