Page MenuHomeFreeBSD

First step in bring hpts and infrastructure up for bbr v1 update part1
ClosedPublic

Authored by rrs on Jul 2 2019, 10:32 AM.

Details

Summary

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.

Test Plan

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

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.Jul 2 2019, 10:32 AM

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.

rrs updated this revision to Diff 59311.Jul 2 2019, 12:50 PM
rrs updated this revision to Diff 59312.Jul 2 2019, 1:00 PM

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.

tuexen accepted this revision.Jul 3 2019, 12:20 PM

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

NUM_OF_HPTSI_SLOTS / 4 instead of NUM_OF_HPTSI_SLOTS/4.

621 ↗(On Diff #59312)
return (tick_now - prev_tick);

instead of

return(tick_now - prev_tick);
627 ↗(On Diff #59312)

NUM_OF_HPTSI_SLOTS - 1 instead of NUM_OF_HPTSI_SLOTS-1

629 ↗(On Diff #59312)
return ((NUM_OF_HPTSI_SLOTS - prev_tick) + tick_now);

instead of

return((NUM_OF_HPTSI_SLOTS - prev_tick) + tick_now);
694 ↗(On Diff #59312)
return (NUM_OF_HPTSI_SLOTS - dis_to_travel);

stead of

return(NUM_OF_HPTSI_SLOTS - dis_to_travel);
895 ↗(On Diff #59312)

Why some many whitespaces between else and if?

925 ↗(On Diff #59312)
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
360 ↗(On Diff #59312)
tv.tv_sec = m->m_pkthdr.rcv_tstmp / 1000000000;

instead of

tv.tv_sec = m->m_pkthdr.rcv_tstmp /1000000000;
361 ↗(On Diff #59312)
tv.tv_usec = (m->m_pkthdr.rcv_tstmp % 1000000000) / 1000;

instead of

tv.tv_usec = (m->m_pkthdr.rcv_tstmp % 1000000000)/1000;
376 ↗(On Diff #59312)
while (m) {

instead of

while(m)
385 ↗(On Diff #59312)
return (retval);

instead of

return(retval);
393 ↗(On Diff #59312)
return (retval);

instead of

return(retval);
408 ↗(On Diff #59312)
return (1);

instead of

return(1);
417 ↗(On Diff #59312)
return (tp->snd_max - tp->snd_una);

instead of

return(tp->snd_max - tp->snd_una);
424 ↗(On Diff #59312)
return (ctf_outstanding(tp) - rc_sacked);

instead of

return(ctf_outstanding(tp) - rc_sacked);
858 ↗(On Diff #59312)
return (decayed_count);

instead of

return(decayed_count);
This revision is now accepted and ready to land.Jul 3 2019, 12:20 PM
rrs updated this revision to Diff 59341.Jul 3 2019, 12:59 PM

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.

This revision now requires review to proceed.Jul 3 2019, 12:59 PM
rrs updated this revision to Diff 59342.Jul 3 2019, 1:08 PM

Address phabricator nits

tuexen accepted this revision.Jul 3 2019, 1:58 PM
This revision is now accepted and ready to land.Jul 3 2019, 1:58 PM
tuexen requested changes to this revision.Jul 3 2019, 8:01 PM

The already removed #include "opt_kern_tls.h" found its way back and breaks compilation.

netinet/tcp_stacks/rack_bbr_common.c
43 ↗(On Diff #59342)

Including opt_kern_tls.h breaks compilation of head if RACK is enabled.

This revision now requires changes to proceed.Jul 3 2019, 8:01 PM
rrs updated this revision to Diff 59366.Jul 3 2019, 8:09 PM

It helps if one remembers to bring the changes of the .h file too ... opps

rrs updated this revision to Diff 59367.Jul 3 2019, 8:12 PM

And we can't loose the commented out opt_kern_tls until the kern tls hits the tree

tuexen requested changes to this revision.Jul 6 2019, 1:16 PM

tcp_input_data needs some tweaks to work in kernels with options VIMAGE in its configuration file.

netinet/tcp_hpts.c
1239 ↗(On Diff #59367)

You haven't called CURVNET_SET() yet. So I guess you need a similar dance like you do in tcp_hptsi() here.

1333 ↗(On Diff #59367)

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

This revision now requires changes to proceed.Jul 6 2019, 1:16 PM
swills added a subscriber: swills.Jul 8 2019, 9:28 PM
koobs added a subscriber: koobs.Jul 10 2019, 10:06 AM
rrs updated this revision to Diff 59597.Jul 10 2019, 11:00 AM

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

rrs updated this revision to Diff 59602.Jul 10 2019, 3:20 PM

Add in the hopeless exit to tcp_hpts and also get the corrected (with DSACK patch) rack_18q22p2

tuexen accepted this revision.Jul 10 2019, 7:46 PM

I tested this on a slow system with and without VIMAGE. It works now also on these platforms.

This revision is now accepted and ready to land.Jul 10 2019, 7:46 PM
This revision was automatically updated to reflect the committed changes.