On Fri, Oct 24, 2014 at 2:23 PM, Pravin Shelar <pshe...@nicira.com> wrote: > 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? > 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 >>> +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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
