Capture AVX context when redirecting threads for suspension by VSadov · Pull Request #47212 · dotnet/runtime

VSadov · 2021-01-20T06:04:46Z

Resolves:#47016

jkotas · 2021-01-20T06:15:46Z

src/coreclr/vm/threadsuspend.cpp

+    _ASSERTE(!success);
+
+    // So now allocate a buffer of that size and call IntitializeContext again
+    LPVOID buffer = malloc(contextSize);


Nit: In VM, the typically pattern is to new (nothrow) BYTE[....] instead of calling malloc directly

I had in fact tried this, but I observed an assert saying new [] isnt allowed here. Wonder what was different then.

VSadov · 2021-01-20T23:01:33Z

About the context size savings due to compaction:

On my machine:

sizeof(CONTEXT) -> 1232 bytes
InitializeContext with XSTATE_MASK_AVX -> 1775 bytes
InitializeContext2 with xStateCompactionMask = XSTATE_MASK_LEGACY | XSTATE_MASK_AVX -> 1647 bytes

I see 128 bytes savings from compaction, which is ~10%. Not a lot.
However my machine does not have AVX512 and perhaps on a machine with AVX512 the savings could be larger. Could be 800-900 bytes difference if my math is right.
It would be interesting to see what happens on actual AVX512 hardware though.

Also - somehow we do not have InitializeContext2 statically available fir our build config. The documentation specifies the same requirements for both InitializeContext and InitializeContext2 - Win7SP1, so I am not sure why we have one and not another.
As a workaround I check for InitializeContext2 dynamically.

VSadov · 2021-01-21T03:47:59Z

I think this is ready to be reviewed.

src/coreclr/vm/threads.h

mangod9 · 2021-01-21T04:09:16Z

src/coreclr/vm/threadsuspend.cpp

+    {
+        success = pfnInitializeContext2 ?
+            pfnInitializeContext2(buffer, context, &pOSContext, &contextSize, xStateCompactionMask):
+            InitializeContext(buffer, context, &pOSContext, &contextSize);


does our CI validate on non-AVX machine? Would be good to find one to ensure this change is not breaking on those.

It would be interesting to try on AVX512 machine as well. I doubt the CI machines/VMs have that.

Nearly everything has AVX now and AVX512 is fairly rare.

VMs in Azure should support some 512 variant:

GenuineIntel Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz 3DNOW not supported 3DNOWEXT not supported ABM not supported ADX supported AES supported AVX supported AVX2 supported AVX512CD supported AVX512ER not supported AVX512F supported AVX512PF not supported

jkotas · 2021-01-21T05:53:52Z

src/coreclr/vm/threadsuspend.cpp

+
+        if (!success)
+        {
+            free(buffer);


This should be delete[] buffer.

jkotas · 2021-01-21T05:56:19Z

src/coreclr/vm/threadsuspend.cpp

+    *contextBuffer = buffer;
+
+#else 
+    pOSContext = new (nothrow) CONTEXT();


Suggested change

pOSContext = new (nothrow) CONTEXT();

pOSContext = new (nothrow) CONTEXT;

I believe that new (nothrow) CONTEXT() will zero-initialize the context that is not necessary. This change should omit the zero-initialization.

That's true - verified with the compiler explorer

janvorli · 2021-01-21T13:59:48Z

src/coreclr/vm/threadsuspend.cpp

+
+    _ASSERTE(!success);
+
+    // So now allocate a buffer of that size and call IntitializeContext again


Suggested change

// So now allocate a buffer of that size and call IntitializeContext again

// So now allocate a buffer of that size and call InitializeContext again

janvorli · 2021-01-21T14:26:14Z

src/coreclr/vm/threadsuspend.cpp

+        pfnInitializeContext2(NULL, context, NULL, &contextSize, xStateCompactionMask) :
+        InitializeContext(NULL, context, NULL, &contextSize);
+
+    _ASSERTE(!success);


This should be _ASSERTE(success), right?

No, InitializeContext fails with ERROR_INSUFFICIENT_BUFFER but returns contextSize. Different than other win32 apis I guess.

Ah, ok. Then it would be nice to add checking the GetLastError()==ERROR_INSUFFICIENT_BUFFER to the _ASSERTE condition.

Good point.

janvorli · 2021-01-21T14:28:42Z

src/coreclr/vm/threadsuspend.cpp

+            InitializeContext(buffer, context, &pOSContext, &contextSize);
+
+        // if AVX is supported set the appropriate features mask in the context
+        if (success == TRUE && supportsAVX)


A nit - why success==TRUE and not just success?

janvorli · 2021-01-21T14:32:58Z

src/coreclr/vm/threadsuspend.cpp

+    *contextBuffer = buffer;
+
+#else 
+    pOSContext = new (nothrow) CONTEXT();


That's true - verified with the compiler explorer

janvorli · 2021-01-21T15:18:21Z

src/coreclr/vm/threadsuspend.cpp

+PINITIALIZECONTEXT2 pfnInitializeContext2 = NULL;
+
+#ifdef TARGET_X86
+#define CONTEXT_COMPLETE (CONTEXT_FULL | CONTEXT_FLOATING_POINT |       \


A nit - you can use CONTEXT_ALL | CONTEXT_EXCEPTION_REQUEST for all targets

Will change. Thanks!

Turns out this is not a case on ARM64.
On ARM64 CONTEXT_ALL also includes x18, which always contains TEB pointer in user mode. We are not getting it in RedirectCurrentThreadAtHandledJITCase case and that causes assertions, since we expect more.

Ill make this as it was originally.

Hmm, interesting. On Unix, x18 is not handled in a special way and is always part of the context.

janvorli · 2021-01-21T15:18:54Z

src/coreclr/vm/threadsuspend.cpp

+            // This should not normally fail.
+            // The system silently ignores any feature specified in the FeatureMask
+            // which is not enabled on the processor.
+            success = SetXStateFeaturesMask(pOSContext, XSTATE_MASK_AVX);


Won't the context initialized by the InitializeContext already have this set?

Samples in the docs make this call. (https://docs.microsoft.com/en-us/windows/win32/debug/working-with-xstate-context)
I think InitializeContext only knows that we want XSTATE, but more exact feature mask must be set separately.

janvorli · 2021-01-21T15:26:19Z

src/coreclr/vm/threadsuspend.cpp

-
-        if (!pCtx)
-        {
-            // Even when our caller is YieldTask, we can find a NULL


Isn't this case possible anymore?

YieldTask scenario no longer exists. Looks like it has been mostly removed, but some pieces remained.

It is tempting to do more clean up, but this change might need to be back-ported, so I tried to limit the size.

Ok, thank you for the explanation.

janvorli · 2021-01-21T15:31:00Z

src/coreclr/vm/threadsuspend.cpp

    // Get and save the thread's context

+#if defined(TARGET_X86) || defined(TARGET_AMD64)
+    _ASSERTE((pCtx->ContextFlags & CONTEXT_XSTATE) ==


Cannot we use the CopyContext API?

I do not know if it would be always safe to use CopyContext. CopyContext doc mentions that the context must be initialized with InitializeContext. We fall back to regular CONTEXT when we do not have AVX.

Actually I noticed that we still use InitializeContext even if AVX is disabled and it does not cost us much extra. Therefore I will use CopyContext. It will make this part a simpler.

janvorli · 2021-01-21T15:35:51Z

src/coreclr/vm/threadsuspend.cpp

        // If the thread is at a GC safe point, push a RedirectedThreadFrame with
        // the interrupted context and pulse the GC mode so that GC can proceed.
        FrameWithCookie<RedirectedThreadFrame> frame(interruptedContext);
-        pThread->SetSavedRedirectContext(NULL);


Don't we need to mark the redirect context as in use here?

No, in this case interruptedContext is stack-allocated in the signal handler. It is not the redirect instance, which we are guarding for under/over use. Marking "in use" here will detect a different instance and trigger an assert.

The original SetSavedRedirectContext(NULL) was a bit suspicious since it was not balanced with a "returning" set. If we ever had an actual redirect instance allocated, it might have leaked here.

mangod9

LGTM

VSadov · 2021-01-22T02:21:10Z

it looks like AVX in windows can be disabled via bcdedit /set xsavedisable 1 , so I tested the changes with that.

everything seems to work fine. The original repro for this bug does not repro, regardless of having a fix or not.
we still use InitializeContext in no-AVX mode and it gives us context buffer size 39 bytes bigger than sizeof(CONTEXT). I think that is acceptable.
since we use InitializeContext anyways, we can use CopyContext as @janvorli suggested, which makes RedirectCurrentThreadAtHandledJITCase a bit simpler.
I will make such change.
libs tests are all passing with AVX disabled
clr vector tests had some failures, but it is very likely a feature detection problem in the test harness. I will enter a separate issue on that - if we care to fix.

VSadov · 2021-01-22T04:12:42Z

I think I have addressed all the concerns on this. Please let me know if I missed something.

mangod9 · 2021-01-22T05:23:30Z

Should we validate with avx512? I have an Azure VM so can do a sanity check if you have a self-contained repro available.

VSadov · 2021-01-22T17:19:27Z

@mangod9 - Thanks! I wanted to run tests and do some debugging, so I just acquired a similar VM

Here are my observations:

regular tests all passed
the repro sample failed without a fix and passed with the fix - as expected
There is a noticeable size benefit from context compaction on this machine.
the size of context buffer:
InitializeContext -> 3375 bytes
InitializeContext2 with compaction flags -> 1647 bytes, same as on machine with no AVX512

Hardware:

	Name			Intel Xeon Platinum
	Codename		Skylake-SP
	Specification		Intel(R) Xeon(R) Platinum 8168 CPU @ 2.70GHz
	Package (platform ID)	Socket 3647 LGA (0x0)
	CPUID			6.5.4
	Extended CPUID		6.55
	Core Stepping		H0/U0
	Technology		14 nm
	Core Speed		2693.0 MHz                                                                  
	Stock frequency		2700 MHz                                                                    V
	Instructions sets	MMX, SSE, SSE2, SSE3, SSSE3, SSE4.1, SSE4.2, EM64T, VT-x, AES, AVX, AVX2, AVX512F, FMA3, TSX

mangod9 · 2021-01-22T17:22:02Z

Ok, nice. Good to go then :). Thx!

VSadov · 2021-01-22T17:52:53Z

Thanks!!!

VSadov · 2021-01-22T20:19:52Z

/backport to release/5.0

github-actions · 2021-01-22T20:20:49Z

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/504558594

github-actions · 2021-01-22T20:22:53Z

@VSadov backporting to release/5.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Save avx regs during SuspendThread
error: sha1 information is lacking or useless (src/coreclr/vm/threadsuspend.cpp).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Save avx regs during SuspendThread
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

…sion (#47447) * port of #47212 * missing change for Unix.

ghost added the area-System.Threading label Jan 20, 2021

jkotas reviewed Jan 20, 2021

View reviewed changes

mangod9 and others added 7 commits January 20, 2021 18:06

Save avx regs during SuspendThread

5b19c1f

few more changes and comments

b6a4d1d

fix build on windows arm

35cde35

fix build on Unix

75342f2

Use new[] instead of malloc

5407148

fix for Unix

69914ba

Use InitializeContext2 to enable context compaction.

320ff2e

VSadov force-pushed the avx branch from fa9c44f to 320ff2e Compare January 21, 2021 02:07

VSadov marked this pull request as ready for review January 21, 2021 03:47

VSadov requested review from janvorli and mangod9 January 21, 2021 03:47

mangod9 reviewed Jan 21, 2021

View reviewed changes

src/coreclr/vm/threads.h Show resolved Hide resolved

mangod9 reviewed Jan 21, 2021

View reviewed changes

src/coreclr/vm/threads.h Show resolved Hide resolved

mangod9 reviewed Jan 21, 2021

View reviewed changes

jkotas reviewed Jan 21, 2021

View reviewed changes

src/coreclr/vm/threadsuspend.cpp Outdated

if (!success)

{

free(buffer);

Copy link

Member

jkotas Jan 21, 2021

Choose a reason for hiding this comment

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

This should be delete[] buffer.

jkotas reviewed Jan 21, 2021

View reviewed changes

janvorli reviewed Jan 21, 2021

View reviewed changes

VSadov added 2 commits January 21, 2021 09:26

PR feedback

cef0ef9

Reverted CONTEXT_COMPLETE change.

2a93fc3

mangod9 approved these changes Jan 21, 2021

View reviewed changes

Use CopyContext in RedirectCurrentThreadAtHandledJITCase

5bda484

VSadov mentioned this pull request Jan 22, 2021

A few AVX tests failing when XSAVE is disabled at OS level #47317

Closed

runfoapp bot mentioned this pull request Jan 22, 2021

Inability to unzip assets during build on Unix x64 #32805

Closed

VSadov merged commit d0b10ed into dotnet:master Jan 22, 2021

VSadov deleted the avx branch January 22, 2021 17:53

This was referenced Jan 22, 2021

SequenceEqual broken for concurrent use? #47016

Closed

Saving/restoring of extended context during thread suspension #38974

Closed

VSadov added a commit to VSadov/runtime that referenced this pull request Jan 26, 2021

port of dotnet#47212

8f97652

VSadov mentioned this pull request Jan 26, 2021

[release/5.0] Capture AVX context when redirecting threads for suspension #47447

Merged

VSadov added a commit to VSadov/coreclr that referenced this pull request Jan 28, 2021

port of dotnet/runtime#47212

c9db701

VSadov mentioned this pull request Jan 28, 2021

[release/3.1] Capture AVX context when redirecting threads for suspension dotnet/coreclr#28131

Merged

Anipik pushed a commit that referenced this pull request Feb 10, 2021

[release/5.0] Capture AVX context when redirecting threads for suspen…

3fe7fe5

…sion (#47447) * port of #47212 * missing change for Unix.

aik-jahoda pushed a commit to dotnet/coreclr that referenced this pull request Feb 10, 2021

port of dotnet/runtime#47212 (#28131)

58f6883

ghost locked as resolved and limited conversation to collaborators Feb 21, 2021

	pOSContext = new (nothrow) CONTEXT();
	pOSContext = new (nothrow) CONTEXT;


		_ASSERTE(!success);

		// So now allocate a buffer of that size and call IntitializeContext again

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

VSadov commented Jan 21, 2021

Uh oh!

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

mangod9 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

VSadov commented Jan 22, 2021

Uh oh!

mangod9 commented Jan 22, 2021

VSadov commented Jan 20, 2021 •

edited

Loading

VSadov commented Jan 20, 2021 •

edited

Loading

VSadov commented Jan 22, 2021 •

edited

Loading

VSadov commented Jan 22, 2021 •

edited

Loading