[ovs-dev] [PATCH v2] netdev-vport : Fix userspace tunnel ioctl(SIOCGIFINDEX) info logs.

[Ilya Maximets]

On 10/25/21 07:16, lin huang wrote:
> From: linhuang <linhuang at ruijie.com.cn>
> 
> Userspace tunnel doesn't have a valid device in the kernel. So
> get_ifindex() function (ioctl) always get error during
> adding a port, deleting a port or updating a port status.
> 
> The info log is
> "2021-08-29T09:17:39.830Z|00059|netdev_linux|INFO|ioctl(SIOCGIFINDEX)
> on vxlan_sys_4789 device failed: No such device"
> 
> If there are a lot of userspace tunnel ports on a bridge, the
> iface_refresh_netdev_status() function will spend a lot of time.
> 
> So ignore userspace tunnel port ioctl(SIOCGIFINDEX) operation, just
> return -ENODEV.

That makes sense to remove these unnecessary calls and logs, you're right.

One general comment:  with this change, can we remove the stripping
of that log message from the tests/system-traffic.at ?

Some more comments for the implementation inline.

Bets regards, Ilya Maximets.

> 
> Signed-off-by: linhuang <linhuang at ruijie.com.cn>
> Reviewed-by: Aaron Conole <aconole at redhat.com>
> ---
>  lib/netdev-offload.c | 6 ++++--
>  lib/netdev-vport.c   | 3 ++-
>  vswitchd/bridge.c    | 2 ++
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 8075cfbd8..00b7515cf 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -577,7 +577,9 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
>                      struct dpif_port *dpif_port)
>  {
>      struct port_to_netdev_data *data;
> -    int ifindex = netdev_get_ifindex(netdev);
> +    int ifindex;
> +
> +    netdev_set_dpif_type(netdev, dpif_type);
>  
>      ovs_rwlock_wrlock(&netdev_hmap_rwlock);
>      if (netdev_ports_lookup(dpif_port->port_no, dpif_type)) {
> @@ -589,6 +591,7 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
>      data->netdev = netdev_ref(netdev);
>      dpif_port_clone(&data->dpif_port, dpif_port);
>  
> +    ifindex = netdev_get_ifindex(netdev);
>      if (ifindex >= 0) {
>          data->ifindex = ifindex;
>          hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex);
> @@ -596,7 +599,6 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
>          data->ifindex = -1;
>      }
>  
> -    netdev_set_dpif_type(netdev, dpif_type);
>  
>      hmap_insert(&port_to_netdev, &data->portno_node,
>                  netdev_ports_hash(dpif_port->port_no, dpif_type));

Function netdev_ports_insert() is part of the offloading framework
and netdev_set_dpif_type() currently used only in offloading context.
But this changes expands the usage to non-offloading cases.
So, I think, it will be better to move the netdev_set_dpif_type()
out of netdev-offload.c to the lib/dpif.c:do_open() just before
calling the netdev_ports_insert().

> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 499c0291c..411ac343a 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -1151,8 +1151,9 @@ netdev_vport_get_ifindex(const struct netdev *netdev_)
>  {
>      char buf[NETDEV_VPORT_NAME_BUFSIZE];
>      const char *name = netdev_vport_get_dpif_port(netdev_, buf, sizeof(buf));
> +    const char *type = netdev_get_dpif_type(netdev_);

Might be better to re-name to 'dpif_type'.

>  
> -    return linux_get_ifindex(name);
> +    return (strcmp(type, "netdev")) ? linux_get_ifindex(name) : -ENODEV;

"netdev" is not a very reliable name.  We may also have "dummy" there or
even something else in the future.  So, originally I tried to only compare
with the "system" keyword for this kind of checks.  Therefore, suggesting
to invert the logic here by comparing with a "system" keyword instead.

Nit: no need to parenthesize the strcmp.

>  }
>  
>  #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index c790a56ad..473d79acd 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2052,6 +2052,8 @@ iface_do_create(const struct bridge *br,
>          goto error;
>      }
>  
> +    netdev_set_dpif_type(netdev, br->type);

Why do we need that?  Call to dpif_open() is just a few lines below.

> +
>      error = iface_set_netdev_config(iface_cfg, netdev, errp);
>      if (error) {
>          goto error;
>