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.

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

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

rrs created this revision.Apr 9 2018, 5:11 PM
jtl accepted this revision.Apr 10 2018, 6:41 PM

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
rrs added inline comments.Apr 10 2018, 8:26 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

rrs added inline comments.Apr 10 2018, 8:28 PM
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.

jtl added inline comments.Apr 10 2018, 10:45 PM
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.

rrs added inline comments.Apr 11 2018, 6:15 AM
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?

rrs updated this revision to Diff 41357.Apr 11 2018, 6:47 AM

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
rrs updated this revision to Diff 41376.Apr 11 2018, 7:03 PM

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

hiren added a subscriber: hiren.Apr 11 2018, 8:59 PM

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 ↗(On Diff #41376)

Can you please add comments next to 2 entries above explaining what they do?

rrs added a comment.Apr 12 2018, 11:50 AM

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 ↗(On Diff #41376)

I will see what I can do :)

rrs updated this revision to Diff 41400.Apr 12 2018, 12:25 PM

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.

hiren accepted this revision as: hiren.Apr 12 2018, 5:40 PM

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

Style bug here. <tab> after #define

imp added a comment.Apr 30 2018, 5:37 PM

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

head/sys/sys/kern_prefetch.h
3

full years only. 2016-2018

43

Why does amd64 need a special thing here?

ae added a subscriber: ae.May 18 2018, 1:15 PM
ae added inline comments.
head/sys/netinet/tcp_hpts.c
172

This seems redundant, from a quick look I don't see anything that may require these headers. And opt_ipsec.h also is unneeded.