This patch updates the Rack and BBR to the latest version from NF
Details
- Reviewers
tuexen - Group Reviewers
transport - Commits
- rS360639: This commit brings things into sync with the advancements that
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
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. 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 | ||
1–2 | Maybe use 2016-2020? | |
1–2 | 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 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 | |
678 | Maybe: | |
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 | |
991 | ok | |
1051 | A comment perhaps then i.e. | |
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) |
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
tcp_bbr.h | ||
---|---|---|
1–2 | I have them all -20 but yeah I like 2020 better.. I was just following other files. |