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

Remove CPU groups notion from the gc.cpp #23537

Merged
merged 5 commits into from Apr 4, 2019

Conversation

Projects
None yet
2 participants
@janvorli
Copy link
Member

commented Mar 29, 2019

This change removes all explicit manipulation and handling of CPU groups
from the gc.cpp and hides it behind the GCToOSInterface. This is a step
to prepare for removing the CPU groups emulation on Unix. In fact, I've
already updated the standalone Unix GC to be able to affinitize GC
threads to any subset of CPUs and added previously missing support for
the affinity setting itself. The NUMA support still remains missing there.

It also adds a new way to specify GC thread affinitization that is not
limited to 64 threads. Instead of affinity mask stored in 64 bit
integer, we now use a bitset that can contain as many processors as
GC can support. And there is a new GC config to specify the affinity in
a form of a range list.

@janvorli janvorli added the area-GC label Mar 29, 2019

@janvorli janvorli added this to the 3.0 milestone Mar 29, 2019

@janvorli janvorli requested a review from Maoni0 Mar 29, 2019

@janvorli janvorli self-assigned this Mar 29, 2019

Show resolved Hide resolved src/gc/gcconfig.h Outdated
Show resolved Hide resolved src/gc/env/gcenv.os.h Outdated
Show resolved Hide resolved src/gc/gcconfig.h
@@ -42,6 +66,20 @@ bool GCToOSInterface::Initialize()
g_pageSizeUnixInl = GetOsPageSize();
#endif

uintptr_t pmask, smask;
if (!!::GetProcessAffinityMask(::GetCurrentProcess(), (PDWORD_PTR)&pmask, (PDWORD_PTR)&smask))

This comment has been minimized.

Copy link
@Maoni0

Maoni0 Mar 29, 2019

Member

GetProcessAffinityMask [](start = 12, length = 22)

I must be missing something - if you are running in cpu groups this would make return pmask/smask as 0. so g_processAffinitySet will be 0. is there code that fills it in for the cpu group case on windows?

This comment has been minimized.

Copy link
@janvorli

janvorli Mar 29, 2019

Author Member

This is not changing the current behavior. This process mask getting has just moved from GCHeap::Initialize to GCToOSInterface::Initialize.
As for the mask, it never returns 0 at the initialization time as the process is running in a single CPU group and the affinity mask is relative to that group.

Actually, the GC code even without my changes ignores the process affinity mask when CPU groups are enabled. See the process_mask that was replaced by g_processAffinitySet.
I guess the reason was that the process affinity mask at launch time (that can be specified using the AFFINITY parameter of the start command) is always relative to a single group, but we use more groups when groups are enabled and thus that mask doesn't make much sense (well, we could say that we would apply that mask on the original group the process was launched in and the rest of the groups are not affected, but that would be weird).

So I'd suggest modifying the behavior for the case when CPU groups are enabled. We would completely ignore the launch-time process affinity mask (which we already do), but we would use the mask or ranges specified by the config knobs as-is to set the g_processAffinitySet. In case the user didn't set any of these knobs, the g_processAffinitySet would be set to contain all the processors on the system.

This comment has been minimized.

Copy link
@Maoni0

Maoni0 Mar 29, 2019

Member

we talked about this - I was mistakenly thinking that the CPU group affinitizing happened before GC init which cause my confusion; and correct, the GCHeapAffinitizeRanges should be used in all cases that are applicable, including on Windows where it's running in multiple CPU groups.

uint16_t gn, gpn;
CPUGroupInfo::GetGroupForProcessor((uint16_t)heap_number, &gn, &gpn);

// dprintf(3, ("using processor group %d, mask %Ix for heap %d\n", gn, (uintptr_t)1 << gpn), heap_number));

This comment has been minimized.

Copy link
@Maoni0

Maoni0 Mar 29, 2019

Member

dprintf [](start = 8, length = 7)

seeing that you cannot use dprintf's here, the lines with dprintf's should be deleted

This comment has been minimized.

Copy link
@janvorli

janvorli Mar 29, 2019

Author Member

I have left it there just to see if you'd think this dprintf was important enough so that we should put it to the caller side and print similar thing based on the values returned by the GetProcessorForHeap.

Show resolved Hide resolved src/gc/gc.cpp
Show resolved Hide resolved src/gc/gc.cpp

@janvorli janvorli force-pushed the janvorli:remove-gc-cpu-group-knowledge branch from 43df86e to 468adae Apr 2, 2019

janvorli added some commits Mar 25, 2019

Remove CPU groups handling from the gc.cpp
This change removes all explicit manipulation and handling of CPU groups
from the gc.cpp and hides it behind the GCToOSInterface. This is a step
to prepare for removing the CPU groups emulation on Unix. In fact, I've
already updated the standalone Unix GC to be able to affinitize GC
threads to any subset of CPUs and added previously missing support for
the affinity setting itself. The NUMA support still remains missing there.

It also adds a new way to specify GC thread affinitization that is not
limited to 64 threads. Instead of affinity mask stored in 64 bit
integer, we now use a bitset that can contain as many processors as
GC can support. And there is a new GC config to specify the affinity in
a form of a range list.
Use the GCHeapAffinitizeRanges / GCHeapAffinitizeMask exclusively
The GCHeapAffinitizeRanges are now used only when CPU groups are enabled
and the GCHeapAffinitizeMask when CPU groups are disabled.

@janvorli janvorli force-pushed the janvorli:remove-gc-cpu-group-knowledge branch from 468adae to dd80afe Apr 3, 2019

@janvorli

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@Maoni0 I've added a commit fixing the flipped error codes that you have noticed.

Modify affinity range config format for Windows
Each entry has to be prefixed by group number followed by comma. There
is nothing like global CPU index on Windows, all the APIs that support
more than 64 processors use group, in-group index pair. So specifying
the affinity this way matches what users are used to.
@janvorli

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@dotnet-bot test Ubuntu x64 Checked CoreFX Tests please

@janvorli

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

/azp run coreclr-ci

@azure-pipelines

This comment has been minimized.

Copy link

commented Apr 4, 2019

Azure Pipelines failed to run 1 pipeline(s).

@janvorli janvorli closed this Apr 4, 2019

@janvorli janvorli reopened this Apr 4, 2019

@Maoni0

Maoni0 approved these changes Apr 4, 2019

Copy link
Member

left a comment

LGTM!

@janvorli janvorli merged commit 126aaf4 into dotnet:master Apr 4, 2019

34 of 43 checks passed

Ubuntu x64 Checked CoreFX Tests Build finished.
Details
Windows_NT arm Cross Checked Innerloop Build and Test Build finished.
Details
Windows_NT arm64 Cross Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Checked CoreFX Tests Build finished.
Details
Windows_NT x64 Release CoreFX Tests Build finished.
Details
coreclr-ci Build #20190404.722 had test failures
Details
coreclr-ci (Test Pri0 Windows_NT x64 checked) Test Pri0 Windows_NT x64 checked failed
Details
coreclr-ci (Test Pri0 Windows_NT x86 checked) Test Pri0 Windows_NT x86 checked failed
Details
coreclr-ci (Test pri0 Linux_musl x64 checked) Test pri0 Linux_musl x64 checked failed
Details
Ubuntu arm Cross Checked crossgen_comparison Build and Test Build finished.
Details
Ubuntu arm Cross Release crossgen_comparison Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
WIP Ready for review
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
coreclr-ci (Build Linux arm checked) Build Linux arm checked succeeded
Details
coreclr-ci (Build Linux arm64 checked) Build Linux arm64 checked succeeded
Details
coreclr-ci (Build Linux arm64 release) Build Linux arm64 release succeeded
Details
coreclr-ci (Build Linux_musl x64 checked) Build Linux_musl x64 checked succeeded
Details
coreclr-ci (Build Linux_musl x64 release) Build Linux_musl x64 release succeeded
Details
coreclr-ci (Build Linux_rhel6 x64 release) Build Linux_rhel6 x64 release succeeded
Details
coreclr-ci (Build Windows_NT arm Checked) Build Windows_NT arm Checked succeeded
Details
coreclr-ci (Build Windows_NT arm64 Checked) Build Windows_NT arm64 Checked succeeded
Details
coreclr-ci (Build Windows_NT x64 Checked) Build Windows_NT x64 Checked succeeded
Details
coreclr-ci (Build Windows_NT x64 Debug) Build Windows_NT x64 Debug succeeded
Details
coreclr-ci (Build Windows_NT x86 Checked) Build Windows_NT x86 Checked succeeded
Details
coreclr-ci (Build Windows_NT x86 Debug) Build Windows_NT x86 Debug succeeded
Details
coreclr-ci (Test Pri0 Linux x64 checked) Test Pri0 Linux x64 checked succeeded
Details
coreclr-ci (Test Pri0 Linux_musl x64 release) Test Pri0 Linux_musl x64 release succeeded
Details
coreclr-ci (Test Pri0 OSX x64 checked) Test Pri0 OSX x64 checked succeeded
Details
coreclr-ci (Test Pri0 Windows_NT arm checked) Test Pri0 Windows_NT arm checked succeeded
Details
coreclr-ci (Test Pri0 Windows_NT arm64 checked) Test Pri0 Windows_NT arm64 checked succeeded
Details
coreclr-ci (Test pri0 Linux arm checked) Test pri0 Linux arm checked succeeded
Details
coreclr-ci (Test pri0 Linux arm64 checked) Test pri0 Linux arm64 checked succeeded
Details
coreclr-ci (build Linux x64 Checked) build Linux x64 Checked succeeded
Details
coreclr-ci (build OSX x64 Checked) build OSX x64 Checked succeeded
Details
coreclr-ci (build Windows_NT arm Release) build Windows_NT arm Release succeeded
Details
coreclr-ci (build Windows_NT arm64 Release) build Windows_NT arm64 Release succeeded
Details
coreclr-ci (build Windows_NT x64 Release) build Windows_NT x64 Release succeeded
Details
license/cla All CLA requirements met.
Details

@janvorli janvorli deleted the janvorli:remove-gc-cpu-group-knowledge branch Apr 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.