On Tue, 3 Mar 2026 at 23:43, Ilya Maximets <[email protected]> wrote:
> On 1/27/26 4:11 PM, Matteo Perin via dev wrote: > > This commit adds the peer_ifindex to the interface status column for > > Linux veth devices, allowing applications like OVN to programmatically > > discover veth pairs without requiring additional system calls or manual > > configuration. > > > > The implementation leverages the existing ethtool infrastructure: > > - Uses ETHTOOL_GDRVINFO to identify veth devices and get n_stats > > - Queries ETH_SS_STATS string set to get statistic names > > - Dynamically allocates buffer based on n_stats > > - Uses ETHTOOL_GSTATS to retrieve veth statistics > > - Finds peer_ifindex by name in the statistics array > > > > The ifindex value is cached to avoid scalability issues with a > > considerable amount of ports. > > The cached peer_ifindex is then properly invalidated when the > > corresponding veth device is removed. > > This ensures that, when veth pairs are deleted and recreated with > > the same name, the new peer_ifindex is correctly detected and reported. > > > > A test that create veth pairs across network namespaces, verifies > > peer_ifindex reporting, and validates correct detection of ifindex > > changes when pairs are recreated was added as well. > > > > Signed-off-by: Matteo Perin <[email protected]> > > --- > > lib/netdev-linux-private.h | 1 + > > lib/netdev-linux.c | 49 ++++++++++++++++++++++++++++++++++++++ > > tests/system-interface.at | 47 ++++++++++++++++++++++++++++++++++++ > > vswitchd/vswitch.xml | 13 ++++++++++ > > 4 files changed, 110 insertions(+) > > Hi, Matteo. Thanks for the patch! It seems functionally correct (though > I didn't actually try it). I left a few comments below, mostly style > and formatting related. > > Best regards, Ilya Maximets. > > > > > diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h > > index cf4a021d3..3f3fc04b5 100644 > > --- a/lib/netdev-linux-private.h > > +++ b/lib/netdev-linux-private.h > > @@ -97,6 +97,7 @@ struct netdev_linux { > > uint8_t current_duplex; /* Cached from ETHTOOL_GSET. */ > > > > struct ethtool_drvinfo drvinfo; /* Cached from ETHTOOL_GDRVINFO. */ > > + uint64_t peer_ifindex; /* Cached from ETHTOOL_GSTATS > (veth). */ > > struct tc *tc; > > > > /* For devices of class netdev_tap_class only. */ > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > > index 93a064cc2..45779cffa 100644 > > --- a/lib/netdev-linux.c > > +++ b/lib/netdev-linux.c > > @@ -3730,11 +3730,52 @@ netdev_linux_get_status(const struct netdev > *netdev_, struct smap *smap) > > > > COVERAGE_INC(netdev_get_ethtool); > > memset(&netdev->drvinfo, 0, sizeof netdev->drvinfo); > > + netdev->peer_ifindex = 0; > > error = netdev_linux_do_ethtool(netdev->up.name, > > cmd, > > ETHTOOL_GDRVINFO, > > "ETHTOOL_GDRVINFO"); > > if (!error) { > > + /* For veth devices, also get the 'peer_ifindex'. */ > > + size_t n_stats = netdev->drvinfo.n_stats; > > + if (!strcmp(netdev->drvinfo.driver, "veth") && > > + n_stats > 0) { > > This fits into a single line. But also there is too much nesting > happening here and it's hard to read. The content of this if > statement can be moved into a separate function. > > Also, while writing the function, please convert all the blocks > like: > > if (!error) { > <do something> > } > > into: > > if (error) { > return; > } > <do something> > > To save on indentation. > > > + struct ethtool_gstrings *names = NULL; > > + struct ethtool_stats *stats = NULL; > > + int s_error; > > + > > + /* Get the names of the statistics. */ > > + s_error = netdev_linux_read_definitions(netdev, > > + ETH_SS_STATS, > > + &names); > > Indent to the opening parenthesis. > > > + if (!s_error) { > > + stats = xzalloc(sizeof *stats + > > + n_stats * sizeof stats->data[0]); > > + stats->cmd = ETHTOOL_GSTATS; > > + stats->n_stats = n_stats; > > + /* Get the statistics values. */ > > + s_error = netdev_linux_do_ethtool(netdev->up.name, > > + (struct ethtool_cmd *) stats, > > + ETHTOOL_GSTATS, > > + "ETHTOOL_GSTATS"); > > This should also be indented to the opening parenthesis. If the cast > doesn't fit, can cast and assing to 'cmd' before the call. > > > + if (!s_error) { > > + /* Find 'peer_ifindex' in names and get value. > */ > > + for (uint32_t i = 0; > > + i < names->len && i < stats->n_stats; > > + i++) { > > Indent to the opening parenthesis, i.e. 1 space to the right. > > > + char *name = > > + (char *) &names->data[i * > ETH_GSTRING_LEN]; > > + if (!strcmp(name, "peer_ifindex") && > > + stats->data[i] > 0) { > > + netdev->peer_ifindex = stats->data[i]; > > + break; > > + } > > + } > > + } > > + free(stats); > > + } > > + free(names); > > No need to free names on error. > > > + } > > netdev->cache_valid |= VALID_DRVINFO; > > } > > } > > @@ -3743,6 +3784,14 @@ netdev_linux_get_status(const struct netdev > *netdev_, struct smap *smap) > > smap_add(smap, "driver_name", netdev->drvinfo.driver); > > smap_add(smap, "driver_version", netdev->drvinfo.version); > > smap_add(smap, "firmware_version", netdev->drvinfo.fw_version); > > + > > + /* Report the cached 'peer_ifindex' (for veth devices). */ > > + if (netdev->peer_ifindex > 0) { > > It seems like the value can't be negative in this code, so the > '> 0' part is redundant. > > > + smap_add_format(smap, > > + "peer_ifindex", > > + "%"PRIu64, > > + netdev->peer_ifindex); > > This can be two lines instead of 4 - everything on the same line as > smap_add > and the last value on its own, indented to the opening parenthesis. > > > + } > > } > > ovs_mutex_unlock(&netdev->mutex); > > > > diff --git a/tests/system-interface.at b/tests/system-interface.at > > index 15c4a0e2e..b24cbc855 100644 > > --- a/tests/system-interface.at > > +++ b/tests/system-interface.at > > @@ -221,3 +221,50 @@ half > > > > OVS_TRAFFIC_VSWITCHD_STOP > > AT_CLEANUP > > + > > +dnl Test that peer_ifindex is correctly reported for veth devices. > > +AT_SETUP([interface - veth peer_ifindex status]) > > +AT_SKIP_IF([test $HAVE_ETHTOOL = "no"]) > > + > > +OVS_TRAFFIC_VSWITCHD_START() > > + > > +dnl Create a network namespace for the peer veth end. > > +AT_CHECK([ip netns add testns]) > > +on_exit 'ip netns del testns' > > We have ADD_NAMESPACES macro that handles everything. > > > + > > +dnl Create a veth pair: ovs-veth0 (main ns) <-> ovs-veth1 (testns). > > +AT_CHECK([ip link add ovs-veth0 type veth peer name ovs-veth1]) > > +AT_CHECK([ip link set ovs-veth1 netns testns]) > > +on_exit 'ip link del ovs-veth0' > > This should be right after addition. > > > + > > +dnl Add ovs-veth0 to br0. > > This is obvious, no need for a comment. > > > +AT_CHECK([ovs-vsctl add-port br0 ovs-veth0]) > > + > > +dnl Get the actual ifindex of ovs-veth1 in testns and verify it's > reported. > > +PEER1_IFINDEX=$(ip netns exec testns ip link show ovs-veth1 | sed -n > 's/^\([[0-9]]*\):.*/\1/p') > > May be better to use lowercase for shell variables to easier distinguish > them from m4 macros. Also, might be better to AT_CHECK into stdout and > then parse it. It would also save some line length. > > > + > > +dnl Verify OVS reports the correct peer_ifindex. > > +OVS_WAIT_UNTIL_EQUAL([ovs-vsctl get interface ovs-veth0 > status:peer_ifindex], ["\"$PEER1_IFINDEX\""]) > > may split the line after the comma. Indent to the opening parenthesis. > Also, I didn't try this test, but is it necessary to add those quotes? > > > + > > +dnl The veth pair is deleted and then recreated with the same name, > this tests ifindex stale values detection. > > Line is a bit too long. > > > +AT_CHECK([ip link del ovs-veth0]) > > +AT_CHECK([ip link add ovs-veth0 type veth peer name ovs-veth1]) > > +AT_CHECK([ip link set ovs-veth1 netns testns]) > > + > > +dnl Get the NEW peer's ifindex (will be different). > > +PEER2_IFINDEX=$(ip netns exec testns ip link show ovs-veth1 | sed -n > 's/^\([[0-9]]*\):.*/\1/p') > > Same here. > > > + > > +dnl Verify OVS detects the change and reports the NEW peer_ifindex. > > +OVS_WAIT_UNTIL_EQUAL([ovs-vsctl get interface ovs-veth0 > status:peer_ifindex], ["\"$PEER2_IFINDEX\""]) > > And here. > > > + > > +dnl Verify the two peer ifindexes are different. > > +test "$PEER1_IFINDEX" != "$PEER2_IFINDEX" > > This should be under AT_CHECK, otherwise it doesn't really verify much. > > > + > > +OVS_TRAFFIC_VSWITCHD_STOP(["dnl > > +/could not open network device ovs-veth0/d > > +/cannot get .*STP status on nonexistent port/d > > +/ethtool command .*on network device ovs-veth0 failed/d > > +/error receiving .*ovs-veth0/d > > +/ovs-veth0: removing policing failed/d"]) > > + > > +AT_CLEANUP > > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > > index b7a5afc0a..25f635e82 100644 > > --- a/vswitchd/vswitch.xml > > +++ b/vswitchd/vswitch.xml > > @@ -3746,6 +3746,19 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 > type=patch options:peer=p1 \ > > The version string of the network adapter's firmware, if > available. > > </column> > > > > + <column name="status" key="peer_ifindex"> > > + <p> > > + For veth devices on Linux, this column contains the interface > index > > + of the peer veth device. This allows applications to > > + programmatically discover veth pairs without requiring > additional > > + system calls or parsing network interface information. > > + </p> > > + <p> > > + Only present for veth devices (when <ref column="status" > > + key="driver_name"/> is "veth"). Not available on non-Linux > systems. > > + </p> > > + </column> > > + > > <column name="status" key="source_ip"> > > The source IP address used for an IPv4/IPv6 tunnel end-point, > such as > > <code>gre</code>. > Thank you, Ilya. I should have addressed all the style inconsistencies and errors in the v4 revision of the patch. On Tue, 3 Mar 2026 at 23:43, Ilya Maximets <[email protected]> wrote: > > + > > +dnl Verify OVS reports the correct peer_ifindex. > > +OVS_WAIT_UNTIL_EQUAL([ovs-vsctl get interface ovs-veth0 > status:peer_ifindex], ["\"$PEER1_IFINDEX\""]) > > may split the line after the comma. Indent to the opening parenthesis. > Also, I didn't try this test, but is it necessary to add those quotes? Yes, this is necessary because ovs-vsctl get returns string values with literal quotes, so I had to keep those to get an exact match. Best Regards, Matteo _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
