Ben,
Thanks for your thoughtful comments. Apologies for my late reply...
Dave.
On Fri, Oct 10, 2014 at 6:43 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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.
>
Agreed. I considered making that change for VLAN presence, but thought my
first patch shouldn't over-reach.
I think that match_format() should include the tpid when it is
> relevant.
>
> Certainly, but we can't yet match on the tpid...
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.
Definately Agree. I saw that the OF spec has the same problem. They
define the eth_type as 'Ethernet type of the OpenFlow packet payload, after
VLAN tags.' So there is nothing currently to match on VLAN tpid. Does OVS
go it's own way in a case like this, or discuss this with the OF crowd?
> 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).
>
> No, I did not address normal action.
> In flow_get_vlan_tpid(), this:
> if (!flow->vlan_tci & htons(VLAN_CFI)) {
> should likely be:
> if (!(flow->vlan_tci & htons(VLAN_CFI))) {
>
> Sure.
> 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?
>
> I used a fn so I could provide a safe value if it wasn't set properly and
had debug logs in there originally. I was concerned that I could
adequately test all cases. For example, older OF used to simply do
mod_vlan_vid and not do a push_vlan.
> odp_execute_actions() should use the TPID provided in the push_vlan
> action, e.g. 'vlan->vlan_tpid'.
>
> Yes.
> 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);
>
> Yes.
> 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.
>
> Hadn't considered that datapath mismatch could occur. I'll look into it.
> 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;
> };
>
> OF implied that all three would use the same struct. But MPLS has its own.
I'll remove the comment.
> I did not review the datapath changes.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
