Skip to content

SR-10611: ProcessInfo.activeProcessorCount too high in containerised Linux #2215

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

Merged
merged 2 commits into from
May 8, 2019

Conversation

spevans
Copy link
Contributor

@spevans spevans commented May 3, 2019

  • On Linux, use sched_getaffinity() falling back to
    sysconf(_SC_NPROCESSORS_ONLN) as this should provide a better
    value for ProcessInfo.activeProcessorCount when running under
    cgroups (eg Docker).

Not sure if this is the best solution and will need an offline test in a Docker environment.

…Linux

- On Linux, use sched_getaffinity() falling back to
  sysconf(_SC_NPROCESSORS_ONLN) as this should provide a better
  value for ProcessInfo.activeProcessorCount when running under
  cgroups (eg Docker).
@spevans
Copy link
Contributor Author

spevans commented May 3, 2019

cc @ianpartridge Does this seem reasonable when running under Docker? The solutions used in other languages mentioned in https://bugs.swift.org/browse/SR-10611?filter=10905 seem to use this method.

@spevans
Copy link
Contributor Author

spevans commented May 3, 2019

@swift-ci test

@millenomi
Copy link
Contributor

@swift-ci please test

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Ignoring the build related things, yes, this is the right way to query the CPU information under cgroups.

@@ -451,6 +454,14 @@ CF_PRIVATE CFIndex __CFActiveProcessorCount() {
pcnt = 0;
}
#elif DEPLOYMENT_TARGET_LINUX

#ifdef _SCHED_H
Copy link
Member

Choose a reason for hiding this comment

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

Please use cmake for this:

include(CheckSymbolExists)
include(CheckIncludeFile)
check_include_file("sched.h" HAVE_SCHED_H)
check_symbol_exists(sched_getaffinity "sched.h" HAVE_SCHED_GETAFFINITY)

@spevans
Copy link
Contributor Author

spevans commented May 7, 2019

@swift-ci test

1 similar comment
@spevans
Copy link
Contributor Author

spevans commented May 7, 2019

@swift-ci test

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Thanks for doing the additional cleanups.

if(HAVE_SCHED_GETAFFINITY)
target_compile_definitions(CoreFoundation
PRIVATE
-DHAVE_SCHED_GETAFFINITY)
Copy link
Member

Choose a reason for hiding this comment

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

A level of indentation would be nice.

@spevans
Copy link
Contributor Author

spevans commented May 7, 2019

@swift-ci test and merge

1 similar comment
@spevans
Copy link
Contributor Author

spevans commented May 8, 2019

@swift-ci test and merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants