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 Tue, Jan 22, 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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pkelsey created this revision.Tue, Jan 22, 2:10 AM
kristof added inline comments.Thu, Jan 24, 3:13 AM
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 added inline comments.Thu, Jan 24, 3:27 AM
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.

kristof added inline comments.Thu, Jan 24, 3:31 AM
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 updated this revision to Diff 53138.Thu, Jan 24, 3:41 AM

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

pkelsey added inline comments.Thu, Jan 24, 3:43 AM
sbin/ifconfig/ifconfig.c
1060 ↗(On Diff #53081)

That behavior is unchanged by this patch.

kristof added inline comments.Thu, Jan 24, 3:45 AM
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 added inline comments.Thu, Jan 24, 3:46 AM
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 added inline comments.Thu, Jan 24, 3:53 AM
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.

ae accepted this revision.Thu, Jan 24, 8:33 AM
This revision is now accepted and ready to land.Thu, Jan 24, 8:33 AM
kristof accepted this revision.Thu, Jan 24, 9:04 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.