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

[nc]

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.

[nc]

Let's also do alias_proxy.c.

[donner]

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

[donner]

sys/netinet/libalias/alias.c

381

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

623

break missing

654

dito

1647

dito

1715

dito

sys/netinet/libalias/alias_proxy.c

197 ↗

(On Diff #74267)

dito

[nc]

Sure, fixed it.

[rgrimes]

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.

[nc]

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.

[nc]

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.

[donner]

In D25607#566660, @rgrimes wrote:

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]

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.

[donner]

sys/netinet/libalias/alias.c

585

May I ask for a break here?

sys/netinet/libalias/alias_proxy.c

197 ↗

(On Diff #74276)

"break" after "return" is dead code.

[kp]

Can you rebase this on current main? I ran into a conflict, but didn't investigate further.

sys/netinet/libalias/alias.c

363

Indent case at the same level as switch.

[philip]

I agree that this makes the code more readable.
The patch should be rebased though. There's a conflict in netinet/libalias/alias.c.

[donner]

While we are here, may you reformat the code to match style(9).
swicth and case does line up:

while ((ch = getopt(argc, argv, "abNn:")) != -1)
        switch (ch) {           /* Indent the switch. */
        case 'a':               /* Do not indent the case. */
                aflag = 1;      /* Indent case body one tab. */
                /* FALLTHROUGH */
        case 'b':
                bflag = 1;
                break;