In ifconfig(8), don't build, sort and search all system addresses when performing a non-status action on a single interface

[pkelsey]

Summary

When performing a non-status operation on a single interface, it is not necessary for ifconfig to build a list of all addresses in the system, sort them, then iterate through them looking for the entry for the single interface of interest. Doing so becomes increasingly expensive as the number of interfaces in the system grows (e.g., in a system with 1000+ vlan(4) interfaces).

For example, on a modern-ish Xeon system, performing an operation such as disabling IPv6 or adding an IPv4 address on an interface takes ~450ms with 2000 vlan(4) interfaces present, growing to ~2100ms with 4000 vlan(4) interfaces present.

This patch reduces those operation costs to ~2ms on the same system.

Test Plan

Tested using the following script:

#!/bin/sh

num_vlans=4000
intf=em1
#ifconfig=/usr/src/sbin/ifconfig/ifconfig
ifconfig=ifconfig

octet2=0
octet3=1
start=$(date +%s)
for v in `jot $num_vlans 1`; do
    ${ifconfig} $intf.$v create vlan $v vlandev $intf
    ${ifconfig} $intf.$v inet6 ifdisabled
    ${ifconfig} $intf.$v 10.0.${octet2}.${octet3}/16
    octet3=$(($octet3 + 1))
    if [ $octet3 -eq 255 ]; then
        octet3=1;
        octet2=$(($octet2 + 1))
    fi
done
stop=$(date +%s)
echo "$(( ($stop - $start) * 1000 / $num_vlans )) ms avg per create+config"

[kp]

sbin/ifconfig/ifconfig.c

1047 ↗

(On Diff #53081)

This would probably be easier to follow if you added a name (or ifname) argument.

There's a background project to try to lib-ify ifconfig, so anything that removes global state is helpful.

1060 ↗

(On Diff #53081)

Would we get this error if the interface doesn't exist?

[pkelsey]

sbin/ifconfig/ifconfig.c

1047 ↗

(On Diff #53081)

I could do that, although it wouldn't really change the balance of code using global state as this is just extracting existing code that uses the 'name' global int a function to avoid copy-pasting it.

1060 ↗

(On Diff #53081)

It appears that with the current kernel code, yes that is the failure case that could occur.

It's also possible for an error to occur if this ioctl is invoked in a vnet that is not up and running yet, but it's not clear that case can be hit via running ifconfig.

[kp]

sbin/ifconfig/ifconfig.c

1060 ↗

(On Diff #53081)

Right, but without this patch 'ifconfig foo' results in 'ifconfig: interface foo does not exist', and I think we'd want to keep that behaviour.

[pkelsey]

Added ifname parameter to getifflags(), which now uses that instead of the global name var.

[pkelsey]

sbin/ifconfig/ifconfig.c

1060 ↗

(On Diff #53081)

That behavior is unchanged by this patch.

[kp]

sbin/ifconfig/ifconfig.c

1060 ↗

(On Diff #53081)

Doesn't that mean that check on the name length in line 539 is redundant? If we've already checked that the interface (name) exists, it should be of an acceptable length.

[pkelsey]

sbin/ifconfig/ifconfig.c

1060 ↗

(On Diff #53081)

Specifically, 'ifconfig foo' resulting in 'ifconfig: interface foo does not exist' is due to if_nametoindex() returning 0.

[pkelsey]

sbin/ifconfig/ifconfig.c

1060 ↗

(On Diff #53081)

Hmm, possibly. This was a straight extract of what is going on in the ifa processing loop below, so the corresponding check would be redundant there also I think based on the same underlying reasoning, which would be that the system never contains interface names longer than IFNAMSIZ.

[kp]

To be clear: I don't object to your change. It's just that ifconfig is a bit of a mess and any opportunity for cleanup is tempting. Especially if I don't have to do it.