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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
