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

feat: Setting the model name for arm based CPUs #1373

Merged
merged 4 commits into from Nov 6, 2022

Conversation

yaozbek
Copy link
Contributor

@yaozbek yaozbek commented Nov 4, 2022

- Added arm model and model name as map.
- The modelName is set again according to the model value when the model name is empty.
- Based on lscpu source code.. https://github.com/util-linux/util-linux/blob/master/sys-utils/lscpu-arm.c

---

Signed-off-by: Yalcin Ozbek <yalcinozbekceng@gmail.com>

The contents of the /proc/cpuinfo file on some of the arm devices we work with are as follows. ModelName value is set to empty.. However, the model name can be obtained with commands such as lscpu. The source code of lscpu can be viewed here
This PR has been created so that the ModelName value is not set empty.

Example 1:

processor	: 0
BogoMIPS	: 400.00
Features	: fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
CPU implementer	: 0x41
CPU architecture: 8
CPU variant	: 0x0
CPU part	: 0xd03
CPU revision	: 4

processor	: 1
BogoMIPS	: 400.00
Features	: fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
CPU implementer	: 0x41
CPU architecture: 8
CPU variant	: 0x0
CPU part	: 0xd03
CPU revision	: 4

processor	: 2
BogoMIPS	: 400.00
Features	: fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
CPU implementer	: 0x41
CPU architecture: 8
CPU variant	: 0x0
CPU part	: 0xd03
CPU revision	: 4

processor	: 3
BogoMIPS	: 400.00
Features	: fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
CPU implementer	: 0x41
CPU architecture: 8
CPU variant	: 0x0
CPU part	: 0xd03
CPU revision	: 4

Example 2:

processor	: 0
BogoMIPS	: 243.75
Features	: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm lrcpc dcpop asimddp ssbs
CPU implementer	: 0x41
CPU architecture: 8
CPU variant	: 0x3
CPU part	: 0xd0c
CPU revision	: 1

processor	: 1
BogoMIPS	: 243.75
Features	: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm lrcpc dcpop asimddp ssbs
CPU implementer	: 0x41
CPU architecture: 8
CPU variant	: 0x3
CPU part	: 0xd0c
CPU revision	: 1

- Added arm model and model name as map.
- The modelName is set again according to the model value when the model name is empty.
- Based on lscpu source code.. https://github.com/util-linux/util-linux/blob/master/sys-utils/lscpu-arm.c

---

Signed-off-by: Yalcin Ozbek <yalcinozbekceng@gmail.com>
Copy link
Owner

@shirou shirou left a comment

Choose a reason for hiding this comment

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

Good catch, thank you!

I have tested on an GCP ARM instance (Thank you, sponsors!), I got nothing on ModelName.
/proc/cpuinfo is as follows and does not contain the model name nor cpu line. So your code is not executed.

% cat /proc/cpuinfo 
processor       : 0
BogoMIPS        : 50.00
Features        : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm lrcpc dcpop asimddp ssbs
CPU implementer : 0x41
CPU architecture: 8
CPU variant     : 0x3
CPU part        : 0xd0c
CPU revision    : 1

I think it would be better to move the code into "model". (Although I am concerned about the order in which "model" and "modelname" appears in the cpuinfo file.)

And also, I don't have a knowledge about model id uniqueness, but is it only for ARM? If so, it would be safer to add a check to see if it is ARM.

cpu/cpu_linux.go Outdated Show resolved Hide resolved
Signed-off-by: Yalcin Ozbek <yalcinozbekceng@gmail.com>
@yaozbek
Copy link
Contributor Author

yaozbek commented Nov 5, 2022

Good catch, thank you!

I have tested on an GCP ARM instance (Thank you, sponsors!), I got nothing on ModelName. /proc/cpuinfo is as follows and does not contain the model name nor cpu line. So your code is not executed.

% cat /proc/cpuinfo 
processor       : 0
BogoMIPS        : 50.00
Features        : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm lrcpc dcpop asimddp ssbs
CPU implementer : 0x41
CPU architecture: 8
CPU variant     : 0x3
CPU part        : 0xd0c
CPU revision    : 1

I think it would be better to move the code into "model". (Although I am concerned about the order in which "model" and "modelname" appears in the cpuinfo file.)

And also, I don't have a knowledge about model id uniqueness, but is it only for ARM? If so, it would be safer to add a check to see if it is ARM.

  • In all ARM devices I work with, the model is added before the model name in /proc/cpuinfo. This is the case with most amd devices I've worked with. Because the model name is mapped to the model. You can review the source code of lscpu. lscpu source code
    For this reason, it will be beneficial for us to have the relevant code in modelName. Also, even if a modelName is defined, we do not overwrite this defined value with the if control.

  • You're right about the other point, these unique hexadecimal values ​​are for ARM only. For this reason, I added the condition that the vendorID should be ARM in the if block.

@shirou
Copy link
Owner

shirou commented Nov 5, 2022

My point is, there is at least one machine which does not have a model name in cpuinfo. This prevents the code itself from executing. How about to move the code to outside of the for-loop, like here?

Signed-off-by: Yalcin Ozbek <yalcinozbekceng@gmail.com>
@yaozbek
Copy link
Contributor Author

yaozbek commented Nov 5, 2022

My point is, there is at least one machine which does not have a model name in cpuinfo. This prevents the code itself from executing. How about to move the code to outside of the for-loop, like here?

you are right. I moved the code.

Signed-off-by: Yalcin Ozbek <yalcinozbekceng@gmail.com>
Copy link
Owner

@shirou shirou left a comment

Choose a reason for hiding this comment

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

I have confirmed it works on GCP ARM instance. Thank you so much!

=== RUN   TestCpuInfo
    cpu_test.go:123: {"cpu":0,"vendorId":"ARM","family":"","model":"0xd0c","stepping":1,"physicalId":"","coreId":"0","cores":1,"modelName":"Neoverse-N1","mhz":0,"cacheSize":0,"flags":["fp","asimd","evtstrm","aes","pmull","sha1","sha2","crc32","atomics","fphp","asimdhp","cpuid","asimdrdm","lrcpc","dcpop","asimddp","ssbs"],"microcode":""}

@shirou shirou merged commit 59246d8 into shirou:master Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants