| Message ID | MEYP282MB330229B6E19056EE4B81064DCD619@MEYP282MB3302.AUSP282.PROD.OUTLOOK.COM |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3] netdev-vport : Fix userspace tunnel ioctl(SIOCGIFINDEX) info logs. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Wed, 2021-11-24 at 13:32 +0000, lin huang wrote: > From: linhuang <linhuang@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. > > Thanks for taking care of the previous issues. Acked-by: Mike Pattrick <mkp@redhat.com>
On 11/24/21 14:32, lin huang wrote: > From: linhuang <linhuang@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. > > Signed-off-by: Lin Huang <linhuang@ruijie.com.cn> > Test-by: Mike Pattrick <mkp@redhat.com> > Reviewed-by: Aaron Conole <aconole@redhat.com> > Reviewed-by: Ilya Maximets <i.maximets@ovn.org> Hello. Thanks for v3. And sorry for delays. One comment about the commit message: please, don't add tags that wasn't actually provided in previous reviews. Also, patch changed noticeably between versions, so tags can not be preserved in this case. Some code comments inline. > --- > lib/netdev-vport.c | 4 +++- > vswitchd/bridge.c | 2 ++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index 499c029..f0ff02b 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -1151,8 +1151,10 @@ 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 *dpif_type = netdev_get_dpif_type(netdev_); > > - return linux_get_ifindex(name); > + return (dpif_type && !strcmp(dpif_type, "system") > + ? linux_get_ifindex(name) : -ENODEV); The operator precedence seems tricky here and hard to understand. Another problem is that if dpif_type is not defined, we should default to executing linux_get_ifindex() instead of returning an error. Suggesting to re-write as an 'if' condition like this: if (dpif_type && strcmp(dpif_type, "system")) { /* Not a system device. */ return -ENODEV; } return linux_get_ifindex(name); What do you think? Another option is to replace the 'dpif_type' NULL check with the ovs_assert(dpif_type), because we're not expecting it to be NULL with this change. Best regards, Ilya Maximets.
lin huang <miterv@outlook.com> writes: > From: linhuang <linhuang@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. > > Signed-off-by: Lin Huang <linhuang@ruijie.com.cn> > Test-by: Mike Pattrick <mkp@redhat.com> > Reviewed-by: Aaron Conole <aconole@redhat.com> > Reviewed-by: Ilya Maximets <i.maximets@ovn.org> > --- Acked-by: Aaron Conole <aconole@redhat.com>
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 499c029..f0ff02b 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -1151,8 +1151,10 @@ 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 *dpif_type = netdev_get_dpif_type(netdev_); - return linux_get_ifindex(name); + return (dpif_type && !strcmp(dpif_type, "system") + ? linux_get_ifindex(name) : -ENODEV); } #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 5223aa8..513ef7e 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->ofproto->type); + error = iface_set_netdev_config(iface_cfg, netdev, errp); if (error) { goto error;