On Fri, Jul 15, 2016 at 11:00 PM, Thomas F Herbert <thomasfherb...@gmail.com> wrote: > On 7/15/16 9:08 AM, Xiao Liang wrote: >> >> Thanks a lot. Please see inline. >> >> On Fri, Jul 15, 2016 at 7:17 PM, Thomas F Herbert >> <thomasfherb...@gmail.com> wrote: >>> >>> On 7/2/16 10:17 AM, Xiao Liang wrote: >>>> >>>> Hi Tom, >>>> >>>> Thanks for your test and comments! Please see inline. >>>> >>>> On Sat, Jul 2, 2016 at 1:34 AM, Thomas F Herbert >>>> <thomasfherb...@gmail.com> wrote: >>>>> >>>>> Xiao, >>>>> >>>>> Thanks. I think this is nice work and I think it advances the ball. >>>>> >>>>> It needs to be rebased to latest. I got some fuzz applying the patch. >>>>> It fails make check on latest master on a few tests: 403, 405, 1091, >>>>> 1117, >>>>> 1119 and 2092. >>>> >>>> A primary problem is that I forgot to modify odp_flow_from_string, >>>> which causes flow parse not working in tests. I'm checking if there >>>> are other bugs and try to fix them. >>>> >>>>> I tested the patch with my kernel module patch and it works. I am >>>>> passing >>>>> double tagged traffic through your patch now. >>>>> I like your approach of using an array to encode the vlans in the flow. >>>>> It seems to fail appctl trace. See http://pastebin.com/fn4AKL7q I think >>>>> when >>>>> it parses the packet representation, it is confusing the dl_type with >>>>> the >>>>> outer TPID. >>>> >>>> I think the dl_type resembles eth_type in OpenFlow, which defined as >>>> "ethernet type of the OpenFlow packet payload, after VLAN tags". In >>>> implementation, it should be the innermost ethertype that the switch >>>> can recognise. Suppose we support two VLAN headers, an IPv4 packet >>>> with one or two VLAN headers will have dl_type 0x0800, and an IPv4 >>>> packet with more than two VLAN headers will have dl_type 0x8100 or >>>> 0x88a8. Seems we can't match outer TPIDs with OpenFlow. >>> >>> I believe that the OF spec says the match should always be on the outer >>> vlan. Currently OVS matches the inner vlan only because of short cuts in >>> the >>> code. >> >> Above I meant ethertype but not VLAN TCI. Further explained in next >> mail about the kernel patch. >> >>>> The flow probably should be: >>>> >>>> >>>> in_port=2,vlan_tci=0x1064,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00 >>>> >>>> >>>> in_port=1,vlan_tci=0x13e7,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00 >>>> The dl_type doesn't matter in this case. >>> >>> Ben Pfaff and I had a discussion at OVSCON 2015 about getting rid of the >>> implied vlan detection on the CFI bit and moving to always detecting >>> vlans >>> by TPID. This would allow vlan with a 0 tci. I am sure he will weigh in >>> on >>> this. >>> >> I agree with you. But the the field name should be something else, >> like "tpid", rather than dl_type. With dl_type being the innermost >> ethertype, we could test if an packet (no matter tagged or untagged) >> is IPv4 by simply matching on this field. >> BTW, I see in the pastebin, you are testing with vlan_tci=0x0000, but >> there are only flows matching vlan 100 and 999. >> >>>>> I have a few other comments inline below. >>>>> It may also need some cleanup here and there and there may be some >>>>> white >>>>> space introduced by patch. >>>>> I didn't fully understand the double tagged tunnel type and didn't >>>>> immediately have a way to test that. >>>>> >>>>> --Tom >>>>> >>>>>> Add new port VLAN mode "dot1q-tunnel": >>>>>> - Example: >>>>>> ovs-vsctl set Port p1 vlan_mode=dot1q-tunnel tag=100 >>>>> >>>>> Don't you mean dot1ad tunnel, below you show cvlans? >>>> >>>> Here I followed naming convention of the Cisco CLI "switchport mode >>>> dot1q-tunnel". By Q-in-Q it means dot1Q-in-dot1Q, and it's simply two >>>> 802.1q headers in a 802.1ad frame (see >>>> https://en.wikipedia.org/wiki/IEEE_802.1ad). I don't have strong >>>> preference for the name. It can be changed if it's too confusing. >>>> >>>>>> + struct vlan_hdr vlan[FLOW_MAX_VLAN_HEADERS]; /* 802.1q VLAN >>>>>> headers */ >>>>> >>>>> Proper terminology would be just VLAN headers. When both vlans are >>>>> present >>>>> that is 802.1ad not 802.1q. >>>> >>>> As explained above. I will remove the "802.1q" because it's confusing. >>>> >>>>> I think encoding the vlans as an array is a good idea. >>>>> >>>>> It is interesting that you put the vlan headers before the dl_type in >>>>> the >>>>> order >>>>> they appear in the packet. I am concerned that the inner dl_type is not >>>>> in >>>>> the same 64 bit word as the vlan_tci in the case of single vlans. I >>>>> have >>>>> more commentary on this below. >>>>> >>>> Please see below. >>>> >>>>>> --- a/lib/flow.c >>>>>> +++ b/lib/flow.c >>>>>> @@ -72,8 +72,6 @@ const uint8_t flow_segment_u64s[4] = { >>>>>> /* miniflow_extract() assumes the following to be true to >>>>>> optimize >>>>>> the >>>>>> * extraction process. */ >>>>>> -ASSERT_SEQUENTIAL_SAME_WORD(dl_type, vlan_tci); >>>>>> - >>>>> >>>>> This bothers me. There is legacy behavior that does many matches on >>>>> Ethertype+tci. I struggled with this as well in my version of the patch >>>>> which encoded both TPIDs. I am concerned that this patch to double >>>>> tagged >>>>> vlans may substantially decrease performance of legacy single tagged >>>>> vlans. >>>>> In my latest patch, when I encoded a single vlan I put it in the inner >>>>> position in flow even though that was contradictory to the idea that >>>>> the >>>>> flow fields should be in order that they were found in the packet. Even >>>>> though you use an array for encoding the vlans, they both are always >>>>> encoded >>>>> in the flow. We have to think about where the single vlan should be >>>>> encoded. >>>> >>>> This is my understanding (not very sure, please correct me if wrong): >>>> The performance optimization is only relevant to miniflow_extract and >>>> expand. This assertion is used by miniflow_extract so that it can push >>>> dl_type and vlan_tci sequentially without any padding. There's no >>>> problem with functionality as long as flow and miniflow structures are >>>> in sync (see the miniflow_pad_to_64 added). There is performance >>>> impact because more data are pushed into miniflow, but it shouldn't be >>>> too much. >>> >>> Well, we don't know about the performance impact. There could be >>> significant >>> impact to DPDK netdev. When matches fail in the EMC, they go to the >>> miniflow. This could cause the layer 2 flow to drop out of cache. It is >>> hard >>> to verify this without testing. The SAME_WORD test was done to protect >>> against this. >>> >> I don't have DPDK testbed. Do you think putting dl_type and outer >> vlan_tci in the same 64-bit word will improve matching performance? I >> can modify the patch if someone is willing to test. > > I think Eric has plans to implement and test with DPDK. I want to raise the > issue so everyone is thinking about it. >
That's great! _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
