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.
Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 913 Build 913: arc lint + arc unit
Event Timeline
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.
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.
I know this has been committed but please check my remarks.
sys/netinet/ip_ipsec.c | ||
---|---|---|
165 | Shouldn't this be direction OUT? |
sys/netinet/ip_ipsec.c | ||
---|---|---|
165 | Good catch! Fixing now. |
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.