On Mon, Jun 8, 2026 at 4:56 AM Adrian Moreno via dev < [email protected]> wrote:
> We are relying on netlink notifications for netdev status changes. > > However, given we periodically have to send RTM_GETLINK anyways to get > the stats, it seems a good opportunity to double-check the status and > update netdevs if something changed. > > Signed-off-by: Adrian Moreno <[email protected]> > --- > lib/netdev-afxdp.c | 2 +- > lib/netdev-linux-private.h | 5 ++-- > lib/netdev-linux.c | 48 ++++++++++++++++++++++++++++++++++---- > 3 files changed, 48 insertions(+), 7 deletions(-) > > diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c > index 8ef2ac192..fc0196cc0 100644 > --- a/lib/netdev-afxdp.c > +++ b/lib/netdev-afxdp.c > @@ -1254,7 +1254,7 @@ netdev_afxdp_get_stats(const struct netdev *netdev, > > ovs_mutex_lock(&dev->mutex); > > - error = get_stats_via_netlink(netdev, &dev_stats); > + error = get_stats_via_netlink(dev, &dev_stats); > if (error) { > VLOG_WARN_RL(&rl, "%s: Error getting AF_XDP statistics.", > netdev_get_name(netdev)); > diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h > index cf4a021d3..d7a2620c2 100644 > --- a/lib/netdev-linux-private.h > +++ b/lib/netdev-linux-private.h > @@ -36,6 +36,7 @@ > #include "ovs-atomic.h" > #include "timer.h" > > +struct netdev_linux; > struct netdev; > > /* The maximum packet length is 16 bits */ > @@ -52,10 +53,10 @@ struct netdev_rxq_linux { > int netdev_linux_construct(struct netdev *); > int netdev_linux_get_status(const struct netdev *, struct smap *); > void netdev_linux_run(const struct netdev_class *); > - > -int get_stats_via_netlink(const struct netdev *netdev_, > +int get_stats_via_netlink(struct netdev_linux *netdev, > struct netdev_stats *stats); > > + > struct netdev_linux { > struct netdev up; > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index faef1134a..7ce3ee5ba 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -819,6 +819,37 @@ netdev_linux_wait(const struct netdev_class > *netdev_class OVS_UNUSED) > } > } > > + > +/* Check whether the current state of the netdev is the same as the one > + * reported by netlink. */ > +static bool > +netdev_linux_check(struct netdev_linux *dev, > + const struct rtnetlink_change *change) > + OVS_REQUIRES(dev->mutex) > +{ > + bool result = true; > + > + if (dev->ifi_flags != change->ifi_flags) { > + result = false; > + } > + if (dev->mtu != change->mtu) { > + result = false; > + } > + if (!eth_addr_is_zero(change->mac) && > + !eth_addr_equals(dev->etheraddr, change->mac)) { > + result = false; > + } > + if (dev->ifindex != change->if_index) { > + result = false; > + } > + if (dev->is_lag_primary != (change->primary && > + netdev_linux_kind_is_lag(change->primary))) { > + result = false; > + } > + > + return result; > Nit, but every use of netdev_linux_check inverts the return value of this function. Currently it returns true when the netdev has not changed, but the callers all care to check if the netdev has changed. Why not just flip all the boolean values in this function and rebrand it on a check of if the netdev has changed? -M > +} > + > static void > netdev_linux_changed(struct netdev_linux *dev, unsigned int mask) > OVS_REQUIRES(dev->mutex) > @@ -2279,7 +2310,7 @@ netdev_linux_get_stats(const struct netdev *netdev_, > > ovs_mutex_lock(&netdev->mutex); > get_stats_via_vport(netdev_, stats); > - error = get_stats_via_netlink(netdev_, &dev_stats); > + error = get_stats_via_netlink(netdev, &dev_stats); > if (error) { > if (!netdev->vport_stats_error) { > error = 0; > @@ -2318,7 +2349,7 @@ netdev_tap_get_stats(const struct netdev *netdev_, > struct netdev_stats *stats) > > ovs_mutex_lock(&netdev->mutex); > get_stats_via_vport(netdev_, stats); > - error = get_stats_via_netlink(netdev_, &dev_stats); > + error = get_stats_via_netlink(netdev, &dev_stats); > if (error) { > if (!netdev->vport_stats_error) { > error = 0; > @@ -6754,7 +6785,9 @@ netdev_stats_from_rtnl_link_stats64(struct > netdev_stats *dst, > } > > int > -get_stats_via_netlink(const struct netdev *netdev_, struct netdev_stats > *stats) > +get_stats_via_netlink(struct netdev_linux *netdev, > + struct netdev_stats *stats) > + OVS_REQUIRES(netdev->mutex) > { > struct rtnetlink_change change = {0}; > struct rtnl_link_stats64 _stats64; > @@ -6771,7 +6804,7 @@ get_stats_via_netlink(const struct netdev *netdev_, > struct netdev_stats *stats) > sizeof(struct ifinfomsg) + NL_ATTR_SIZE(IFNAMSIZ), > RTM_GETLINK, NLM_F_REQUEST); > ofpbuf_put_zeros(&request, sizeof(struct ifinfomsg)); > - nl_msg_put_string(&request, IFLA_IFNAME, netdev_get_name(netdev_)); > + nl_msg_put_string(&request, IFLA_IFNAME, > netdev_get_name(&netdev->up)); > error = nl_transact(NETLINK_ROUTE, &request, &reply); > ofpbuf_uninit(&request); > if (error) { > @@ -6791,6 +6824,13 @@ get_stats_via_netlink(const struct netdev *netdev_, > struct netdev_stats *stats) > VLOG_WARN_RL(&rl, "RTM_GETLINK reply lacks stats"); > error = EPROTO; > } > + > + /* Verify the internal state of netdevs matches the one given by > + * netlink. */ > + if (!netdev_linux_check(netdev, &change)) { > + netdev_linux_update__(netdev, &change); > + } > + > } else { > VLOG_WARN_RL(&rl, "invalid RTM_GETLINK reply"); > error = EPROTO; > -- > 2.54.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
