[ovs-dev] Initial VXLAN Support in OVS

Jesse Gross jesse at nicira.com
Sat Mar 31 01:55:19 UTC 2012


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



More information about the dev mailing list