On Wed, Jun 17, 2026 at 01:31:00PM -0400, Mike Pattrick wrote:
> 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?
>
Good idea! Thanks.
> -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
