Page MenuHomeFreeBSD

Add of high-precision-timer-system used by rack and bbr for pacing. Also sync up the function block differences needed by bbr/rack.
ClosedPublic

Authored by rrs on Apr 9 2018, 5:11 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 18, 6:39 AM
Unknown Object (File)
Sun, Mar 17, 5:12 AM
Unknown Object (File)
Jan 31 2024, 5:08 AM
Unknown Object (File)
Jan 31 2024, 5:04 AM
Unknown Object (File)
Jan 31 2024, 5:04 AM
Unknown Object (File)
Jan 29 2024, 11:00 AM
Unknown Object (File)
Dec 15 2023, 11:51 AM
Unknown Object (File)
Nov 30 2023, 4:40 PM

Details

Summary

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.

Test Plan

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

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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.

This revision is now accepted and ready to land.Apr 10 2018, 6:41 PM
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
doing it in the other branch I have open in our git repo.. if so we
will push it back at that time.

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
you have inet6.

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
netinet/tcp_sack.c optional inet | inet6
netinet/tcp_subr.c optional inet | inet6
netinet/tcp_syncache.c optional inet | inet6
netinet/tcp_timer.c optional inet | inet6
netinet/tcp_timewait.c optional inet | inet6
netinet/tcp_usrreq.c optional inet | inet6
netinet/udp_usrreq.c optional inet | inet6

Why not just optional inet?

This address Gleb and Jonathans comments and also now passes a build universe

This revision now requires review to proceed.Apr 11 2018, 6:47 AM

After jtl and I chatted I finally understand his request (just a bit thick I guess) so
now I wrap up his final comment.

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?

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 :)

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.

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!

This revision is now accepted and ready to land.Apr 12 2018, 5:40 PM
This revision was automatically updated to reflect the committed changes.
hselasky added inline comments.
head/sys/sys/mbuf.h
203 ↗(On Diff #41651)

Style bug here. <tab> after #define

just a couple if nits. I've not looked closely at the TCP side of things.

head/sys/sys/kern_prefetch.h
3 ↗(On Diff #41651)

full years only. 2016-2018

43 ↗(On Diff #41651)

Why does amd64 need a special thing here?

ae added inline comments.
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.