| Message ID | 20190219015140.6755-1-chrism@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] netdev-vport: Use the vxlan dst_port in netdev name | expand |
On Tue, Feb 19, 2019 at 10:51:40AM +0900, Chris Mi wrote: > If vxlan dst_port is not default 4789, "ovs-dpctl dump-flows" > will fail. The error message is: > > netdev_linux|INFO|ioctl(SIOCGIFINDEX) on vxlan_sys_4789 device failed: No such device > > That's because when calling netdev_vport_construct() for netdev > vxlan_sys_xxxx, the default dst_port is used. Actually, the dst_port > value is in the name of the netdev. Use it to avoid the error. During ovs initialization, the interface is created and then the construct is called. That's where this patch is setting the dst_port, which looks okay because after that set_tunnel_config() is called when setting the iface, and dst_port gets updated with the db config. However, during dump-flows it opens the netdev but doesn't look at the db as far as I can tell, then you see the issue because it is hardcoded to the default port. I don't think it makes sense to check the db at this point since we care more about the real iface name. With that said, the patch makes sense to me and as you said, would be nice to apply the same fix to other tunnel types. fbl > > Signed-off-by: Chris Mi <chrism@mellanox.com> > --- > > v1 > == > > Any comment about this patch? We are not sure if it is correct > to verify the port from the name. If it is correct, is it applicable > for other tunnels? Thanks! > > > lib/netdev-vport.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index 808a43f99..be68fad1a 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -189,17 +189,28 @@ netdev_vport_alloc(void) > int > netdev_vport_construct(struct netdev *netdev_) > { > + const struct netdev_class *class = netdev_get_class(netdev_); > + const char *dpif_port = netdev_vport_class_get_dpif_port(class); > struct netdev_vport *dev = netdev_vport_cast(netdev_); > const char *type = netdev_get_type(netdev_); > > ovs_mutex_init(&dev->mutex); > eth_addr_random(&dev->etheraddr); > > - /* Add a default destination port for tunnel ports if none specified. */ > + /* Add a default destination port for tunnel ports if none specified. > + * For vxlan, the dst_port is in the netdev name when creating it, so > + * use it instead of the default one */ > if (!strcmp(type, "geneve")) { > dev->tnl_cfg.dst_port = htons(GENEVE_DST_PORT); > } else if (!strcmp(type, "vxlan")) { > - dev->tnl_cfg.dst_port = htons(VXLAN_DST_PORT); > + const char *p, *name = netdev_->name; > + uint16_t port = VXLAN_DST_PORT; > + > + if (!strncmp(name, dpif_port, strlen(dpif_port))) { > + p = name + strlen(dpif_port) + 1; > + port = atoi(p); > + } > + dev->tnl_cfg.dst_port = htons(port); > update_vxlan_global_cfg(netdev_, NULL, &dev->tnl_cfg); > } else if (!strcmp(type, "lisp")) { > dev->tnl_cfg.dst_port = htons(LISP_DST_PORT); > -- > 2.14.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Flavio, > -----Original Message----- > From: Flavio Leitner <fbl@sysclose.org> > Sent: Wednesday, February 20, 2019 11:01 PM > To: Chris Mi <chrism@mellanox.com> > Cc: ovs-dev@openvswitch.org; simon.horman@netronome.com > Subject: Re: [ovs-dev] [PATCH] netdev-vport: Use the vxlan dst_port in > netdev name > > On Tue, Feb 19, 2019 at 10:51:40AM +0900, Chris Mi wrote: > > If vxlan dst_port is not default 4789, "ovs-dpctl dump-flows" > > will fail. The error message is: > > > > netdev_linux|INFO|ioctl(SIOCGIFINDEX) on vxlan_sys_4789 device failed: > No such device > > > > That's because when calling netdev_vport_construct() for netdev > > vxlan_sys_xxxx, the default dst_port is used. Actually, the dst_port > > value is in the name of the netdev. Use it to avoid the error. > > > During ovs initialization, the interface is created and then the > construct is called. That's where this patch is setting the dst_port, > which looks okay because after that set_tunnel_config() is called > when setting the iface, and dst_port gets updated with the db config. > > However, during dump-flows it opens the netdev but doesn't look at > the db as far as I can tell, then you see the issue because it is > hardcoded to the default port. I don't think it makes sense to check > the db at this point since we care more about the real iface name. > > With that said, the patch makes sense to me and as you said, would > be nice to apply the same fix to other tunnel types. Thanks for your detailed info and support for this change. Patch v2 was sent for review. Thanks, Chris > > fbl > > > > > Signed-off-by: Chris Mi <chrism@mellanox.com> > > --- > > > > v1 > > == > > > > Any comment about this patch? We are not sure if it is correct > > to verify the port from the name. If it is correct, is it applicable > > for other tunnels? Thanks! > > > > > > lib/netdev-vport.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > > index 808a43f99..be68fad1a 100644 > > --- a/lib/netdev-vport.c > > +++ b/lib/netdev-vport.c > > @@ -189,17 +189,28 @@ netdev_vport_alloc(void) > > int > > netdev_vport_construct(struct netdev *netdev_) > > { > > + const struct netdev_class *class = netdev_get_class(netdev_); > > + const char *dpif_port = netdev_vport_class_get_dpif_port(class); > > struct netdev_vport *dev = netdev_vport_cast(netdev_); > > const char *type = netdev_get_type(netdev_); > > > > ovs_mutex_init(&dev->mutex); > > eth_addr_random(&dev->etheraddr); > > > > - /* Add a default destination port for tunnel ports if none specified. */ > > + /* Add a default destination port for tunnel ports if none specified. > > + * For vxlan, the dst_port is in the netdev name when creating it, so > > + * use it instead of the default one */ > > if (!strcmp(type, "geneve")) { > > dev->tnl_cfg.dst_port = htons(GENEVE_DST_PORT); > > } else if (!strcmp(type, "vxlan")) { > > - dev->tnl_cfg.dst_port = htons(VXLAN_DST_PORT); > > + const char *p, *name = netdev_->name; > > + uint16_t port = VXLAN_DST_PORT; > > + > > + if (!strncmp(name, dpif_port, strlen(dpif_port))) { > > + p = name + strlen(dpif_port) + 1; > > + port = atoi(p); > > + } > > + dev->tnl_cfg.dst_port = htons(port); > > update_vxlan_global_cfg(netdev_, NULL, &dev->tnl_cfg); > > } else if (!strcmp(type, "lisp")) { > > dev->tnl_cfg.dst_port = htons(LISP_DST_PORT); > > -- > > 2.14.4 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma > il.openvswitch.org%2Fmailman%2Flistinfo%2Fovs- > dev&data=02%7C01%7Cchrism%40mellanox.com%7C7ea17aefa05649b8 > 815f08d6974440ce%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C63 > 6862716738809934&sdata=vsI2UA1d96E2LxRiX9cDiNCb4UxjOVlMSsvZ2D > Xoy30%3D&reserved=0
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 808a43f99..be68fad1a 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -189,17 +189,28 @@ netdev_vport_alloc(void) int netdev_vport_construct(struct netdev *netdev_) { + const struct netdev_class *class = netdev_get_class(netdev_); + const char *dpif_port = netdev_vport_class_get_dpif_port(class); struct netdev_vport *dev = netdev_vport_cast(netdev_); const char *type = netdev_get_type(netdev_); ovs_mutex_init(&dev->mutex); eth_addr_random(&dev->etheraddr); - /* Add a default destination port for tunnel ports if none specified. */ + /* Add a default destination port for tunnel ports if none specified. + * For vxlan, the dst_port is in the netdev name when creating it, so + * use it instead of the default one */ if (!strcmp(type, "geneve")) { dev->tnl_cfg.dst_port = htons(GENEVE_DST_PORT); } else if (!strcmp(type, "vxlan")) { - dev->tnl_cfg.dst_port = htons(VXLAN_DST_PORT); + const char *p, *name = netdev_->name; + uint16_t port = VXLAN_DST_PORT; + + if (!strncmp(name, dpif_port, strlen(dpif_port))) { + p = name + strlen(dpif_port) + 1; + port = atoi(p); + } + dev->tnl_cfg.dst_port = htons(port); update_vxlan_global_cfg(netdev_, NULL, &dev->tnl_cfg); } else if (!strcmp(type, "lisp")) { dev->tnl_cfg.dst_port = htons(LISP_DST_PORT);
If vxlan dst_port is not default 4789, "ovs-dpctl dump-flows" will fail. The error message is: netdev_linux|INFO|ioctl(SIOCGIFINDEX) on vxlan_sys_4789 device failed: No such device That's because when calling netdev_vport_construct() for netdev vxlan_sys_xxxx, the default dst_port is used. Actually, the dst_port value is in the name of the netdev. Use it to avoid the error. Signed-off-by: Chris Mi <chrism@mellanox.com> --- v1 == Any comment about this patch? We are not sure if it is correct to verify the port from the name. If it is correct, is it applicable for other tunnels? Thanks! lib/netdev-vport.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)