Page MenuHomeFreeBSD

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

Authored by jtl on Oct 7 2016, 6:32 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 14, 8:04 AM
Unknown Object (File)
Sun, Apr 14, 2:10 AM
Unknown Object (File)
Tue, Apr 9, 7:53 AM
Unknown Object (File)
Tue, Mar 26, 6:54 PM
Unknown Object (File)
Feb 23 2024, 10:45 PM
Unknown Object (File)
Feb 3 2024, 4:11 PM
Unknown Object (File)
Jan 19 2024, 5:02 AM
Unknown Object (File)
Jan 15 2024, 6:20 AM

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 - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5508
Build 5733: arc lint + arc unit

Event Timeline

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.
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 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 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 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.

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

jtl edited edge metadata.

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

rrs edited edge metadata.
This revision is now accepted and ready to land.Oct 10 2016, 8:59 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 edited edge metadata.
lstewart added inline comments.
sys/netinet/tcp_subr.c
1229

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

Good catch. Thanks!

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