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

[Pravin Shelar]

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?

>> +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,
>                                                 ^^^^