Add preemptible work item APIs to cxplat library #94

dthaler · 2023-08-25T22:28:43Z

Move preemptible work item APIs from usersim library to cxplat library
Move preemptible rundown protection APIs from usersim library to cxplat library
Add tests for the above
Fix initialization bug in aligned allocate

nibanks · 2023-08-27T15:23:33Z

cxplat/src/cxplat_winuser/rundown_winuser.cpp

+ * @brief This following class implements a mock of the Windows Kernel's rundown reference implementation.
+ * 1) It uses a map to track the number of references to a given cxplat_rundown_reference_t structure.
+ * 2) The address of the cxplat_rundown_reference_t structure is used as the key to the map.
+ * 3) A single condition variable is used to wait for the ref count of any cxplat_rundown_reference_t structure to reach 0.
+ * 4) The class is a singleton and is created during static initialization and destroyed during static destruction.


Whis is this so complicated, and not simply a ref count and a completion event? Is it purely for debugging reasons? If so, we need a hyper optimized release mode version.

It was already this way in the usersim repo, all I did was rename it and move it to another file.
@Alan-Jowett was this originally your code?

The closest C++ equivalent of a Windows event is a condition variable. The C++ version was used to permit this code to be more easily made cross-platform.

An event / condition_variable per run-down reference would leak as there is no "free" API in kernel mode.

That said, it we don't care about Linux support, we could switch this to WaitOnAddress/WakeByAddress which is similar to this code, but implemented in the OS.

The goal first and foremost is performance. Code simplicity is secondary. So, it's perfectly fine to map the abstraction to whatever is most performant for each platform. If Linux has something different, that's ok.

we should move any workarounds for "no free API in kernel mode" over to the usersim directory and keep the complexity out of the cxplat directory.

Filed issue #98

nibanks · 2023-08-27T15:25:26Z

cxplat/src/cxplat_winuser/workitem_winuser.cpp

+    cxplat_status_t result = CXPLAT_STATUS_SUCCESS;
+
+    if (!cxplat_acquire_rundown_protection(&_cxplat_preemptible_work_items_rundown_reference)) {
+        return CXPLAT_STATUS_UNSUCCESSFUL;


Instead of unsuccessful, can we do something like invalid state? Because if this fails, it means we haven't initialized or are shutting down, right (i.e. not the correct/expected state)?

I didn't see an NTSTATUS value with quite that meaning, so changed to use NOT_SUPPORTED instead of UNSUCCESSFUL.

While not 100% exact, we have this in msquic:

#define QUIC_STATUS_INVALID_STATE STATUS_INVALID_DEVICE_STATE // 0xc0000184

https://github.com/microsoft/msquic/blob/main/src/inc/msquic_winkernel.h#L97

STATUS_INVALID_DEVICE_STATE ("The device is not in a valid state to perform this request.") implies a device is involved, and maps to win32 ERROR_BAD_COMMAND ("The device does not recognize the command.").

STATUS_INVALID_STATE_TRANSITION ("The requested state transition is invalid and cannot be performed.") on the other hand seems to be more generic, and maps to win32 ERROR_INVALID_STATE ("The group or resource is not in the correct state to perform the requested operation.")

Since STATUS_INVALID_STATE_TRANSITION seems to be more in the spirit here, I will use that, since a "device" per se may not be involved.

nibanks · 2023-08-27T15:26:31Z

cxplat/src/cxplat_winuser/workitem_winuser.cpp

+        return CXPLAT_STATUS_UNSUCCESSFUL;
+    }
+
+    *work_item = (cxplat_preemptible_work_item_t*)cxplat_allocate(sizeof(cxplat_preemptible_work_item_t));


IMO, we should have special tags for all internally allocated objects so we can track them appropriately.

Agree. it was already this way in the usersim repo (and the ebpf-for-windows repo before that) though, I just moved/renamed the function. Filed #97.

nibanks · 2023-08-27T15:26:56Z

cxplat/src/cxplat_winuser/workitem_winuser.cpp

+    // initialize the _callback_environment structure before calling CreateThreadpoolWork.
+    (*work_item)->work = CreateThreadpoolWork(_cxplat_preemptible_routine, *work_item, &_callback_environment);
+    if ((*work_item)->work == nullptr) {
+        result = CXPLAT_STATUS_FROM_WIN32(GetLastError());


We're going to need to add logging eventually at every first point of failure, like this.

nibanks · 2023-08-27T15:27:29Z

cxplat/src/cxplat_winuser/workitem_winuser.cpp

+    SetThreadpoolThreadMaximum(_pool, 1);
+    return_value = SetThreadpoolThreadMinimum(_pool, 1);


Perhaps some code comments to explain why this is done?

@Alan-Jowett was this originally your code? The code was already this way in this repo, this isn't new code.

The minimum was to ensure that we make forward progress on low memory. The maximum was to catch cases where work-items depend on each other.

I.e. if work item A blocks while executing waiting on work item B, then it may deadlock on low memory. Better to deadlock all the time.

Sounds like another case of the code being debug-focused, which is fine for debug builds, but not retail/release builds.

sure: maybe Alan can add some ifndef NDEBUG around such code?

nibanks · 2023-08-27T15:29:24Z

cxplat/cxplat_test/cxplat_test.vcxproj

@@ -131,7 +131,9 @@
  </ItemDefinitionGroup>
  <ItemGroup>
    <ClCompile Include="cxplat_memory_test.cpp" />
+    <ClCompile Include="cxplat_rundown_test.cpp" />


As more and more stuff is added, it's going to make it an even bigger job to bring Posix platform support, which is a requirement for MsQuic, into the picture here. Perhaps it's time to do this so that future PRs can then add the piecemeal support for the platform.

feel free :)
Should this move to the cxplat repo before then or after then?

Let's move it to cxplat first.

nibanks · 2023-08-28T15:05:54Z

cxplat/src/cxplat_winuser/symbol_decoder.h

 inline cxplat_status_t
 _cxplat_symbol_decoder_initialize()
 {
    // Initialize DbgHelp.dll.
    SymSetOptions(SYMOPT_UNDNAME | SYMOPT_DEFERRED_LOADS | SYMOPT_LOAD_LINES);
    if (!SymInitialize(GetCurrentProcess(), nullptr, TRUE)) {
+        int err = GetLastError();
+        if (err == ERROR_INVALID_PARAMETER) {
+            // The process already did SymInitialize(), which is fine.


Does this mean the caller is calling _cxplat_symbol_decoder_initialize more than once? If so, what if they call _cxplat_symbol_decoder_deinitialize more than once?

No, it means the caller calls SymInitialize directly without using cxplat.

What happens if it also calls the uninitialize equivalent too?

This API is internal, not exposed to callers, only callable inside this library, from cxplat_cleanup().
A better question is: what happens if the caller calls cxplat_cleanup() multiple times.
Filed issue #99 to track that.

Signed-off-by: Dave Thaler <[email protected]>

dthaler requested review from Alan-Jowett, mtfriesen and nibanks as code owners August 25, 2023 22:28

dthaler force-pushed the preempt branch 4 times, most recently from e12ccc5 to 25d12b4 Compare August 25, 2023 23:59

nibanks reviewed Aug 27, 2023

View reviewed changes

nibanks reviewed Aug 28, 2023

View reviewed changes

Alan-Jowett approved these changes Aug 28, 2023

View reviewed changes

dthaler mentioned this pull request Aug 28, 2023

Optimize cxplat_winuser/rundown_winuser.cpp #98

Open

dthaler added 5 commits August 28, 2023 10:34

Add preemptible work item APIs to cxplat library

94d4c1e

Signed-off-by: Dave Thaler <[email protected]>

Fix symbol decoder if already in use by the process

4d9bc96

Signed-off-by: Dave Thaler <[email protected]>

Fix initialization bug in aligned allocate

e3ff623

Signed-off-by: Dave Thaler <[email protected]>

Fix release tests

fe81630

Signed-off-by: Dave Thaler <[email protected]>

Use INVALID_STATE error code

22a7014

Signed-off-by: Dave Thaler <[email protected]>

dthaler force-pushed the preempt branch from bc2107b to 22a7014 Compare August 28, 2023 17:35

dthaler merged commit 1d9a9f4 into main Aug 28, 2023
5 checks passed

dthaler deleted the preempt branch August 28, 2023 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preemptible work item APIs to cxplat library #94

Add preemptible work item APIs to cxplat library #94

dthaler commented Aug 25, 2023 •

edited

Loading

nibanks Aug 27, 2023

dthaler Aug 27, 2023 •

edited

Loading

Alan-Jowett Aug 28, 2023

nibanks Aug 28, 2023

dthaler Aug 28, 2023

dthaler Aug 28, 2023

nibanks Aug 27, 2023

dthaler Aug 28, 2023

nibanks Aug 28, 2023

dthaler Aug 28, 2023

nibanks Aug 27, 2023

dthaler Aug 27, 2023

nibanks Aug 27, 2023

dthaler Aug 27, 2023

nibanks Aug 27, 2023

dthaler Aug 27, 2023 •

edited

Loading

Alan-Jowett Aug 28, 2023

nibanks Aug 28, 2023

dthaler Aug 28, 2023

nibanks Aug 27, 2023

dthaler Aug 28, 2023

nibanks Aug 28, 2023

nibanks Aug 28, 2023

dthaler Aug 28, 2023

nibanks Aug 28, 2023

dthaler Aug 28, 2023

		SetThreadpoolThreadMaximum(_pool, 1);
		return_value = SetThreadpoolThreadMinimum(_pool, 1);

Add preemptible work item APIs to cxplat library #94

Add preemptible work item APIs to cxplat library #94

Conversation

dthaler commented Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

dthaler Aug 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dthaler Aug 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dthaler commented Aug 25, 2023 •

edited

Loading

dthaler Aug 27, 2023 •

edited

Loading

dthaler Aug 27, 2023 •

edited

Loading