Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

fix Issue 19861 - core.cpuid reports the wrong number of threads #2620

Merged
merged 2 commits into from
Jun 28, 2019

Conversation

rainers
Copy link
Member

@rainers rainers commented May 30, 2019

do not use i7 detection on AMD processors
use cpuid 0x8000_001E to detect the number of threads per core

It seems AMD uses the term "core" for "logical cores" or threads, too.

@dlang-bot
Copy link
Contributor

dlang-bot commented May 30, 2019

Thanks for your pull request, @rainers!

Bugzilla references

Auto-close Bugzilla Severity Description
19861 minor core.cpuid reports the wrong number of threads

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + druntime#2620"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label May 30, 2019
@rainers
Copy link
Member Author

rainers commented May 30, 2019

@FraMecca This worked for an AMD Ryzen, could you check it on your system, too?

@thewilsonator
Copy link
Contributor

Enforce whitespace before opening parenthesis
--
  | grep -nrE "\<(for\|foreach\|foreach_reverse\|if\|while\|switch\|catch\|version)\(" $(find src -name '*.d') ; test $? -eq 1
  | src/core/cpuid.d:949:            version(GNU) asm pure nothrow @nogc {
  | posix.mak:383: recipe for target 'style_lint' failed

@rainers
Copy link
Member Author

rainers commented Jun 5, 2019

Anyone else with an AMD processor who can try this? Running and verifying this should be enough:

import core.cpuid;
import core.stdc.stdio;

void main()
{
    printf("coresPerCPU: %d\n", coresPerCPU);
    printf("threadsPerCPU: %d\n", threadsPerCPU);
}

Do we know whether there is an AMD used by any CI?

Parallel marking relies on this information, so would be good to have this in before the next release.

@JinShil
Copy link
Contributor

JinShil commented Jun 5, 2019

Anyone else with an AMD processor who can try this?

cc @baziotis

@baziotis
Copy link
Contributor

baziotis commented Jun 5, 2019

I have an AMD Ryzen 7 1700. It is 8-core processor, with 2 threads per core.
But I get:

coresPerCPU: 16
threadsPerCPU: 1

lscpu data:

CPU(s):              16
Thread(s) per core:  2
Core(s) per socket:  8

@rainers
Copy link
Member Author

rainers commented Jun 5, 2019

have an AMD Ryzen 7 1700. It is 8-core processor, with 2 threads per core.
But I get:
coresPerCPU: 16
threadsPerCPU: 1

Thanks for testing. I guess this is without this patch? With the patch it should be correct (8/16), but unfortunately your CPU is exactly the same I've been trying it on.

@baziotis
Copy link
Contributor

baziotis commented Jun 6, 2019

but unfortunately your CPU is exactly the same I've been trying it on.

What are the odds..

I guess this is without this patch? With the patch it should be correct (8/16)

Yes and yes.

do not use i7 detection on AMD processors
use cpuid 0x8000_001E to detect the number of threads per core
@rainers rainers changed the base branch from master to stable June 17, 2019 05:58
@rainers
Copy link
Member Author

rainers commented Jun 17, 2019

Rebased on stable as this is important for parallel marking to work on AMD systems.
I added a test to print out cpuid values, mostly to see if there is an AMD processor in our test fleet.

@rainers rainers force-pushed the issue_19861 branch 2 times, most recently from 71c0eb5 to 3a5db12 Compare June 17, 2019 06:28
@rainers
Copy link
Member Author

rainers commented Jun 17, 2019

auto tester redmond reports dubious values for Intel Core2, too:

processor: Intel(R) Core(TM)2 Quad CPU    Q9400  @ 2.66GHz
threadsPerCPU:    1
coresPerCPU:      4

@FraMecca
Copy link

Sorry for the delay. It works on my machines.

@rainers
Copy link
Member Author

rainers commented Jun 18, 2019

Sorry for the delay. It works on my machines.

Thanks for testing. I'll remove the verbose output of all the flags from the test.

@thewilsonator
Copy link
Contributor

this good to go?

@rainers
Copy link
Member Author

rainers commented Jun 27, 2019

this good to go?

I think so. The bad result for Intel Core2 is another issue, but it is a pretty old CPU (released 2008).

@dlang-bot dlang-bot merged commit 3ba98e6 into dlang:stable Jun 28, 2019
tramker added a commit to tramker/druntime that referenced this pull request Sep 25, 2019
fix Issue 19861 - core.cpuid reports the wrong number of threads
merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

Conflicts:
	win32.mak
	win64.mak
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue
Projects
None yet
6 participants