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.
Tags
None
Referenced Files
Unknown Object (File)
Jan 2 2024, 4:11 PM
Unknown Object (File)
Dec 30 2023, 5:22 AM
Unknown Object (File)
Dec 20 2023, 4:45 AM
Unknown Object (File)
Dec 1 2023, 7:16 AM
Unknown Object (File)
Nov 30 2023, 7:53 PM
Unknown Object (File)
Nov 18 2023, 11:33 PM
Unknown Object (File)
Nov 18 2023, 10:27 PM
Unknown Object (File)
Nov 18 2023, 8:46 PM

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

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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.

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.

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
360
tv.tv_sec = m->m_pkthdr.rcv_tstmp / 1000000000;

instead of

tv.tv_sec = m->m_pkthdr.rcv_tstmp /1000000000;
361
tv.tv_usec = (m->m_pkthdr.rcv_tstmp % 1000000000) / 1000;

instead of

tv.tv_usec = (m->m_pkthdr.rcv_tstmp % 1000000000)/1000;
376
while (m) {

instead of

while(m)
385
return (retval);

instead of

return(retval);
393
return (retval);

instead of

return(retval);
408
return (1);

instead of

return(1);
417
return (tp->snd_max - tp->snd_una);

instead of

return(tp->snd_max - tp->snd_una);
424
return (ctf_outstanding(tp) - rc_sacked);

instead of

return(ctf_outstanding(tp) - rc_sacked);
858
return (decayed_count);

instead of

return(decayed_count);
This revision is now accepted and ready to land.Jul 3 2019, 12:20 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

Address phabricator nits

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
44

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

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

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

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

This revision now requires changes to proceed.Jul 6 2019, 1:16 PM

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

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

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