Page MenuHomeFreeBSD

libalias: Use switch/case statements to compare IPv4 protocol type (TCP/UDP/ICMP)
AcceptedPublic

Authored by neel_neelc.org on Fri, Jul 10, 12:13 AM.

Details

Summary

libalias: Use switch/case statements to compare IPv4 protocol type (TCP/UDP/ICMP) instead of if statements. This improves performance by reducing the lookup time for different types of IP packets, therefore reducing the CPU usage slightly.

Submitted by: Neel Chauhan <neel AT neelc DOT org>

Test Plan

Set up IPFW in-kernel NAT or natd as normal.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

neel_neelc.org requested review of this revision.Fri, Jul 10, 12:13 AM

Let's also do alias_proxy.c.

neel_neelc.org edited the summary of this revision. (Show Details)Fri, Jul 10, 2:14 AM
neel_neelc.org edited the summary of this revision. (Show Details)Fri, Jul 10, 2:17 AM

This will bring in a better coding style and improved readability.
But for the speed improvement, I'd like to see some evidence.

sys/netinet/libalias/alias.c
381–382

style(9) insists on break; for each case.

624

break missing

657

dito

1650

dito

1719

dito

sys/netinet/libalias/alias_proxy.c
197

dito

Sure, fixed it.

neel_neelc.org marked 5 inline comments as done.Fri, Jul 10, 3:03 PM

I see a fair amount of white space/formatting only changes in this, it MAY make since to break those out as 0 effort review.
I also have concerns over any performance claims, though I see the old code is probably at least sub optimal in that it often checks for UDP, then for TCP when the volume of traffic should almost always be mostly TCP. Also modern compilers should just "handle this" without rewritting code unless something odd is being done. If you really want to micro optimize I suspect rewriting this as "branchless" code that results in cmovl or other instructions might be interesting. I have heard of 40x code speed improvements using such tenchiques, but that is mostly in tight loops.

Have you looked at the assembler output to understand *why* your getting a performance gain?

How did you measure and obtain the 50-60% increase, what was the traffic load and mix?
Might be interesting to see if you get this same increase by doing the If's in the order, TCP, UDP, ICMP, which is what I suspect the perf increase is from as the code is now it requres 2 compares before we decide the packet is TCP, which should be the most common case.

The test output was a speedtest.net on a system behind IPFW NAT, but I looked at my test firewall box and realized the patch didn't apply.

I will do more benchmarks shortly.

This patch DOES reduce CPU usage, but not by 50%:

HEAD:

USER  PID  %CPU %MEM   VSZ  RSS TT  STAT STARTED      TIME COMMAND
root   11 663.9  0.0     0  128  -  RNL  09:15   277:00.70 [idle]
root    0  81.0  0.0     0  640  -  DLs  09:15     0:14.94 [kernel]
root   12  49.0  0.0     0  512  -  WL   09:15     1:19.83 [intr]

Patched:

USER  PID  %CPU %MEM   VSZ  RSS TT  STAT STARTED     TIME COMMAND
root   11 678.0  0.0     0  128  -  RNL  10:01   15:31.88 [idle]
root    0  74.0  0.0     0  640  -  DLs  10:01    0:12.60 [kernel]
root   12  45.9  0.0     0  512  -  WL   10:01    0:08.38 [intr]

This was tested using iperf3 with 20 TCP connections.

The descriotion's "50-60%" is incorrect and will be removed from the description. I just got confused with stock IPFW with full-duplex speedtests, sorry.

neel_neelc.org edited the summary of this revision. (Show Details)Fri, Jul 10, 5:09 PM

I also have concerns over any performance claims, though I see the old code is probably at least sub optimal in that it often checks for UDP, then for TCP when the volume of traffic should almost always be mostly TCP.

This is going to change, especially for high volume traffic.
HTTP/3 aka QUIC (as a successor of HTTP/2 aka SPDY) is UDP/443.

We see a bit more the 50% of our (Broadband-ISP) traffic to use UDP/443 instead of TCP/443.

gnn accepted this revision.Sun, Jul 12, 8:41 PM
gnn added a subscriber: gnn.

Second guessing the compiler is a fool's errand and I rather doubt you'd ever see a 50% "win" with this kind of change. That being said it makes the code more readable to my eyes and is acceptable.

This revision is now accepted and ready to land.Sun, Jul 12, 8:41 PM
sys/netinet/libalias/alias.c
586

May I ask for a break here?

sys/netinet/libalias/alias_proxy.c
197

"break" after "return" is dead code.