msquic icon indicating copy to clipboard operation
msquic copied to clipboard

Treat CxPlatWorker as a full object

Open ThadHouse opened this issue 1 year ago • 5 comments

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 avatar Aug 25 '24 05:08 ThadHouse

@microsoft-github-policy-service agree

ThadHouse avatar Aug 25 '24 06:08 ThadHouse

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

nibanks avatar Aug 25 '24 17:08 nibanks

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 :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 25 '24 18:08 codecov[bot]

     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 avatar Aug 26 '24 15:08 nibanks

     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.

ThadHouse avatar Aug 26 '24 15:08 ThadHouse

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

ThadHouse avatar Aug 29 '24 23:08 ThadHouse

"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 avatar Aug 30 '24 11:08 nibanks

Looks like there are lots of test failures.

nibanks avatar Aug 30 '24 17:08 nibanks

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

ThadHouse avatar Aug 30 '24 17:08 ThadHouse

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 avatar Aug 30 '24 17:08 nibanks