On Fri, Oct 24, 2014 at 7:05 AM, Thomas Graf <tg...@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?
>> +b...@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@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
