Refactor for pluggable congestion control algorithtm #1949

Wizmann · 2021-08-29T14:16:37Z

Refactor the code to support pluggable congestion control algorithm.

src/core/congestion_control.h

src/inc/msquic.h

nibanks · 2021-08-29T17:18:17Z

/azp run

azure-pipelines · 2021-08-29T17:18:35Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks · 2021-08-30T15:53:16Z

/azp run

azure-pipelines · 2021-08-30T15:53:35Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks · 2021-08-31T14:45:18Z

/azp run

azure-pipelines · 2021-08-31T14:45:36Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks · 2021-08-31T15:00:59Z

For the CLOG stuff, you should just need to run the update-sidecar.ps1 script.

nibanks · 2021-08-31T15:34:05Z

Windows build seems to be spitting out a lot of errors too:

D:\a\1\msquic\src\core\cubic.h(84,1): warning C4013: 'FIELD_SIZE' undefined; assuming extern returning int (compiling source file D:\a\1\msquic\src\core\ack_tracker.c) [D:\a\1\msquic\build\windows\x86_schannel\src\core\core.vcxproj]
D:\a\1\msquic\src\core\cubic.h(86,1): error C2275: 'QUIC_CONGESTION_CONTROL': illegal use of this type as an expression (compiling source file D:\a\1\msquic\src\core\ack_tracker.c) [D:\a\1\msquic\build\windows\x86_schannel\src\core\core.vcxproj]

nibanks · 2021-09-01T15:27:51Z

/azp run

azure-pipelines · 2021-09-01T15:29:03Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks · 2021-09-01T16:20:54Z

/azp run

azure-pipelines · 2021-09-01T16:21:14Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks · 2021-09-01T16:44:04Z

One build error:

/home/vsts/work/1/s/src/core/cubic.c:125:1: error: redundant 'CubicCongestionControlFinalize' declaration [readability-redundant-declaration,-warnings-as-errors]
CubicCongestionControlFinalize(
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/vsts/work/1/s/src/core/cubic.h:120:1: note: previously declared here
CubicCongestionControlFinalize(
^

src/core/congestion_control.h

src/core/inline.c

src/core/send_buffer.c

nibanks · 2021-09-02T01:26:51Z

/azp run

azure-pipelines · 2021-09-02T01:27:10Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks · 2021-09-02T11:16:56Z

/azp run

azure-pipelines · 2021-09-02T11:17:17Z

Azure Pipelines successfully started running 1 pipeline(s).

src/core/congestion_control.h

src/core/cubic.h

src/core/quicdef.h

nibanks · 2021-09-03T13:45:34Z

/azp run

azure-pipelines · 2021-09-03T13:45:53Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks · 2021-09-03T15:04:14Z

/azp run

azure-pipelines · 2021-09-03T15:04:33Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks · 2021-09-03T15:06:19Z

src/core/cubic_impl.h

@@ -0,0 +1,106 @@
+/*++
+
+    Copyright (c) Microsoft Corporation.


Why do we need a separate file for this?

We need to seperate the definition of QUIC_CONGESTION_CONTROL_CUBIC and all cubic functions so that:

We can include cubic.h in congestion_control.h for using QUIC_CONGESTION_CONTROL_CUBIC in union QUIC_CONGESTION_CONTROL_CONTEXT. This union is to find the max context size for all congestion control algorithms.

To avoid circular reference for cubic_impl.h and congestion_control.h.

nibanks · 2021-09-03T15:06:59Z

src/core/cubic_impl.c

+
+#include "precomp.h"
+#ifdef QUIC_CLOG
+#include "cubic_impl.c.clog.h"


Why not just cubic.c?

nibanks · 2021-09-03T15:41:14Z

Windows kernel build failed. I think you missed some project file updates for file name changes.

ClCompile:
  C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\bin\HostX86\x64\CL.exe /c /I..\inc /ID:\a\1\msquic\build\winkernel\x64_Release_schannel\inc /ID:\a\1\msquic\build\winkernel\x64_Release_schannel\obj\core.kernel\ /ID:\a\1\msquic\build\winkernel\x64_Release_schannel\obj\core.kernel\ /Zi /nologo /W4 /WX /diagnostics:column /MP /Ox /Ot /Oy- /GL /D CODE_ANALYSIS /D QUIC_EVENTS_MANIFEST_ETW /D QUIC_LOGS_MANIFEST_ETW /D QUIC_TELEMETRY_ASSERTS=1 /D CXPLAT_TLS_SECRETS_SUPPORT=1 /D SECURITY_KERNEL /D SECURITY_WIN32 /D _WIN64 /D _AMD64_ /D AMD64 /D _WIN32_WINNT=0x0A00 /D WINVER=0x0A00 /D WINNT=1 /D NTDDI_VERSION=0x0A000008 /D KMDF_VERSION_MAJOR=1 /D KMDF_VERSION_MINOR=15 /GF /Gm- /Zp8 /GS /guard:cf /Gy /fp:precise /Qspectre /Zc:wchar_t- /Zc:forScope /Zc:inline /GR- /Fo"D:\a\1\msquic\build\winkernel\x64_Release_schannel\obj\core.kernel\\" /Fd"D:\a\1\msquic\build\winkernel\x64_Release_schannel\bin\core.pdb" /external:W4 /Gz /wd4603 /wd4627 /wd4986 /wd4987 /FI"C:\Program Files (x86)\Windows Kits\10\Include\10.0.19041.0\shared\warning.h" /analyze /analyze:projectdirectory"D:\a\1\msquic\src\core" /analyze:rulesetdirectory";C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\Team Tools\Static Analysis Tools\\Rule Sets;" /analyze:ruleset"C:\Program Files (x86)\Windows Kits\10\CodeAnalysis\DriverMinimumRules.ruleset" /analyze:quiet /analyze:stacksize1024 /analyze:plugin"C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\bin\HostX86\x86\EspXEngine.dll" /analyze:plugin"C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x86\WindowsPrefast.dll" /analyze:plugin"C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x86\drivers.dll" /FC /errorReport:queue /Gw /kernel /ZH:SHA_256 -d2jumptablerdata -d2epilogunwindrequirev2 ack_tracker.c api.c binding.c configuration.c congestion_control.c connection.c crypto.c crypto_tls.c cubic_impl.c datagram.c frame.c injection.c library.c listener.c lookup.c loss_detection.c mtu_discovery.c operation.c packet.c packet_builder.c packet_space.c path.c range.c recv_buffer.c registration.c send.c send_buffer.c sent_packet_metadata.c settings.c stream.c stream_recv.c stream_send.c stream_set.c timer_wheel.c version_neg.c worker.c cubic.h cubic_impl.h
##[warning]cl(0,0): Warning D9024: unrecognized source file type 'cubic.h', object file assumed
cl : command line warning D9024: unrecognized source file type 'cubic.h', object file assumed [D:\a\1\msquic\src\core\core.kernel.vcxproj]
##[warning]cl(0,0): Warning D9027: source file 'cubic.h' ignored
cl : command line warning D9027: source file 'cubic.h' ignored [D:\a\1\msquic\src\core\core.kernel.vcxproj]
##[warning]cl(0,0): Warning D9024: unrecognized source file type 'cubic_impl.h', object file assumed
cl : command line warning D9024: unrecognized source file type 'cubic_impl.h', object file assumed [D:\a\1\msquic\src\core\core.kernel.vcxproj]
##[warning]cl(0,0): Warning D9027: source file 'cubic_impl.h' ignored
cl : command line warning D9027: source file 'cubic_impl.h' ignored [D:\a\1\msquic\src\core\core.kernel.vcxproj]
##[warning]cl(0,0): Warning D9024: unrecognized source file type 'cubic.h', object file assumed
cl : command line warning D9024: unrecognized source file type 'cubic.h', object file assumed [D:\a\1\msquic\src\core\core.kernel.vcxproj]
##[warning]cl(0,0): Warning D9024: unrecognized source file type 'cubic.h', object file assumed
cl : command line warning D9024: unrecognized source file type 'cubic.h', object file assumed [D:\a\1\msquic\src\core\core.kernel.vcxproj]
##[warning]cl(0,0): Warning D9027: source file 'cubic.h' ignored
cl : command line warning D9027: source file 'cubic.h' ignored [D:\a\1\msquic\src\core\core.kernel.vcxproj]
...

Wizmann · 2021-09-04T05:46:45Z

/azp run

azure-pipelines · 2021-09-04T05:46:51Z

Commenter does not have sufficient privileges for PR 1949 in repo microsoft/msquic

nibanks · 2021-09-04T11:13:44Z

/azp run

azure-pipelines · 2021-09-04T11:14:02Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks

Thanks for making this change! It's been on our to-do list for a while!

nibanks · 2021-09-07T19:14:14Z

@Wizmann what CC algorithm are you working on? Is it BBR? v1 or v2? We likely wouldn't accept BBRv1 code.

Wizmann · 2021-09-08T03:01:02Z

It's great to see this PR has been completed. It makes my day! Thanks!

The CC algo I'm working on is BBRv1. As BBR v2 is still in alpha/preview (link), I think it should be a safe play to use a more mature CC algo, especially in a prod environment.

Besides, mvfst and chromimum both have the support of BBRv1. May I know the reason why MsQuic doesn't like BBRv1?

nibanks · 2021-09-08T12:23:49Z

BBRv1 has known problems (doesn't fairly share with loss-based CC algorithms) that would prevent us from actually using it in production. These issues are why BBRv2 was created.

Wizmann · 2021-09-08T17:02:04Z

Per our tests, the bandwidth fairness and latency fairness should be OK if the connections don't reach the limit of the bottleneck.

It's a dilemma that we have to choose from a CC algo with fairness flaw (BBRv1) and a CC algo still in alpha (BBRv2).

From my point of view, BBRv1 still has many users and capable for multiple scenarios.

nibanks · 2021-09-08T17:04:49Z

Per our tests, the bandwidth fairness and latency fairness should be OK if the connections don't reach the limit of the bottleneck.

But isn't the whole point of CC to deal with what happens if you're bottlenecked by the network? You have to assume you reach this bottleneck.

If you have the code already, feel free to send the PR and we can discuss more; but if you're going to dedicate significant time, we highly recommend BBRv2 instead.

Wizmann · 2021-09-09T04:20:19Z

But isn't the whole point of CC to deal with what happens if you're bottlenecked by the network?

Sometimes it's not true. Say we have a LFN, 200ms RTT and 0.2% packet loss, the best we can have for Cubic is 11MB/s. But for BBRv1, we can have 115MB/s at most.

Appearently, 11MB/s for Cubic is not "bottlenecked by the network", it just can't fully utilize the bandwidth.

I think if we use BBR along with other Cubic connectionns without reaching the bottleneck of the network, e.g. have a BBR connection with 50MB/s throughput, we can utilize more bandwidth and do no harm to the fairness. (Correct me if I'm wrong)

For the code part, we've already have BBR code for testing, it may take some time for PR. And we'll consider BBRv2 of course.

Wizmann added 3 commits August 29, 2021 21:58

pluggable congestion control algorithm

673f920

refine naming

72761be

refine naming

0c4cf25

Wizmann requested a review from a team as a code owner August 29, 2021 14:16

try fix linux build

96c8791

nibanks reviewed Aug 29, 2021

View reviewed changes

src/core/congestion_control.h Outdated Show resolved Hide resolved

nibanks reviewed Aug 29, 2021

View reviewed changes

src/inc/msquic.h Show resolved Hide resolved

nibanks added Area: Core Related to the shared, core protocol logic external Proposed by non-MSFT labels Aug 29, 2021

Wizmann added 2 commits August 30, 2021 23:25

fix linux build

55f44e1

fix code comments

e300602

try to fix compile error for generated clog files

51d33d5

try fix pipeline build error of sidecar

7d6392e

fix #ifndef error in msquic_posix.h

38dfdb7

nibanks reviewed Sep 1, 2021

View reviewed changes

src/core/congestion_control.h Outdated Show resolved Hide resolved

nibanks reviewed Sep 1, 2021

View reviewed changes

src/core/inline.c Outdated Show resolved Hide resolved

nibanks reviewed Sep 1, 2021

View reviewed changes

src/core/send_buffer.c Outdated Show resolved Hide resolved

fix compile error and code style

4ce465c

fix typo in settings.c

67b380a

nibanks reviewed Sep 2, 2021

View reviewed changes

src/core/congestion_control.h Show resolved Hide resolved

src/core/congestion_control.h Show resolved Hide resolved

src/core/congestion_control.h Outdated Show resolved Hide resolved

src/core/cubic.h Outdated Show resolved Hide resolved

src/core/quicdef.h Outdated Show resolved Hide resolved

fix comments

efed7c9

recreate sidecar files and fix build error on linux

c1bb642

nibanks reviewed Sep 3, 2021

View reviewed changes

src/core/cubic_impl.c

#include "precomp.h"

#ifdef QUIC_CLOG

#include "cubic_impl.c.clog.h"

Copy link

Member

nibanks Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just cubic.c?

fix core.kernel.vcxproj

5bd3bb7

thhous-msft approved these changes Sep 7, 2021

View reviewed changes

nibanks approved these changes Sep 7, 2021

View reviewed changes

nibanks merged commit d2a2570 into microsoft:main Sep 7, 2021

Refactor for pluggable congestion control algorithtm #1949

Refactor for pluggable congestion control algorithtm #1949

Conversation

Wizmann commented Aug 29, 2021

nibanks commented Aug 29, 2021

azure-pipelines bot commented Aug 29, 2021

nibanks commented Aug 30, 2021

azure-pipelines bot commented Aug 30, 2021

nibanks commented Aug 31, 2021

azure-pipelines bot commented Aug 31, 2021

nibanks commented Aug 31, 2021

nibanks commented Aug 31, 2021

nibanks commented Sep 1, 2021

azure-pipelines bot commented Sep 1, 2021

nibanks commented Sep 1, 2021

azure-pipelines bot commented Sep 1, 2021

nibanks commented Sep 1, 2021

nibanks commented Sep 2, 2021

azure-pipelines bot commented Sep 2, 2021

nibanks commented Sep 2, 2021

azure-pipelines bot commented Sep 2, 2021

nibanks commented Sep 3, 2021

azure-pipelines bot commented Sep 3, 2021

nibanks commented Sep 3, 2021

azure-pipelines bot commented Sep 3, 2021

nibanks Sep 3, 2021

Choose a reason for hiding this comment

Wizmann Sep 3, 2021

Choose a reason for hiding this comment

nibanks Sep 3, 2021

Choose a reason for hiding this comment

nibanks commented Sep 3, 2021

Wizmann commented Sep 4, 2021

azure-pipelines bot commented Sep 4, 2021

nibanks commented Sep 4, 2021

azure-pipelines bot commented Sep 4, 2021

nibanks left a comment

Choose a reason for hiding this comment

nibanks commented Sep 7, 2021

Wizmann commented Sep 8, 2021

nibanks commented Sep 8, 2021

Wizmann commented Sep 8, 2021

nibanks commented Sep 8, 2021

Wizmann commented Sep 9, 2021