Page MenuHomeFreeBSD

Optimize the case where we have IPSEC enabled but do not have security policies.
ClosedPublic

Authored by gnn on Oct 23 2015, 8:17 PM.

Details

Summary

Turning on IPSEC used to introduce a slight amount of performance degradation (7%) for host host TCP connections over 10Gbps links, even when there were no secuirty policies in place. There is no change in performance on 1Gbps network links. Testing GENERIC vs. GENERIC-NOIPSEC vs. GENERIC with this change shows that the new code removes any overhead introduced by having IPSEC always in the kernel.

Test Plan

Tested using iperf-twohost-nooffload test in the netperf repo. NICs that have TSO/LRO are unaffected by the IPSEC code because they bypass much of the stack. The test turns off these features before executing.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 913
Build 913: arc lint + arc unit

Event Timeline

gnn updated this revision to Diff 9668.Oct 23 2015, 8:17 PM
gnn retitled this revision from to Optimize the case where we have IPSEC enabled but do not have security policies..
gnn updated this object.
gnn edited the test plan for this revision. (Show Details)
gnn added a reviewer: network.Oct 23 2015, 8:18 PM
loos added a subscriber: loos.Oct 26 2015, 12:11 PM
julian added a subscriber: julian.Oct 26 2015, 3:54 PM

My only comment is that if you have a simple enough test.. i.e. "are there any policies (regardless of direction) then I'd go for guarding the callers rather that returning immediately.. in other words don't even do the call.
amd64 may have very cheap function calls but they still cause register spills etc. some other processors may cost more..

general concept is good though.

ipsec_hdrsiz is calle d in 6 places (according to fxr) but some of those places offer a much larger saving than just the function call. Some largish #ifdef IPSEC clauses could be wrapped with a guarding 'if'..

ipsec46_in_reject in 2 places (in the same file), but they are already in ipsec (ipsec4_in_reject and ipsec6_in_reject).. guarding THEM saves TWO function calls (*) and you'd probbaly only have to guard he ones not in ipsec as I think THEY are called from both within and without ipsec. ((*)though it might get inlined)
ipsec_hdrsiz_tcp in 1 place and might not win much either way
ip_ipsec_output in 1 place, but skipping the call would also include skipping the switch statement used for the return value.

gnn added a comment.Oct 26 2015, 4:14 PM

Only one of the checks is completely generic, most all of the rest have an idea of the direction of the packets.

The calls to ipsec_hdrsiz are already protected in the place you point out. See the tcp_subr.c change above.

I prefer to keep these changes as encapsulated as possible within IPSEC rather than unduly polluting the rest of the stack with them.

That being said, a later pass might address these issues.

allanjude accepted this revision.Oct 27 2015, 12:48 AM
allanjude added a reviewer: allanjude.
This revision is now accepted and ready to land.Oct 27 2015, 12:48 AM
gnn closed this revision.Oct 27 2015, 12:49 AM

committed as rS290028

eri added a subscriber: eri.Oct 28 2015, 6:38 PM

I know this has been committed but please check my remarks.

sys/netinet/ip_ipsec.c
165

Shouldn't this be direction OUT?

gnn added inline comments.Oct 29 2015, 8:41 PM
sys/netinet/ip_ipsec.c
165

Good catch! Fixing now.

ae edited edge metadata.Aug 22 2016, 3:41 PM

I discovered that this change breaks 'net.inet.ipsec.def_policy' handling. If you set default_policy to "DISCARD", it will not work until you add some security policies to SPD. Don't know how it is useful or popular, but this probably should be mentioned somewhere.