[ovs-dev] Initial VXLAN Support in OVS

[Kyle Mestery (kmestery)]

On Mar 30, 2012, at 8:55 PM, Jesse Gross wrote:
> On Thu, Mar 29, 2012 at 1:12 PM, Kyle Mestery (kmestery)
> <kmestery at 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 was thinking the same here, so this is good.

> 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?)
> 
This was the intention. I will update it to better reflect reality, however.

> 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:
> 
I will fix these things up as well before resubmitting this patch.

> /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.


For my next patch submission, will take care of both of the above.

Thanks for the review and the pointers on patch submission!

Kyle