[ovs-dev] [PATCH v1 4/4] openvswitch: Userspace tunneling.

[Kyle Mestery]

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