So this is the first step in bring the Netflix version of BBRv1 into FreeBSD. We
have been working on it for quite some time. Also we will also bring in
an updated rack which better addresses the Sack Attack issue as
well as brings it into parity to what NF has in its current release.
Details
Details
- Reviewers
tuexen - Group Reviewers
transport - Commits
- rS362880: MFC r349893:
rS349893: This commit updates rack to what is basically being used at NF as
Tests can have been done extensively with this code
running lots of connections both with Rack and BBR.
The catch is you have to have these and a few other
things updated to run the latest bbr/rack to excercise.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
With removing these four header files, I can compile and use RACK on a head system.
| sys/netinet/tcp_stacks/rack_bbr_common.c | ||
|---|---|---|
| 43 ↗ | (On Diff #59310) | Including opt_kern_tls.h breaks compilation of head with RACK enabled. |
| 53 ↗ | (On Diff #59310) | Including sys/qmath.h breaks compilation of head with RACK enabled. |
| 62 ↗ | (On Diff #59310) | Including sys/stats.h breaks compilation of head with RACK enabled. |
| 69 ↗ | (On Diff #59310) | Including sys/tim_filter.h breaks compilation of head with RACK enabled. |
Comment Actions
So interestingly those includes are not needed.. not even sure the tls one is need (hwtls should be in the tree soon).. however more
interesting is why my freebsd head build works.. not sure why since it does not have hwtls in the mix.. need to go check whats going
on in my conf.
Comment Actions
You might want to address the nits. All of them are whitespace issue.
I tested this and this version also fixes the issue I observed which was also addressed by D20110.
| netinet/tcp_hpts.c | ||
|---|---|---|
| 268 | NUM_OF_HPTSI_SLOTS / 4 instead of NUM_OF_HPTSI_SLOTS/4. | |
| 621 | return (tick_now - prev_tick); instead of return(tick_now - prev_tick); | |
| 627 | NUM_OF_HPTSI_SLOTS - 1 instead of NUM_OF_HPTSI_SLOTS-1 | |
| 629 | return ((NUM_OF_HPTSI_SLOTS - prev_tick) + tick_now); instead of return((NUM_OF_HPTSI_SLOTS - prev_tick) + tick_now); | |
| 694 | return (NUM_OF_HPTSI_SLOTS - dis_to_travel); stead of return(NUM_OF_HPTSI_SLOTS - dis_to_travel); | |
| 895 | Why some many whitespaces between else and if? | |
| 925 | if (have_slept < hpts->p_hpts_sleep_time) instead of if(have_slept < hpts->p_hpts_sleep_time) | |
| netinet/tcp_stacks/rack_bbr_common.c | ||
| 361 | tv.tv_sec = m->m_pkthdr.rcv_tstmp / 1000000000; instead of tv.tv_sec = m->m_pkthdr.rcv_tstmp /1000000000; | |
| 362 | tv.tv_usec = (m->m_pkthdr.rcv_tstmp % 1000000000) / 1000; instead of tv.tv_usec = (m->m_pkthdr.rcv_tstmp % 1000000000)/1000; | |
| 377 | while (m) {instead of while(m) | |
| 386 | return (retval); instead of return(retval); | |
| 394 | return (retval); instead of return(retval); | |
| 409 | return (1); instead of return(1); | |
| 418 | return (tp->snd_max - tp->snd_una); instead of return(tp->snd_max - tp->snd_una); | |
| 425 | return (ctf_outstanding(tp) - rc_sacked); instead of return(ctf_outstanding(tp) - rc_sacked); | |
| 859 | return (decayed_count); instead of return(decayed_count); | |
Comment Actions
In my detailed testing of hpts I found we are *not* doing the right thing as to calculating
the time for sleep. The code was starting at where the prev_slot was and moving
through counting the slots until it found a non-empty slot.
But it should start that loop where it last stopped plus 1. This is cur_slot + 1. That
way its sleep will be correct. The effect of this is sometimes (when we have
few clients on the wheel) we sleep too long so the client will be overly late.
Comment Actions
The already removed #include "opt_kern_tls.h" found its way back and breaks compilation.
| netinet/tcp_stacks/rack_bbr_common.c | ||
|---|---|---|
| 43 | Including opt_kern_tls.h breaks compilation of head if RACK is enabled. | |
Comment Actions
tcp_input_data needs some tweaks to work in kernels with options VIMAGE in its configuration file.
| netinet/tcp_hpts.c | ||
|---|---|---|
| 1239 | You haven't called CURVNET_SET() yet. So I guess you need a similar dance like you do in tcp_hptsi() here. | |
| 1333 | You have already called CURVNET_RESTORE(). So I guess you need a similar dance like you do in tcp_hptsi() here. Please note that in addition to the two places I mentioned, you need changes in the loop... | |
Comment Actions
Update to include some fixes Michael and I have been working on in a slow slow machine as well
as bring the Rack stack up to the latest production (with some small changes to make it compile).
Comment Actions
Add in the hopeless exit to tcp_hpts and also get the corrected (with DSACK patch) rack_18q22p2
Comment Actions
I tested this on a slow system with and without VIMAGE. It works now also on these platforms.