[NativeAOT] Use QueueUserAPC2 in GC suspension, if available. #80087

VSadov · 2022-12-31T23:18:21Z

No description provided.

ghost · 2022-12-31T23:18:30Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Not completed yet, but should be functional. There will be some refactoring.

Current test run will fail if lab machines have QueueUserAPC2, intentionally - I want to find out if the OS builds on lab machines have this API, especially on ARM64, where I did not have a way to test.

Author:	VSadov
Assignees:	-
Labels:	`area-NativeAOT-coreclr`
Milestone:	-

MichalStrehovsky · 2023-01-02T00:10:23Z

Most likely everything runs on Win10.

At minimum the ARM64 legs are Win11. From the console log of one of the tests in this PR:

Console log: 'Microsoft.Extensions.Configuration.Functional.Tests' from job a5783e36-675c-4d1c-bd66-912b1b2cd73c workitem fd692c4c-6e4e-464a-bffa-2e49f0d3d415 (windows.11.arm64.open) executed on machine a0006UM running Windows-10-10.0.22621-SP0

22621 is the 22H2 Windows 11 build.

VSadov · 2023-01-02T00:52:28Z

I am testing locally on 22623.1037 (x64 insider preview in Beta/Slow ring) and it has the feature. It is not that much newer than 22621.
Perhaps it is different with ARM64? It should not be though. The API is not architecture-specific.

VSadov · 2023-01-02T08:54:22Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-01-02T08:54:46Z

Azure Pipelines successfully started running 1 pipeline(s).

src/coreclr/nativeaot/Runtime/CMakeLists.txt

src/coreclr/nativeaot/Runtime/windows/PalRedhawkMinWin.cpp

VSadov · 2023-01-06T02:08:42Z

@jkotas - anything else we might want to add to this change?

jkotas · 2023-01-06T02:40:56Z

src/coreclr/nativeaot/Runtime/windows/PalRedhawkMinWin.cpp


+#ifdef FEATURE_SPECIAL_USER_MODE_APC
+typedef BOOL (WINAPI* QueueUserAPC2Proc)(PAPCFUNC ApcRoutine, HANDLE Thread, ULONG_PTR Data, QUEUE_USER_APC_FLAGS Flags);
+static QueueUserAPC2Proc pfnQueueUserAPC2Proc;


Suggested change

static QueueUserAPC2Proc pfnQueueUserAPC2Proc;

static QueueUserAPC2Proc g_pfnQueueUserAPC2Proc;

jkotas · 2023-01-06T02:42:58Z

src/coreclr/nativeaot/Runtime/windows/PalRedhawkMinWin.cpp

+    // See if QueueUserAPC2 supports the special user-mode APC with a callback that includes the interrupted CONTEXT
+    if (pfnQueueUserAPC2Proc != NULL)
+    {
+        if (!(*pfnQueueUserAPC2Proc)(EmptyActivationHandler, GetCurrentThread(), 0, SpecialUserModeApcWithContextFlags))


Can we delay this until we actually need to suspend the thread? We should not even need a dummy call for this. We can just try to call it for real and see whether it failed.

On Win10 this will always fail, but the API is present. We would need to prevent trying over and over.
With some sentinel value for "unknown" state and NULL for "does not work" this might be possible.

Right, we can have a bool flag that says the API failed with "SpecialUserModeApcWithContextFlags not supported error". Once the flag gets set, we will stop trying.

I've uses a sentinel value for uninitialized state. Hijacking pass is single-threaded, so initializing on demand is simple.
Maybe it will save us some ticks at startup.

jkotas · 2023-01-06T05:02:14Z

src/coreclr/nativeaot/Runtime/windows/PalRedhawkMinWin.cpp

+    {
+        g_pfnQueueUserAPC2Proc = (QueueUserAPC2Proc)GetProcAddress(LoadKernel32dll(), "QueueUserAPC2");
+    }
+


This is not thread safe in C/C++.

It is valid for C/C++ compiler to rewrite this as:

QueueUserAPC2Proc pfn = g_pfnQueueUserAPC2Proc; if (g_pfnQueueUserAPC2Proc == QUEUE_USER_APC2_UNINITIALIZED) { g_pfnQueueUserAPC2Proc = pfn = (QueueUserAPC2Proc)GetProcAddress(LoadKernel32dll(), "QueueUserAPC2"); } if (pfn) { .... }

It needs to use volatile or something like that.

Only one thread may perform suspension/hijacking at a time. This code is single threaded, protected by the thread store lock.

It needs comment at least to make this assumption explicit.

I’ve made the variable ‘volatile’ initially n this change, but then realized that there is no need.

I’ll add a comment. It was not obvious.
I even had ‘Interlocked’, etc, before realizing it is single threaded and can be a lot simpler.

jkotas · 2023-01-06T05:04:49Z

src/coreclr/nativeaot/Runtime/windows/PalRedhawkMinWin.cpp

+            return;
+        }
+
+        // queuing an APC failed. we will not try again


Should we check for specific "not suppported" last error here?

I do not think API documents particular errors. They just say the error could be fetched via last error.

If we recognize some errors, what would we do with errors that we do not recognize?

If we recognize some errors, what would we do with errors that we do not recognize?

Ignore the error and try again a bit later.

I’ll check what OS with no support for QUEUE_USER_APC_CALLBACK_DATA_CONTEXT sets the last error to.

jkotas

Thank you!

jkotas · 2023-01-06T14:09:30Z

Looks like the tests are failing with "failed to queue an APC for unusual reason."

VSadov · 2023-01-06T18:22:56Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-01-06T18:23:10Z

Azure Pipelines successfully started running 1 pipeline(s).

VSadov · 2023-01-06T23:45:37Z

Thanks!!

jakobbotsch · 2023-01-07T18:41:06Z

The build is failing locally for me after this PR:

  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(452): error C2061: syntax error: identifier 'QUEUE_USER_APC_FLAGS'
  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(457): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(457): error C2146: syntax error: missing ';' before identifier 'SpecialUserModeApcWithContextFlags'
  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(457): error C2059: syntax error: '='
  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(463): error C2065: 'APC_CALLBACK_DATA': undeclared identifier
  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(463): error C2065: 'data': undeclared identifier
  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(463): error C2065: 'APC_CALLBACK_DATA': undeclared identifier
  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(463): error C2059: syntax error: ')'
  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(464): error C2065: 'data': undeclared identifier
  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(466): error C2065: 'data': undeclared identifier
  C:\dev\dotnet\runtime\src\coreclr\nativeaot\Runtime\windows\PalRedhawkMinWin.cpp(507): error C2065: 'SpecialUserModeApcWithContextFlags': undeclared identifier

jkotas · 2023-01-07T19:33:30Z

@jakobbotsch What is the Windows SDK version on your machine? (dir C:\Program Files (x86)\Windows Kits\10\Include)

VSadov · 2023-01-07T20:58:14Z

As I understand 8.0 main builds with Windows 11 SDK 10.0.22621, which defines these.

VSadov · 2023-01-07T23:07:08Z

Although vsconfig (https://github.com/dotnet/runtime/blob/main/.vsconfig) lists Windows10SDK.19041

VSadov · 2023-01-07T23:08:26Z

This is confusing. When I tried defining these the lab build failed because of duplicate definitions and I only could reproduce the lab behavior by installing 10.0.22621 locally

jkotas · 2023-01-07T23:21:16Z

https://github.com/dotnet/runtime/blob/main/.vsconfig is always out-of-date.

jkotas · 2023-01-07T23:25:32Z

As I understand 8.0 main builds with Windows 11 SDK 10.0.22621

FWIW, this is the SDK version that I have on my machines as well. I always use the recommended Workloads installation approach (https://github.com/dotnet/runtime/blob/main/docs/workflow/requirements/windows-requirements.md#visual-studio).

VSadov · 2023-01-07T23:29:25Z

It is possible to redefine the types/constants under different name. It looks like this is what we did in coreclr CET support.

VSadov · 2023-01-07T23:32:55Z

Here is a PR that should build without 22621 #80333

VSadov · 2023-01-07T23:39:00Z

With that PR I can build even if I remove 22621.

I could not remove Windows 11 22000 SDK though, because, according to VS installer, c++ v143 depends on it and would need to be removed too.

jakobbotsch · 2023-01-08T00:03:12Z

@jakobbotsch What is the Windows SDK version on your machine? (dir C:\Program Files (x86)\Windows Kits\10\Include)

The version was 10.0.19041. It indeed builds fine for me after updating the SDK, so no worries from my side at least.

VSadov added 2 commits December 31, 2022 15:05

QueueUserAPC2

4e3a080

fixes, do not stack APCs

422b864

ghost added the area-NativeAOT-coreclr label Dec 31, 2022

ghost assigned VSadov Dec 31, 2022

adjust for newer SDK

5492cdf

VSadov force-pushed the quapc2 branch from 4826d9f to 183b85b Compare January 1, 2023 00:39

build-analysis bot mentioned this pull request Jan 1, 2023

Tracking issue for CI build timeouts #76454

Closed

VSadov force-pushed the quapc2 branch from 183b85b to 3a5d24e Compare January 1, 2023 17:44

dotnet deleted a comment from azure-pipelines bot Jan 1, 2023

VSadov force-pushed the quapc2 branch from 3a5d24e to 9e33c3f Compare January 1, 2023 19:05

dotnet deleted a comment from azure-pipelines bot Jan 1, 2023

force a failure - to see if lab machines have support for this

8bb1ddf

VSadov force-pushed the quapc2 branch from 9e33c3f to 8bb1ddf Compare January 1, 2023 19:18

dotnet deleted a comment from azure-pipelines bot Jan 1, 2023

VSadov added 5 commits January 1, 2023 16:45

cleanup some includes

392a6a9

FEATURE_SUSPEND_APC2

4ec1aab

remove intentional failure

f73053a

one more case of FEATURE_SUSPEND_APC2

89944d5

comment

51d96af

VSadov marked this pull request as ready for review January 2, 2023 03:04

VSadov requested a review from MichalStrehovsky as a code owner January 2, 2023 03:04

fail if QUEUE_USER_APC_CALLBACK_DATA_CONTEXT supported

c466ff2

dotnet deleted a comment from azure-pipelines bot Jan 2, 2023

jkotas reviewed Jan 2, 2023

View reviewed changes

src/coreclr/nativeaot/Runtime/CMakeLists.txt Outdated Show resolved Hide resolved

jkotas reviewed Jan 2, 2023

View reviewed changes

src/coreclr/nativeaot/Runtime/windows/PalRedhawkMinWin.cpp Outdated Show resolved Hide resolved

VSadov added 2 commits January 3, 2023 21:03

unmark ActivationPending if APC failed to queue.

24da966

a bit more robust against sharing the signal with something else.

3d23d1e

jkotas reviewed Jan 6, 2023

View reviewed changes

initialize QueueUserAPC2 on demand

63d347d

jkotas reviewed Jan 6, 2023

View reviewed changes

Check for STATUS_INVALID_PARAMETER

fc53cc5

jkotas approved these changes Jan 6, 2023

View reviewed changes

VSadov added 2 commits January 6, 2023 08:40

print the error

175c3f3

STATUS_INVALID_PARAMETER -> ERROR_INVALID_PARAMETER

e3f2418

VSadov merged commit 7a6f33b into dotnet:main Jan 6, 2023

VSadov deleted the quapc2 branch January 6, 2023 23:45

ghost locked as resolved and limited conversation to collaborators Feb 7, 2023

	static QueueUserAPC2Proc pfnQueueUserAPC2Proc;
	static QueueUserAPC2Proc g_pfnQueueUserAPC2Proc;

[NativeAOT] Use QueueUserAPC2 in GC suspension, if available. #80087

[NativeAOT] Use QueueUserAPC2 in GC suspension, if available. #80087

Uh oh!

Conversation

VSadov commented Dec 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 31, 2022

Uh oh!

MichalStrehovsky commented Jan 2, 2023

Uh oh!

VSadov commented Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Jan 2, 2023

Uh oh!

azure-pipelines bot commented Jan 2, 2023

Uh oh!

Uh oh!

Uh oh!

VSadov commented Jan 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jan 6, 2023

Uh oh!

VSadov commented Jan 6, 2023

Uh oh!

azure-pipelines bot commented Jan 6, 2023

Uh oh!

VSadov commented Jan 6, 2023

Uh oh!

jakobbotsch commented Jan 7, 2023

Uh oh!

jkotas commented Jan 7, 2023

Uh oh!

VSadov commented Jan 7, 2023

Uh oh!

VSadov commented Jan 7, 2023

Uh oh!

VSadov commented Jan 7, 2023

Uh oh!

jkotas commented Jan 7, 2023

Uh oh!

jkotas commented Jan 7, 2023

Uh oh!

VSadov commented Jan 7, 2023

VSadov commented Dec 31, 2022 •

edited

Loading

VSadov commented Jan 2, 2023 •

edited

Loading

VSadov Jan 6, 2023 •

edited

Loading

VSadov Jan 6, 2023 •

edited

Loading

VSadov commented Jan 7, 2023 •

edited

Loading

jakobbotsch commented Jan 8, 2023 •

edited

Loading