| Message ID | 20260311060533.52598-3-amorenoz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | netdev-linux: Use event-driven netlink notifications. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Wed, Mar 11, 2026 at 2:06 AM Adrian Moreno via dev < ovs-dev@openvswitch.org> 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 <amorenoz@redhat.com> > 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. -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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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); 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);
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 <amorenoz@redhat.com> --- lib/netdev-linux.c | 15 ++++++++++++--- lib/tnl-ports.c | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-)