On Thu, Mar 29, 2012 at 1:12 PM, Kyle Mestery (kmestery) <kmest...@cisco.com> wrote: > This patch builds on top of the VXLAN patch sent by Ben to the OVS list > from October 2011. It is not complete, but I'd like to get some initial > feedback on the approach here. Jesse and I had talked a few months back > on this, but I wanted to do this incrementally. This initial patch adds > tunnel header information to the flow key. It acquires this by having > the tunnel vports store this in ovs_skb_cb for now. Tunnel headers are > also allowed to be set from userspace. For now, this is all very VXLAN > specific, but other tunnel vports could move in this direction as well.
Thanks for working on this Kyle. > Feedback appreciated in it's current form. I think the main thing is that I'd like to see a single code path for tunneling in the kernel, rather than a mixture of flow based VXLAN, port-based VXLAN, and other tunnels. This would actually be mostly userspace work, since it would have to implement the port-based mechanisms in terms of flows. I'm not sure that I understand the intention of the set_tunnel action. I was expecting that it would follow the form of the existing set tun_id action (actually replace it) where the appropriate fields are stored in the skb and used when outputting to a tunnel port. It seems like currently it will modify the IP header of the inner packet, overwrite some payload to add the header, and then forward but without routing. Can you explain the intention here? (maybe modify tunnel packets in transit?) I also see some conflict markers in ovs-monitor-ipsec that look like they are from a failed merge. Finally, I ran sparse on this and it found some issues with types: /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1040:40: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1040:38: warning: incorrect type in assignment (different base types) /home/jesse/openvswitch/datapath/linux/flow.c:1040:38: expected restricted __be64 [usertype] tun_id /home/jesse/openvswitch/datapath/linux/flow.c:1040:38: got unsigned int /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast to restricted __be32 /home/jesse/openvswitch/datapath/linux/flow.c:1211:43: warning: cast from restricted __be64 /home/jesse/openvswitch/datapath/linux/flow.c:1211:41: warning: incorrect type in assignment (different base types) /home/jesse/openvswitch/datapath/linux/flow.c:1211:41: expected restricted __be64 [usertype] <noident> /home/jesse/openvswitch/datapath/linux/flow.c:1211:41: got unsigned int /home/jesse/openvswitch/datapath/linux/flow.c:1271:35: warning: incorrect type in assignment (different base types) /home/jesse/openvswitch/datapath/linux/flow.c:1271:35: expected restricted __be32 [usertype] ipv4_src /home/jesse/openvswitch/datapath/linux/flow.c:1271:35: got restricted __be16 const [usertype] src /home/jesse/openvswitch/datapath/linux/flow.c:1272:35: warning: incorrect type in assignment (different base types) /home/jesse/openvswitch/datapath/linux/flow.c:1272:35: expected restricted __be32 [usertype] ipv4_dst /home/jesse/openvswitch/datapath/linux/flow.c:1272:35: got restricted __be16 const [usertype] dst > BTW: I have a branch on my github for these changes, and I squashed > all my commits for the last few months into one, collapsed with Ben's, > to get the patch I've included below. Let me know if this is ok as well. The main goal is to have a patchset composed of individual logical changes. It's fine to use Ben's patch as a starting point (giving him credit, of course) but things should still be logically separated. In this case, there are changes to CAPWAP and other places that are infrastructure for this patch instead of actually related to VXLAN and so should be in separate patches. Also, would you mind sending these patches inline in email? It makes it easier to comment on the code directly. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
