Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat CxPlatWorker as a full object #4492

Merged
merged 17 commits into from
Sep 3, 2024

Conversation

ThadHouse
Copy link
Contributor

@ThadHouse ThadHouse commented Aug 25, 2024

Description

Currently, if you have an app that is creating a datapath outside of msquic, that datapath always shares the same worker context as msquic. There are some cases where this is not the desired behavior, and instead a completely separate worker context should be used, such as if you need to guarantee there is always a free worker, and you have limited the msquic execution context with QUIC_PARAM_GLOBAL_EXECUTION_CONFIG.

This PR changes CxPlatWorker into a full object, with a default instance used. All the datapath init functions take a Worker Manager, and if that manager is null, the default one is used in its place.

Testing

Alll existing tests will implicitly hit this path, and cover any necessary testing.

Documentation

No documentation changes, as using CxPlat directly generally isn't a supported scenario.

@ThadHouse ThadHouse requested a review from a team as a code owner August 25, 2024 05:48
@ThadHouse
Copy link
Contributor Author

@microsoft-github-policy-service agree

@ThadHouse ThadHouse marked this pull request as draft August 25, 2024 06:18
@ThadHouse ThadHouse changed the title Pass in CxPlatWorker functions as callbacks to datapaths Treat CxPlatWorker as a full object Aug 25, 2024
@ThadHouse ThadHouse marked this pull request as ready for review August 25, 2024 07:00
@nibanks nibanks added the external Proposed by non-MSFT label Aug 25, 2024
@nibanks
Copy link
Member

nibanks commented Aug 25, 2024

Generally looks good. Let's just rename WorkerManager to WorkerPool. I think I like that better. Thanks!

src/platform/platform_worker.c Dismissed Show dismissed Hide dismissed
src/inc/quic_datapath.h Outdated Show resolved Hide resolved
Copy link

codecov bot commented Aug 25, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.69%. Comparing base (22b7dda) to head (87628f7).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/core/library.c 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4492      +/-   ##
==========================================
- Coverage   86.37%   84.69%   -1.68%     
==========================================
  Files          56       56              
  Lines       15515    15520       +5     
==========================================
- Hits        13401    13145     -256     
- Misses       2114     2375     +261     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nibanks
nibanks previously approved these changes Aug 26, 2024
@nibanks
Copy link
Member

nibanks commented Aug 26, 2024

     core.lib(library.obj) : error LNK2001: unresolved external symbol CxPlatWorkerPoolInit [D:\a\msquic\msquic\src\bin\winkernel\msquicpriv.kernel.vcxproj]
     core.lib(library.obj) : error LNK2001: unresolved external symbol CxPlatWorkerPoolUninit [D:\a\msquic\msquic\src\bin\winkernel\msquicpriv.kernel.vcxproj]

Kernel mode doesn't currently rely on the worker pool.

@nibanks nibanks dismissed their stale review August 26, 2024 15:34

Build breaks

@ThadHouse
Copy link
Contributor Author

     core.lib(library.obj) : error LNK2001: unresolved external symbol CxPlatWorkerPoolInit [D:\a\msquic\msquic\src\bin\winkernel\msquicpriv.kernel.vcxproj]
     core.lib(library.obj) : error LNK2001: unresolved external symbol CxPlatWorkerPoolUninit [D:\a\msquic\msquic\src\bin\winkernel\msquicpriv.kernel.vcxproj]

Kernel mode doesn't currently rely on the worker pool.

Fixed. Just made them a no-op in kernel mode. Should be fine, any use of CxPlatAddExecutionContext will assert, and then if any functions are used in the data paths they'll get linker errors.

nibanks
nibanks previously approved these changes Aug 26, 2024
@nibanks nibanks dismissed their stale review August 29, 2024 10:55

Still kernel build failures

@ThadHouse
Copy link
Contributor Author

Ok. Should now correctly be fixed. Forgot how UNREFERENCED_PARAMETER macro worked.

src/inc/quic_platform.h Outdated Show resolved Hide resolved
@nibanks
Copy link
Member

nibanks commented Aug 30, 2024

"D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj" (default target) (9) ->
         D:\a\msquic\msquic\src\inc\quic_platform.h(440,22): error C2065: 'WorkerPool': undeclared identifier [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_platform.h(440,34): error C2059: syntax error: '(' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_datapath.h(408,14): error C2061: syntax error: identifier 'QUIC_EXECUTION_CONFIG' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_datapath.h(428,10): error C2061: syntax error: identifier 'QUIC_EXECUTION_CONFIG' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_platform.h(440,22): error C2065: 'WorkerPool': undeclared identifier [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_platform.h(440,34): error C2059: syntax error: '(' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_platform.h(440,22): error C2065: 'WorkerPool': undeclared identifier [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_platform.h(440,22): error C2065: 'WorkerPool': undeclared identifier [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_datapath.h(408,14): error C2061: syntax error: identifier 'QUIC_EXECUTION_CONFIG' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_platform.h(440,34): error C2059: syntax error: '(' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_datapath.h(428,10): error C2061: syntax error: identifier 'QUIC_EXECUTION_CONFIG' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_platform.h(440,34): error C2059: syntax error: '(' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_datapath.h(408,14): error C2061: syntax error: identifier 'QUIC_EXECUTION_CONFIG' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_datapath.h(408,14): error C2061: syntax error: identifier 'QUIC_EXECUTION_CONFIG' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_datapath.h(428,10): error C2061: syntax error: identifier 'QUIC_EXECUTION_CONFIG' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\inc\quic_datapath.h(428,10): error C2061: syntax error: identifier 'QUIC_EXECUTION_CONFIG' [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\perf\lib\SecNetPerfMain.cpp(230,5): error C2064: term does not evaluate to a function taking 1 arguments [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\perf\lib\SecNetPerfMain.cpp(236,14): error C2660: 'CxPlatDataPathInitialize': function does not take 6 arguments [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\perf\lib\SecNetPerfMain.cpp(238,9): error C3861: 'CxPlatWorkerPoolUninit': identifier not found [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]
         D:\a\msquic\msquic\src\perf\lib\SecNetPerfMain.cpp(282,9): error C3861: 'CxPlatWorkerPoolUninit': identifier not found [D:\a\msquic\msquic\src\perf\lib\perflib.kernel.vcxproj]

@nibanks
Copy link
Member

nibanks commented Aug 30, 2024

Looks like there are lots of test failures.

@ThadHouse
Copy link
Contributor Author

ThadHouse commented Aug 30, 2024

Some of those are failing on main after #4518 was merged. But there are a few that look to be me.

@nibanks
Copy link
Member

nibanks commented Aug 30, 2024

Some of those are failing on main after #4518 was merged. But there are a few that look to be me.

Crap, I thought everything was passing in that PR.

@nibanks nibanks merged commit d059622 into microsoft:main Sep 3, 2024
454 of 469 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Proposed by non-MSFT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants