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

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

Details

Reviewers

ae
kp

Commits

rS344030: MFC r343535:
rS343535: Speed up non-status operations applied to a single interface

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

Tests Skipped

Event Timeline

pkelsey created this revision.Jan 22 2019, 2:10 AM

kp added inline comments.Jan 24 2019, 3:13 AM

sbin/ifconfig/ifconfig.c
1047	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	Would we get this error if the interface doesn't exist?

pkelsey added inline comments.Jan 24 2019, 3:27 AM

sbin/ifconfig/ifconfig.c
1047	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	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 added inline comments.Jan 24 2019, 3:31 AM

sbin/ifconfig/ifconfig.c
1060	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.

pkelsey added inline comments.Jan 24 2019, 3:43 AM

sbin/ifconfig/ifconfig.c
1060	That behavior is unchanged by this patch.

kp added inline comments.Jan 24 2019, 3:45 AM

sbin/ifconfig/ifconfig.c
1060	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 added inline comments.Jan 24 2019, 3:46 AM

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

pkelsey added inline comments.Jan 24 2019, 3:53 AM

sbin/ifconfig/ifconfig.c
1060	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.

ae accepted this revision.Jan 24 2019, 8:33 AM

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.

Closed by commit rS343535: Speed up non-status operations applied to a single interface (authored by pkelsey). · Explain WhyJan 28 2019, 8:30 PM

This revision was automatically updated to reflect the committed changes.

Herald added a subscriber: imp. · View Herald TranscriptJan 28 2019, 8:30 PM