[ovs-dev] netdev-vport: Use the vxlan dst_port in netdev name
diff mbox series

Message ID 20190219015140.6755-1-chrism@mellanox.com
State New
Headers show
Series
  • [ovs-dev] netdev-vport: Use the vxlan dst_port in netdev name
Related show

Commit Message

Chris Mi Feb. 19, 2019, 1:51 a.m. UTC
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(-)

Comments

Flavio Leitner Feb. 20, 2019, 3:01 p.m. UTC | #1
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
Chris Mi Feb. 22, 2019, 2:13 a.m. UTC | #2
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&amp;data=02%7C01%7Cchrism%40mellanox.com%7C7ea17aefa05649b8
> 815f08d6974440ce%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C63
> 6862716738809934&amp;sdata=vsI2UA1d96E2LxRiX9cDiNCb4UxjOVlMSsvZ2D
> Xoy30%3D&amp;reserved=0

Patch
diff mbox series

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);