On Wed, Apr 08, 2026 at 04:53:02PM -0400, Mike Pattrick wrote:
> On Wed, Mar 11, 2026 at 2:06 AM Adrian Moreno via dev <
> [email protected]> wrote:
>
> > Netdev flags are queried multiple times per poll loop. Sending a GETLINK
> > command for each can be very inefficient if there is RTNL lock
> > contention.
> >
> > Cache the result of the query as we do with other netdev attributes.
> >
> > Signed-off-by: Adrian Moreno <[email protected]>
> >
>
> Hi Adrian, I think this this a good idea, but have an implementation
> question below.
>
>
> > ---
> > lib/netdev-linux.c | 15 ++++++++++++---
> > lib/tnl-ports.c | 2 +-
> > 2 files changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 66153bf49..e6127240b 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -265,6 +265,7 @@ enum {
> > VALID_DRVINFO = 1 << 6,
> > VALID_FEATURES = 1 << 7,
> > VALID_NUMA_ID = 1 << 8,
> > + VALID_FLAGS = 1 << 9,
> > };
> >
> > /* Linux 4.4 introduced the ability to skip the internal stats gathering
> > @@ -830,7 +831,7 @@ netdev_linux_run(const struct netdev_class
> > *netdev_class OVS_UNUSED)
> >
> > ovs_mutex_lock(&netdev->mutex);
> > update_flags_local(netdev);
> > - netdev_linux_changed(netdev, netdev->ifi_flags, 0);
> > + netdev_linux_changed(netdev, netdev->ifi_flags,
> > VALID_FLAGS);
> > ovs_mutex_unlock(&netdev->mutex);
> >
> > netdev_close(netdev_);
> > @@ -910,6 +911,7 @@ netdev_linux_update__(struct netdev_linux *dev,
> >
> > dev->ifindex = change->if_index;
> > dev->cache_valid |= VALID_IFINDEX;
> > + dev->cache_valid |= VALID_FLAGS;
> > dev->get_ifindex_error = 0;
> > dev->present = true;
> > } else {
> > @@ -3849,7 +3851,11 @@ static int
> > update_flags_local(struct netdev_linux *netdev)
> > OVS_REQUIRES(netdev->mutex)
> > {
> > - return get_flags(&netdev->up, &netdev->ifi_flags);
> > + int error = get_flags(&netdev->up, &netdev->ifi_flags);
> > + if (!error) {
> > + netdev->cache_valid |= VALID_FLAGS;
> > + }
> > + return error;
> > }
> >
> > static int
> > @@ -3888,7 +3894,8 @@ netdev_linux_update_flags(struct netdev *netdev_,
> > enum netdev_flags off,
> > error = modify_flags(netdev, off, on, old_flagsp);
> > } else {
> > /* Try reading flags over netlink, or fall back to ioctl. */
> > - if (netdev_linux_update_via_netlink(netdev)) {
> > + if (!(netdev->cache_valid & VALID_FLAGS) &&
> > + netdev_linux_update_via_netlink(netdev)) {
> > error = update_flags_local(netdev);
> > }
> > *old_flagsp = iff_to_nd_flags(netdev->ifi_flags);
> > @@ -6955,6 +6962,8 @@ netdev_linux_update_via_netlink(struct netdev_linux
> > *netdev)
> > netdev->ifi_flags = change->ifi_flags;
> > changed = true;
> > }
> > + netdev->cache_valid |= VALID_FLAGS;
> > +
> > if (change->mtu && change->mtu != netdev->mtu) {
> > netdev->mtu = change->mtu;
> > netdev->cache_valid |= VALID_MTU;
> > diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
> > index 56119b300..67b6ebcf7 100644
> > --- a/lib/tnl-ports.c
> > +++ b/lib/tnl-ports.c
> > @@ -402,13 +402,13 @@ insert_ipdev__(struct netdev *dev,
> >
> > ip_dev = xzalloc(sizeof *ip_dev);
> > ip_dev->dev = netdev_ref(dev);
> > - ip_dev->change_seq = netdev_get_change_seq(dev);
> > error = netdev_get_etheraddr(ip_dev->dev, &ip_dev->mac);
> > if (error) {
> > goto err_free_ipdev;
> > }
> > ip_dev->addr = addr;
> > ip_dev->n_addr = n_addr;
> > + ip_dev->change_seq = netdev_get_change_seq(dev);
> >
>
> Why is this change needed here? It doesn't seem related to flags caching.
I should have explained this better, maybe adding a comment or
a line in the commit message.
Before this patch, this code did the following:
netdev_get_flags(dev, &flags);
ip_dev->change_seq = netdev_get_change_seq(dev);
netdev_get_etheraddr(ip_dev->dev, &ip_dev->mac);
// insert ip_dev into addr_list
The first call to "netdev_get_flags()" was triggering a full-blown
"netdev_linux_update_via_netlink" which would update the entire internal
state, including the ethernet address and cache everything.
The subsequent call "netdev_get_etheraddr()" would not trigger any
syscall since the information was cached.
This behavior could be seen as an abstraction leak. This code _knew_
netdev_get_flags was _also_ getting the etheraddr. This assumption is
not true anymore with this patch.
This patch caches the flags, so when the netdev is created, we only get
its flags (via ioctl) and cache them. This lazy initialization of internal
netdev information is useful because many times we don't need anything
but the flags. So when "netdev_get_etheraddr" is called, the etheraddr
is not available, "netdev_linux_update_via_netlink()" is called, and
it changes the netdev's change_seq after it has been read into
"ipdev->change_seq".
The result of "ip_dev->change_seq" always being out of date is that
"tnl_port_map_run" becomes an infite loop of ipdevs being deleted and
re-inserted.
Thanks.
Adrián
>
> -M
>
>
> > ovs_strlcpy(ip_dev->dev_name, netdev_get_name(dev), sizeof
> > ip_dev->dev_name);
> > ovs_list_insert(&addr_list, &ip_dev->node);
> > map_insert_ipdev(ip_dev);
> > --
> > 2.53.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
