Page MenuHomeFreeBSD

Update Rack and BBR to the latest from NF
ClosedPublic

Authored by rrs on Apr 26 2020, 3:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 20, 11:18 PM
Unknown Object (File)
Tue, Apr 9, 4:41 AM
Unknown Object (File)
Mar 25 2024, 9:14 PM
Unknown Object (File)
Mar 21 2024, 1:08 PM
Unknown Object (File)
Mar 21 2024, 1:04 PM
Unknown Object (File)
Mar 21 2024, 1:04 PM
Unknown Object (File)
Mar 21 2024, 1:04 PM
Unknown Object (File)
Mar 21 2024, 1:04 PM
Subscribers

Details

Summary

This patch updates the Rack and BBR to the latest version from NF

Test Plan

Run TCP regression tests using pkt_drill making sure the non-timing related issues do not fail.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

rrs requested review of this revision.Apr 26 2020, 3:15 PM
rrs created this revision.
rrs added reviewers: tuexen, transport.
bbr.c
4839

I guess you want to keep KMOD_TCPSTAT_INC here, since this is a kernel module.

4856

I guess you want to keep KMOD_TCPSTAT_INC here, since this is a kernel module.

4929

I guess you want to keep KMOD_TCPSTAT_INC here, since this is a kernel module.

rack_bbr_common.c
919

You could use bool as the type for nolog, and use true and false when calling it.
Maybe it is simpler to avoid the negation and use bool log as the parameter, swap true and false when calling and use

			if (log)
				tcp_log_end_status(tp, TCP_EI_STATUS_PROGRESS);

instead of

			if (!nolog)
				tcp_log_end_status(tp, TCP_EI_STATUS_PROGRESS);
tcp_bbr.h
2

Maybe use 2016-2020?

3

I think it is Warner removing the All rights reserved. So are you adding it intentionally?

tcp_rack.h
5

Maybe use 2016-2020?

Maybe make this header and the one used in tcp_bbr.h consistent? Add the SPDX there too...

rack.c
477

Do you intentionally remove uint8_t iptos, which was added by Richard for future ECN support?

481

Do you intentionally remove uint8_t iptos, which was added by Richard for future ECN support?

485

Do you intentionally remove uint8_t iptos, which was added by Richard for future ECN support?

489

Do you intentionally remove uint8_t iptos, which was added by Richard for future ECN support?

493

Do you intentionally remove uint8_t iptos, which was added by Richard for future ECN support?

497

Do you intentionally remove uint8_t iptos, which was added by Richard for future ECN support?

501

Do you intentionally remove uint8_t iptos, which was added by Richard for future ECN support?

505

Do you intentionally remove uint8_t iptos, which was added by Richard for future ECN support?

509

Do you intentionally remove uint8_t iptos, which was added by Richard for future ECN support?

636

Can you provide a better description? What does 3 mean? The text read like you are describing a boolean...

678

Text reads like a boolean, but value isn't?

785

Text reads like a boolean, but value isn't...

790

Text reads like a boolean, but value isn't...

795

Text reads like a boolean, but value isn't... Double ? at the end?

823

Why ? at the end?

828

Why ? at the end?

833

Why ? at the end?

838

Why ? at the end?

843

Why ? at the end?

848

Why ? at the end?

853

Why ? at the end?

858

Why ? at the end?

863

Why ? at the end?

868

Why ? at the end?

883

The text reads like a boolean. but the value isn't.

913

Why ? at the end?

991

What instead of what?

1051

No double space.

1160

Just picking one: Where is it freed? I guess they are leaked on unloading the module, or am I missing something?

1177

SACKs instead of SACK's?

bbr.c
4839

yep this was probably not upstream when you did the update.

4856

yep

4929

yep

rack.c
477

No not intentionally. The problem was that our rack lived for a time in parallel rack.c and rack_pacing.c , which means
merges were painful since we had to catch all the changes going in from upstream in rack.c and copy them over to rack_pacing.c
I must have missed that one. A release or two ago rack_pacing.c became rack.c

I will add it back though it looks like it is unused.

481

see above

485

see above

489

see above

493

see above

497

see above

501

see above

505

see above

509

see above

636

How about
What is the maximum number of full TLS records that will be sent at once

678

Maybe:
If not zero, provides a maximum usecond that you can stay in probertt (2sec = 2000000)
would make ie clearer?

785

For these three how about

If non zero, what percentage of goodput to pace at in <xxx>

where xxx is recovery, congestion avoidance or slow start

823

I have removed all sysctl terminating ? marks.

I always more or less thought of this as asking a question but I see that probably is a bit weird ... but so am I ;-)

883

Maybe
Rack timely what threshold do we count to before another boost during b/w decent
would be more clear?

991

ok

1051

A comment perhaps then i.e.
/* Measure controls */

1160

Good question, bbr may also have the same issue. Let me check.

Yep and there were two in BBR.

1177

ok

rack_bbr_common.c
919

ok

tcp_rack.h
5

I will fix this so that all of them have the correct statements... (all consistently the same)

Address all of Michael's first set of comments.

rscheff added inline comments.
rack.c
477

I prepared the iptos stuff for adding in AccECN support later (intially only the handshake, and some simple tie-in with existing ECN reactions).

Obviously, if you have a stack (BBRv1) which is not supporting ECN in the first place, this could be reverted.

D21011 has the initial AccECN handshake code, but once the draft stabilizes after being promoted to PS (and addressing a number of edge cases), I will have to update that diff agian.

And yes, in many places I don't actually need access to iptos (but because of the callout table, I had to add it to all function headers). If you have a more effective way to get access in all the SYN- related states only, that would work for me.

Lets go ahead and add the new function to return that we don't do PRUS_OOB as long as we are in fixing things.

I would suggest leaving the iptos changes in. It does not
harm in BBR and someday we will be doing BBRv2 and in
that we will do ECN.. so best to leave it in place I think.

Even though there are no plans to add ECN to BBRv1, lets go ahead
and add back the uint8_t iptos value since you never know we might
circle back and do this plus we also want to remain somewhat consistent
between BBR and Rack

Follow Michaels suggestion and go 2016 - 2020 in the copyright instead of 2016 - 20

tcp_bbr.h
2

I have them all -20 but yeah I like 2020 better.. I was just following other files.

This revision is now accepted and ready to land.May 2 2020, 10:22 PM