[ovs-dev] [PATCH v1 4/4] openvswitch: Userspace tunneling.
Kyle Mestery
mestery at mestery.com
Fri Oct 24 19:41:35 UTC 2014
On Fri, Oct 24, 2014 at 2:23 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Fri, Oct 24, 2014 at 7:05 AM, Thomas Graf <tgraf at noironetworks.com> wrote:
>> First of all, great to see this work. This is awesome! I read through
>> the code a first time. Planning to do more reviewing but looks very
>> sane already.
>>
>> On 10/16/14 at 11:38am, Pravin B Shelar wrote:
>>> + 192.168.1.1/24
>>> + +--------------+
>>> + | int-br | 192.168.1.2/24
>>> + +--------------+ +--------------+
>>> + | vxlan0 | | vxlan0 |
>>> + +--------------+ +--------------+
>>> + | |
>>> + | |
>>> + | |
>>> + 172.168.1.1/24 |
>>> + +--------------+ |
>>> + | br-eth1 | 172.168.1.2/24
>>> + +--------------+ +---------------+
>>> + | eth1 |----------------------------------| eth1 |
>>> + +--------------+ +----------------
>>
>> Might be worth finding other names than int-br and br-eth1 due to
>> Neutron's usage of br-int and br-ethX to avoid confusion. I realize
>> that everybody is free to name their bridges but examples tend to get
>> used 1:1 ;-)
>>
>
> Does Neutron uses br-ethX for something else to cause confusion?
>
Neutron uses those for the physical bridge mappings when using the OVS
agent with the ML2 plugin [1] and VLANs.
[1] http://docs.openstack.org/havana/install-guide/install/apt/content/network-node-install-plugin-openvswitch-agent.html
>>> +bugs at openvswitch.org
>>> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
>>> index b2257e6..ff7c9d9 100644
>>> --- a/datapath/linux/compat/include/linux/openvswitch.h
>>> +++ b/datapath/linux/compat/include/linux/openvswitch.h
>>> @@ -580,6 +580,15 @@ struct ovs_action_hash {
>>> uint32_t hash_basis;
>>> };
>>>
>>> +#define TNL_PUSH_HEADER_SIZE 128
>>> +struct ovs_action_push_tnl {
>>> + uint32_t tnl_port;
>>> + uint32_t out_port;
>>> + uint32_t header_len;
>>> + uint32_t tnl_type; /* For logging. */
>>> + uint8_t header[TNL_PUSH_HEADER_SIZE];
>>> +};
>>
>> Any reason for this to live in the kernel header?
>>
> This is data for tnl-push action, such structures are defined in openvswitch.h.
>
>>> /**
>>> * enum ovs_action_attr - Action types.
>>> *
>>> @@ -633,6 +642,10 @@ enum ovs_action_attr {
>>> * data immediately followed by a mask.
>>> * The data must be zero for the unmasked
>>> * bits. */
>>> +#ifndef __KERNEL__
>>> + OVS_ACTION_ATTR_TUNNEL_PUSH,
>>> + OVS_ACTION_ATTR_TUNNEL_POP,
>>> +#endif
>>> __OVS_ACTION_ATTR_MAX
>>> };
>>
>> Should we remove the #ifndef so the action values are reserved in the
>> kernel datapath as well? We don't want conflicting actions types later on.
>>
> There is no plan for implementing this action for kernel datapath, so
> for now I do not want to reserve number for the action.
>
>>> +void
>>> +tnl_port_map_insert(odp_port_t port, ovs_be32 ip_dst, ovs_be16 udp_port,
>>> + const char dev_name[])
>>> +{
>>> + const struct cls_rule *cr;
>>> + struct tunnel_port *p;
>>> + struct match match;
>>> +
>>> + memset(&match, 0, sizeof match);
>>> +
>>> + match.flow.dl_type = htons(ETH_TYPE_IP);
>>> + if (udp_port) {
>>> + match.flow.nw_proto = IPPROTO_UDP;
>>> + } else {
>>> + match.flow.nw_proto = IPPROTO_GRE;
>>> + }
>>> +
>>> + match.flow.tp_dst = udp_port;
>>
>> Move this flow init code into a function for use by tnl_port_map_delete()?
>>
> ok.
>
> Thanks for all review.
> --Pravin.
>
>>
>>> +
>>> + /* When matching on incoming flow from remove tnl end point,
>> ^^^^
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list