This adds the tcp_hpts (formerly called the tcp_pacer in NF land). It also adds
missing function block pointers and such that NF has added so that rack and bbr
can be made to work properly and handle switching back and forth.
Details
Details
Basically after building universe (which as yet to be done) I will bring in a rack
module and build it with the new HPTS option so that I can then validate via
file transfer and testing that I have not broken anything during the bring over
from the NF world. Note that without adding a new option the hpts system
does not get added in nor can any new tcp stack that uses it compile or run.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
See nits inline.
FWIW, I accept this, although I'll admit I'm biased since I've reviewed interim drops over the course of months (more than a year?), so may not have the same perspective as someone approaching this from a fresh perspective.
| conf/options | ||
|---|---|---|
| 144 ↗ | (On Diff #41293) | Why not opt_inet.h? |
| netinet/tcp_hpts.c | ||
| 2 ↗ | (On Diff #41293) | SPDX tag. Also, bump the copyright to 2018. |
| 1775 ↗ | (On Diff #41293) | As a future feature (NOT FOR THIS COMMIT), it would be nice to be able to specify the number of threads and have it be less/more than the number of CPUs. In particular, I suspect there are some platforms where they would work better having one thread per real core, rather than one thread per hyper thread core. |
| netinet/tcp_hpts.h | ||
| 4 ↗ | (On Diff #41293) | SPDX tag. Bump the copyright to 2018. |
| sys/kern_prefetch.h | ||
| 1 ↗ | (On Diff #41293) | Needs a copyright/license/SPDX tag. |
| conf/options | ||
|---|---|---|
| 144 ↗ | (On Diff #41293) | Because I did not think of it .. I will make it go there though.. much better I can get rid of all the new opt_xxx.h |
| netinet/tcp_hpts.c | ||
| 2 ↗ | (On Diff #41293) | ack |
| 1775 ↗ | (On Diff #41293) | Yeah, Drew and I have chatted about doing that. We may end up |
| netinet/tcp_hpts.h | ||
| 4 ↗ | (On Diff #41293) | ack |
| sys/kern_prefetch.h | ||
| 1 ↗ | (On Diff #41293) | ack |
| conf/options | ||
|---|---|---|
| 144 ↗ | (On Diff #41293) | Oh wait. I know why.. that would require V4.. and not allow you to have the hpts system in when |
| conf/options | ||
|---|---|---|
| 144 ↗ | (On Diff #41293) | That might be what the name implies, but that's not really the case. By convention, opt_inet.h gets both IPv4-specific options, as well as IPv4/IPv6 generic options. See, for example, TCP_HHOOK, TCP_OFFLOAD, and TCP_RFC7413. |
| conf/options | ||
|---|---|---|
| 144 ↗ | (On Diff #41293) | Then why do we have: netinet/tcp_reass.c optional inet | inet6 Why not just optional inet? |
Comment Actions
After jtl and I chatted I finally understand his request (just a bit thick I guess) so
now I wrap up his final comment.
Comment Actions
Thanks for this work!
I can see some good comments in the code but if there is a general design doc or excerpt on pacer, it'd be nice to add that in the beginning of tcp_hpts.c
AFAIK, rack is not yet in -HEAD. Would default stack be able to use this functionality? Probably a manpage update to indicate how a stack could use pacing?
| sys/netinet/tcp_var.h | ||
|---|---|---|
| 94 | Can you please add comments next to 2 entries above explaining what they do? | |
Comment Actions
Hiren:
Thanks for looking at this. Note that rack is not in yet because it lacks this commit. This is the
first commit in the steps to get rack that we use at NF for 99% of traffic into the FreeBSD core code.
These have to go in first :)
As to design documents I am not much of one to put a design document in the .c file. Not sure
if I have one (I will go look). If I do I would be more inclined to put it into either a readme or manual page.
Let me think about that... however I think it will have to be a follow on, since it will take some time to
write that and I want to get this in so I can get Rack and BBR in.
| sys/netinet/tcp_var.h | ||
|---|---|---|
| 94 | I will see what I can do :) | |
Comment Actions
This adds the comments that Hiren had asked for and I even
add a comment block at the top of tcp_hpts.c to talk about usage
(not a design document but some hints on use). Note also Hiren
that the hpts system can be used by the default freebsd stack. Our
internal NF default stack actually has some of these hooks in it as
well so we can enable "pacing" with various options.
Again we can't get these changes into FreeBSD (if folks want them) until
the tcp_hpts system is in.
Comment Actions
Thanks a lot for the usage writeup. And yep, I do understand that we need to get this in so RACK/BBR can follow. Good that this can be used with default stack so there is some way to test/use this functionality before RACK/BBR comes in.
Again, this is a huge effort and thanks for your work!
| head/sys/sys/mbuf.h | ||
|---|---|---|
| 203 ↗ | (On Diff #41651) | Style bug here. <tab> after #define |
| head/sys/netinet/tcp_hpts.c | ||
|---|---|---|
| 172 ↗ | (On Diff #41651) | This seems redundant, from a quick look I don't see anything that may require these headers. And opt_ipsec.h also is unneeded. |