Page MenuHomeFreeBSD

Add compile-time option to activate hhook(9) framework for TCP
ClosedPublic

Authored by jtl on Oct 7 2016, 6:32 PM.

Details

Summary

Presently, I believe we incur a function call, indirect lookup, and comparison for each incoming TCP frame. We incur an indirect lookup and comparison for each outgoing TCP frame.

(And, as soon as an alternate stack creates its own tcp_output() equivalent, we will probably also incur the overhead of a function call for outgoing packets. Of course, compilers may optimize away the function call, but the rest of the overhead still remains.)

This change adds a new compile-time option (TCP_HHOOK) to determine whether to support the hhook(9) framework for TCP. To retain backwards compatibility, I added it to every configuration file that already defined "options INET". (Therefore, this patch introduces no functional change. In order to see a functional difference, you need to compile a custom kernel without the TCP_HHOOK option.) However, I suspect that some of the platforms that have excluded INET6 support due to resource limitations may want to consider removing this option from the default kernel.

In the TCP stack, the hhook(9) framework provides hooks for kernel modules to add actions that run when a TCP frame is sent or received on a TCP session in the ESTABLISHED state. In the base tree, this functionality is only used for the h_ertt module, which is used by the cc_cdg, cc_chd, cc_hd, and cc_vegas congestion control modules.

Note that any user who uses those congestion control modules and uses a custom kernel configuration will need to add the TCP_HHOOK option to their kernel configuration.

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

jtl updated this revision to Diff 21161.Oct 7 2016, 6:32 PM
jtl retitled this revision from to Add compile-time option to activate hhook(9) framework for TCP.
jtl updated this object.
jtl edited the test plan for this revision. (Show Details)
jtl added a reviewer: rrs.
jtl added a reviewer: lstewart.Oct 7 2016, 6:39 PM
jtl updated this object.Oct 7 2016, 6:42 PM
rrs accepted this revision.Oct 7 2016, 7:04 PM
rrs edited edge metadata.

Looks like a great idea :-)

This revision is now accepted and ready to land.Oct 7 2016, 7:04 PM
hiren accepted this revision.Oct 7 2016, 7:11 PM
hiren added a reviewer: hiren.
hiren added a subscriber: hiren.

You may want to briefly list pros/cons of having or not having the framework so people can decide whether its worth including it.

jtl updated this revision to Diff 21177.Oct 7 2016, 11:49 PM
jtl edited edge metadata.

This is updated to remove the OSD allocation and setup from the TCPCB.

The only thing using this OSD memory was the hhook(9) framework. The only things using the hhook(9) framework were the h_ertt module, and the congestion-control modules that depend on it. The h_ertt module should fail to load if the hooks are not available. And, the congestion control modules appear to have proper dependencies to prevent them from loading if h_ertt is not available.

This revision now requires review to proceed.Oct 7 2016, 11:49 PM
jtl updated this object.Oct 7 2016, 11:56 PM
jtl edited edge metadata.
In D8185#169869, @hiren wrote:

You may want to briefly list pros/cons of having or not having the framework so people can decide whether its worth including it.

Good idea! I've updated the description to make this more clear.

jtl updated this revision to Diff 21178.Oct 7 2016, 11:59 PM

Added a note to UPDATING about the congestion control modules that depend on the new option.

jtl updated this revision to Diff 21236.Oct 10 2016, 8:15 PM
jtl edited edge metadata.

If TCP_HHOOK is not defined, we should not default to compiling the modules that rely on this functionality.

rrs accepted this revision.Oct 10 2016, 8:59 PM
rrs edited edge metadata.
This revision is now accepted and ready to land.Oct 10 2016, 8:59 PM
sjg added a subscriber: sjg.Oct 10 2016, 9:58 PM
sjg added inline comments.
sys/modules/cc/Makefile
12 ↗(On Diff #21236)

${OPT_INET:MTCP_HHOOK} will be "TCP_HHOOK" or ""
If you had a '*' in there, checking for it being something other than TCP_HHOOK might make sense.

I think we have abandoned supporting building with old fmake, so you could make this more readable. Ie. build if ALL_MODULES or OPT_INET contains TCP_HHOOK

.if defined(ALL_MODULES) || ${OPT_INET:U:MTCP_HHOOK} != ""

the following might be closer to what the comment says, ie. if you wanted to build it if ALL_MODULES or OPT_INET is undefined or contains TCP_HHOOK

.if defined(ALL_MODULES) || ${OPT_INET:UTCP_HHOOK:MTCP_HHOOK} != ""

lstewart accepted this revision.Oct 12 2016, 1:41 AM
lstewart edited edge metadata.
lstewart added inline comments.
sys/netinet/tcp_subr.c
1229 ↗(On Diff #21236)

Not needed given that we pass M_ZERO to uma_zalloc()

jtl marked an inline comment as done.Oct 12 2016, 2:04 AM
jtl added inline comments.
sys/netinet/tcp_subr.c
1229 ↗(On Diff #21236)

Good catch. Thanks!

This revision was automatically updated to reflect the committed changes.
jtl marked an inline comment as done.