| Message ID | 1464862729-31563-2-git-send-email-cascardo@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Jun 02, 2016 at 07:18:48AM -0300, Thadeu Lima de Souza Cascardo wrote: > If VXLAN-GBP and VXLAN are set on the same port, only one of those tunnel types > is going to be created. As other tunnel flags are added, like VXLAN-GPE, and > they are incompatible, reject any configuration that put incompatible tunnels on > the same port. > > A manual test has been done as well and the port didn't get an ofport. During a > restart, the other tunnel type was rejected and the right flags were on the > dpif_port. > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> Self-NACK. I need to handle the case when one of the conflicting port is removed. Will send a v2. Cascardo.
On Thu, Jun 2, 2016 at 3:18 AM, Thadeu Lima de Souza Cascardo <cascardo@redhat.com> wrote: > If VXLAN-GBP and VXLAN are set on the same port, only one of those tunnel types > is going to be created. As other tunnel flags are added, like VXLAN-GPE, and > they are incompatible, reject any configuration that put incompatible tunnels on > the same port. > > A manual test has been done as well and the port didn't get an ofport. During a > restart, the other tunnel type was rejected and the right flags were on the > dpif_port. > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> I saw your follow up to yourself but I already started looking at this, so I might as well give the comments that I have. > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index 6016fd8..aaf6bfc 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -38,6 +38,7 @@ > #include "packets.h" > #include "poll-loop.h" > #include "route-table.h" > +#include "simap.h" > #include "smap.h" > #include "socket-util.h" > #include "unaligned.h" > @@ -63,6 +64,7 @@ static bool tunnel_check_status_change__(struct netdev_vport *); > struct vport_class { > const char *dpif_port; > struct netdev_class netdev_class; > + struct simap dst_port_to_exts; > }; I don't think it's really makes sense to have a per-class map. Across classes, I would always expect for dpif_ports to be unique but they might not for the same class on difference dpif backers (i.e. kernel vs. DPDK - though I'm not sure that we really handle this case very well right now). Plus I don't really like the need to pass in the array index of the vport_class when initializing these things. > +static bool > +netdev_vport_may_add(struct netdev *netdev) > +{ I would give this a name that is a little less generic and more specifically reflects what is it > + char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; > + const char *name; > + struct simap_node *node; > + struct vport_class *vclass; > + const struct netdev_vport *vport; > + const struct netdev_class *class = netdev_get_class(netdev); > + > + if (!is_vport_class(class)) { > + return false; > + } > + > + if (!netdev_vport_needs_dst_port(netdev)) { > + return true; > + } > + > + vport = netdev_vport_cast(netdev); > + vclass = vport_class_cast(class); > + name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf); > + > + node = simap_find(&vclass->dst_port_to_exts, name); > + if (!node) { > + simap_put(&vclass->dst_port_to_exts, name, vport->tnl_cfg.exts); When name is added to the simap, it is a reference to the one that is passed in - which in this case is just on the stack. > static int > set_tunnel_config(struct netdev *dev_, const struct smap *args) > { > @@ -615,6 +647,10 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args) > } > ovs_mutex_unlock(&dev->mutex); > > + if (!netdev_vport_may_add(dev_)) { > + return EEXIST; > + } Presumably this check should come before we update the configuration for existing tunnels.
On Thu, Jun 02, 2016 at 12:46:04PM -0700, Jesse Gross wrote: > On Thu, Jun 2, 2016 at 3:18 AM, Thadeu Lima de Souza Cascardo > <cascardo@redhat.com> wrote: > > If VXLAN-GBP and VXLAN are set on the same port, only one of those tunnel types > > is going to be created. As other tunnel flags are added, like VXLAN-GPE, and > > they are incompatible, reject any configuration that put incompatible tunnels on > > the same port. > > > > A manual test has been done as well and the port didn't get an ofport. During a > > restart, the other tunnel type was rejected and the right flags were on the > > dpif_port. > > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> > > I saw your follow up to yourself but I already started looking at > this, so I might as well give the comments that I have. Hi, Jesse. Thanks for the review and for applying the other patches. Some comments below. > > > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > > index 6016fd8..aaf6bfc 100644 > > --- a/lib/netdev-vport.c > > +++ b/lib/netdev-vport.c > > @@ -38,6 +38,7 @@ > > #include "packets.h" > > #include "poll-loop.h" > > #include "route-table.h" > > +#include "simap.h" > > #include "smap.h" > > #include "socket-util.h" > > #include "unaligned.h" > > @@ -63,6 +64,7 @@ static bool tunnel_check_status_change__(struct netdev_vport *); > > struct vport_class { > > const char *dpif_port; > > struct netdev_class netdev_class; > > + struct simap dst_port_to_exts; > > }; > > I don't think it's really makes sense to have a per-class map. Across > classes, I would always expect for dpif_ports to be unique but they > might not for the same class on difference dpif backers (i.e. kernel > vs. DPDK - though I'm not sure that we really handle this case very > well right now). Plus I don't really like the need to pass in the > array index of the vport_class when initializing these things. > I didn't like it either. It wasn't needed as I was calling simap_init and could use a simple {} initializer, bug GCC 4.8 didn't like it. Do you agree with using a single map for VXLAN, as it's the only one with a problem as of now? > > +static bool > > +netdev_vport_may_add(struct netdev *netdev) > > +{ > > I would give this a name that is a little less generic and more > specifically reflects what is it > Commenting seems an important thing as well. I will think of something. > > + char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; > > + const char *name; > > + struct simap_node *node; > > + struct vport_class *vclass; > > + const struct netdev_vport *vport; > > + const struct netdev_class *class = netdev_get_class(netdev); > > + > > + if (!is_vport_class(class)) { > > + return false; > > + } > > + > > + if (!netdev_vport_needs_dst_port(netdev)) { > > + return true; > > + } > > + > > + vport = netdev_vport_cast(netdev); > > + vclass = vport_class_cast(class); > > + name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf); > > + > > + node = simap_find(&vclass->dst_port_to_exts, name); > > + if (!node) { > > + simap_put(&vclass->dst_port_to_exts, name, vport->tnl_cfg.exts); > > When name is added to the simap, it is a reference to the one that is > passed in - which in this case is just on the stack. > Oops. Not sure how that would have worked then. So, I looked at the code and simap_put calls xmemdup0, which makes sense since it can also be used for a replace. > > static int > > set_tunnel_config(struct netdev *dev_, const struct smap *args) > > { > > @@ -615,6 +647,10 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args) > > } > > ovs_mutex_unlock(&dev->mutex); > > > > + if (!netdev_vport_may_add(dev_)) { > > + return EEXIST; > > + } > > Presumably this check should come before we update the configuration > for existing tunnels. So right before overwriting the configuration. Or did you think of something more high level? Again, thanks for the review. Cascardo.
On Thu, Jun 2, 2016 at 3:47 PM, Thadeu Lima de Souza Cascardo <cascardo@redhat.com> wrote: > On Thu, Jun 02, 2016 at 12:46:04PM -0700, Jesse Gross wrote: >> On Thu, Jun 2, 2016 at 3:18 AM, Thadeu Lima de Souza Cascardo >> <cascardo@redhat.com> wrote: >> > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c >> > index 6016fd8..aaf6bfc 100644 >> > --- a/lib/netdev-vport.c >> > +++ b/lib/netdev-vport.c >> > @@ -38,6 +38,7 @@ >> > #include "packets.h" >> > #include "poll-loop.h" >> > #include "route-table.h" >> > +#include "simap.h" >> > #include "smap.h" >> > #include "socket-util.h" >> > #include "unaligned.h" >> > @@ -63,6 +64,7 @@ static bool tunnel_check_status_change__(struct netdev_vport *); >> > struct vport_class { >> > const char *dpif_port; >> > struct netdev_class netdev_class; >> > + struct simap dst_port_to_exts; >> > }; >> >> I don't think it's really makes sense to have a per-class map. Across >> classes, I would always expect for dpif_ports to be unique but they >> might not for the same class on difference dpif backers (i.e. kernel >> vs. DPDK - though I'm not sure that we really handle this case very >> well right now). Plus I don't really like the need to pass in the >> array index of the vport_class when initializing these things. >> > > I didn't like it either. It wasn't needed as I was calling simap_init and could > use a simple {} initializer, bug GCC 4.8 didn't like it. > > Do you agree with using a single map for VXLAN, as it's the only one with a > problem as of now? I don't think there is a need to specialize this for VXLAN since I would expect that the dpif_ports are already non overlapping for different classes. I think the biggest question is whether it is possible to have the same port name across different bridges. In theory this should be possible but it might not be in practice. >> > + char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; >> > + const char *name; >> > + struct simap_node *node; >> > + struct vport_class *vclass; >> > + const struct netdev_vport *vport; >> > + const struct netdev_class *class = netdev_get_class(netdev); >> > + >> > + if (!is_vport_class(class)) { >> > + return false; >> > + } >> > + >> > + if (!netdev_vport_needs_dst_port(netdev)) { >> > + return true; >> > + } >> > + >> > + vport = netdev_vport_cast(netdev); >> > + vclass = vport_class_cast(class); >> > + name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf); >> > + >> > + node = simap_find(&vclass->dst_port_to_exts, name); >> > + if (!node) { >> > + simap_put(&vclass->dst_port_to_exts, name, vport->tnl_cfg.exts); >> >> When name is added to the simap, it is a reference to the one that is >> passed in - which in this case is just on the stack. >> > > Oops. Not sure how that would have worked then. So, I looked at the code and > simap_put calls xmemdup0, which makes sense since it can also be used for a > replace. I think you're right - I misread the comment. Of course, there is a memory leak here related to the deletion of ports that you mentioned before. >> > static int >> > set_tunnel_config(struct netdev *dev_, const struct smap *args) >> > { >> > @@ -615,6 +647,10 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args) >> > } >> > ovs_mutex_unlock(&dev->mutex); >> > >> > + if (!netdev_vport_may_add(dev_)) { >> > + return EEXIST; >> > + } >> >> Presumably this check should come before we update the configuration >> for existing tunnels. > > So right before overwriting the configuration. Or did you think of something > more high level? I think just moving this above the configuration change should solve the problem.
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 6016fd8..aaf6bfc 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -38,6 +38,7 @@ #include "packets.h" #include "poll-loop.h" #include "route-table.h" +#include "simap.h" #include "smap.h" #include "socket-util.h" #include "unaligned.h" @@ -63,6 +64,7 @@ static bool tunnel_check_status_change__(struct netdev_vport *); struct vport_class { const char *dpif_port; struct netdev_class netdev_class; + struct simap dst_port_to_exts; }; bool @@ -71,7 +73,7 @@ netdev_vport_is_vport_class(const struct netdev_class *class) return is_vport_class(class); } -static const struct vport_class * +static struct vport_class * vport_class_cast(const struct netdev_class *class) { ovs_assert(is_vport_class(class)); @@ -404,6 +406,36 @@ parse_tunnel_ip(const char *value, bool accept_mcast, bool *flow, return 0; } +static bool +netdev_vport_may_add(struct netdev *netdev) +{ + char namebuf[NETDEV_VPORT_NAME_BUFSIZE]; + const char *name; + struct simap_node *node; + struct vport_class *vclass; + const struct netdev_vport *vport; + const struct netdev_class *class = netdev_get_class(netdev); + + if (!is_vport_class(class)) { + return false; + } + + if (!netdev_vport_needs_dst_port(netdev)) { + return true; + } + + vport = netdev_vport_cast(netdev); + vclass = vport_class_cast(class); + name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf); + + node = simap_find(&vclass->dst_port_to_exts, name); + if (!node) { + simap_put(&vclass->dst_port_to_exts, name, vport->tnl_cfg.exts); + return true; + } + return node->data == vport->tnl_cfg.exts; +} + static int set_tunnel_config(struct netdev *dev_, const struct smap *args) { @@ -615,6 +647,10 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args) } ovs_mutex_unlock(&dev->mutex); + if (!netdev_vport_may_add(dev_)) { + return EEXIST; + } + return 0; } @@ -883,33 +919,40 @@ get_stats(const struct netdev *netdev, struct netdev_stats *stats) NULL, /* rx_drain */ -#define TUNNEL_CLASS(NAME, DPIF_PORT, BUILD_HEADER, PUSH_HEADER, POP_HEADER) \ - { DPIF_PORT, \ - { NAME, false, \ - VPORT_FUNCTIONS(get_tunnel_config, \ - set_tunnel_config, \ - get_netdev_tunnel_config, \ - tunnel_get_status, \ - BUILD_HEADER, PUSH_HEADER, POP_HEADER) }} +#define TUNNEL_CLASS(TUNNEL, NAME, DPIF_PORT, BUILD_HEADER, PUSH_HEADER, POP_HEADER) \ + { DPIF_PORT, \ + { NAME, false, \ + VPORT_FUNCTIONS(get_tunnel_config, \ + set_tunnel_config, \ + get_netdev_tunnel_config, \ + tunnel_get_status, \ + BUILD_HEADER, PUSH_HEADER, POP_HEADER) }, \ + SIMAP_INITIALIZER(&(TUNNEL)->dst_port_to_exts) } void netdev_vport_tunnel_register(void) { /* The name of the dpif_port should be short enough to accomodate adding * a port number to the end if one is necessary. */ - static const struct vport_class vport_classes[] = { - TUNNEL_CLASS("geneve", "genev_sys", netdev_geneve_build_header, + static struct vport_class vport_classes[] = { + TUNNEL_CLASS(&(vport_classes[0]), "geneve", "genev_sys", + netdev_geneve_build_header, netdev_tnl_push_udp_header, netdev_geneve_pop_header), - TUNNEL_CLASS("gre", "gre_sys", netdev_gre_build_header, + TUNNEL_CLASS(&(vport_classes[1]), "gre", "gre_sys", + netdev_gre_build_header, netdev_gre_push_header, netdev_gre_pop_header), - TUNNEL_CLASS("ipsec_gre", "gre_sys", NULL, NULL, NULL), - TUNNEL_CLASS("vxlan", "vxlan_sys", netdev_vxlan_build_header, + TUNNEL_CLASS(&(vport_classes[2]), "ipsec_gre", "gre_sys", + NULL, NULL, NULL), + TUNNEL_CLASS(&(vport_classes[3]),"vxlan", "vxlan_sys", + netdev_vxlan_build_header, netdev_tnl_push_udp_header, netdev_vxlan_pop_header), - TUNNEL_CLASS("lisp", "lisp_sys", NULL, NULL, NULL), - TUNNEL_CLASS("stt", "stt_sys", NULL, NULL, NULL), + TUNNEL_CLASS(&(vport_classes[4]), "lisp", "lisp_sys", + NULL, NULL, NULL), + TUNNEL_CLASS(&(vport_classes[5]), "stt", "stt_sys", + NULL, NULL, NULL), }; static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; @@ -936,6 +979,7 @@ netdev_vport_patch_register(void) VPORT_FUNCTIONS(get_patch_config, set_patch_config, NULL, - NULL, NULL, NULL, NULL) }}; + NULL, NULL, NULL, NULL)}, + SIMAP_INITIALIZER(&(&patch_class)->dst_port_to_exts)}; netdev_register_provider(&patch_class.netdev_class); } diff --git a/tests/tunnel.at b/tests/tunnel.at index 7f82785..97af317 100644 --- a/tests/tunnel.at +++ b/tests/tunnel.at @@ -548,6 +548,35 @@ AT_CHECK([tail -1 stdout], [0], [Datapath actions: set(tunnel(tun_id=0x0,dst=1.1.1.1,ttl=64,flags(df|key))),4789 ]) +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([tunnel - concomitant incompatible tunnels on the same port]) +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \ + options:remote_ip=flow ofport_request=1]) + +AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan \ + options:remote_ip=flow options:exts=gbp ofport_request=2], [0], + [], [ovs-vsctl: Error detected while setting up 'p2'. See ovs-vswitchd log for details. +]) + +AT_CHECK([grep p2 ovs-vswitchd.log | sed "s/^.*\(p2:.*\)$/\1/"], [0], + [p2: could not set configuration (File exists) +]) + +OVS_VSWITCHD_STOP(["/could not set configuration/d"]) +AT_CLEANUP + +AT_SETUP([tunnel - concomitant incompatible tunnels on different ports]) +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \ + options:remote_ip=flow ofport_request=1]) + +AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan options:dst_port=9000 \ + options:remote_ip=flow options:exts=gbp ofport_request=2]) + +AT_CHECK([grep p2 ovs-vswitchd.log | sed "s/^.*\(bridge br0:.*\)$/\1/"], [0], + [bridge br0: added interface p2 on port 2 +]) OVS_VSWITCHD_STOP AT_CLEANUP
If VXLAN-GBP and VXLAN are set on the same port, only one of those tunnel types is going to be created. As other tunnel flags are added, like VXLAN-GPE, and they are incompatible, reject any configuration that put incompatible tunnels on the same port. A manual test has been done as well and the port didn't get an ofport. During a restart, the other tunnel type was rejected and the right flags were on the dpif_port. Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@redhat.com> --- lib/netdev-vport.c | 78 ++++++++++++++++++++++++++++++++++++++++++------------ tests/tunnel.at | 29 ++++++++++++++++++++ 2 files changed, 90 insertions(+), 17 deletions(-)