Page MenuHomeFreeBSD

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

Authored by pkelsey on Jan 22 2019, 2:10 AM.

Details

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"

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sbin/ifconfig/ifconfig.c
1046

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.

1059

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

sbin/ifconfig/ifconfig.c
1046

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.

1059

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.

sbin/ifconfig/ifconfig.c
1059

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.

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

sbin/ifconfig/ifconfig.c
1059

That behavior is unchanged by this patch.

sbin/ifconfig/ifconfig.c
1059

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.

sbin/ifconfig/ifconfig.c
1059

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

sbin/ifconfig/ifconfig.c
1059

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.

This revision is now accepted and ready to land.Jan 24 2019, 8:33 AM

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.

This revision was automatically updated to reflect the committed changes.