Possible bug in parsing core.ID and core.Index
cyga opened this issue · 10 comments
cyga commented
In changes between v0.10.0 to v0.11.0
(more precisely in git diff 59f01e7caa962f36aef7293abaab6a4f770391ba 34f45f4cfdb628931eb1732fa414a7ae97191354)
the following changes were introduced:
v0.10.0:
ID = "core id" from /proc/cpuinfo
Index = index in /proc/cpuinfo
v0.11.0:
ID = not set (0)
Index = value from /sys/devices/system/cpu/cpuXX/topology/core_id
Is it a bug or a feature?
- Documentation wasn't changed for it:
ghw.ProcessorCore.ID is the uint32 identifier that the host gave this core. Note that this does not necessarily equate to a zero-based index of the core within a physical package. For example, the core IDs for an Intel Core i7 are 0, 1, 2, 8, 9, and 10
ghw.ProcessorCore.Index is the zero-based index of the core on the physical processor package
- If this is designed behaviour can you at least keep ID same as Index (as far as I can see, "core id" has the same value on my linuxes as "/sys/devices/system/cpu/cpuXX/topology/core_id") for some period. So, we can adopt this change during transition period?
jaypipes commented
Hi @cyga! Would you mind providing some details about your host system? Specifically what architecture are you using? To answer your question, no, it was not the intention to change these field values, though the implementation of CPU information gathering was indeed refactored in the v0.11.0 timeframe. /cc @rockrush @glimchb
glimchb commented
from a quick look without any testing yet looks like this might be a fix:
diff --git a/pkg/cpu/cpu_linux.go b/pkg/cpu/cpu_linux.go
index d4b048d..274ca05 100644
--- a/pkg/cpu/cpu_linux.go
+++ b/pkg/cpu/cpu_linux.go
@@ -138,7 +138,7 @@ func processorsGet(ctx *context.Context) []*Processor {
coreID := util.SafeIntFromFile(ctx, coreIdPath)
core := CoreByID(proc.Cores, coreID)
if core == nil {
- core = &ProcessorCore{Index: coreID, NumThreads: 1}
+ core = &ProcessorCore{ID: coreID, Index: len(proc.Cores), NumThreads: 1}
proc.Cores = append(proc.Cores, core)
proc.NumCores += 1
} else {
(END)
cyga commented
Hi @cyga! Would you mind providing some details about your host system? Specifically what architecture are you using? To answer your question, no, it was not the intention to change these field values, though the implementation of CPU information gathering was indeed refactored in the v0.11.0 timeframe. /cc @rockrush @glimchb
$ uname -m
x86_64
CPUs layout using snapshot made previously and using v0.10.0:
$ cat main.go
package main
import (
"fmt"
"github.com/jaypipes/ghw"
)
func main() {
info, err := ghw.CPU(ghw.WithSnapshot(ghw.SnapshotOptions{Path: "testdata/linux-amd64-7407565ac48041e48099d838f0818125.tar.gz"}))
if err != nil {
panic(err)
}
for _, processor := range info.Processors {
fmt.Printf("processor ID=%d\n", processor.ID)
for _, core := range processor.Cores {
fmt.Printf(" core id=%d, index=%d\n", core.ID, core.Index)
for _, logCore := range core.LogicalProcessors {
fmt.Printf(" log core ID=%d\n", logCore)
}
}
}
}
$ go run main.go
processor ID=0
core id=0, index=0
log core ID=0
log core ID=16
core id=1, index=1
log core ID=2
log core ID=18
core id=2, index=2
log core ID=4
log core ID=20
core id=3, index=3
log core ID=6
log core ID=22
core id=4, index=4
log core ID=8
log core ID=24
core id=5, index=5
log core ID=10
log core ID=26
core id=6, index=6
log core ID=12
log core ID=28
core id=7, index=7
log core ID=14
log core ID=30
processor ID=1
core id=0, index=0
log core ID=1
log core ID=17
core id=1, index=1
log core ID=3
log core ID=19
core id=2, index=2
log core ID=5
log core ID=21
core id=3, index=3
log core ID=7
log core ID=23
core id=4, index=4
log core ID=9
log core ID=25
core id=5, index=5
log core ID=11
log core ID=27
core id=6, index=6
log core ID=13
log core ID=29
core id=7, index=7
log core ID=15
log core ID=31
cyga commented
from a quick look without any testing yet looks like this might be a fix:
diff --git a/pkg/cpu/cpu_linux.go b/pkg/cpu/cpu_linux.go index d4b048d..274ca05 100644 --- a/pkg/cpu/cpu_linux.go +++ b/pkg/cpu/cpu_linux.go @@ -138,7 +138,7 @@ func processorsGet(ctx *context.Context) []*Processor { coreID := util.SafeIntFromFile(ctx, coreIdPath) core := CoreByID(proc.Cores, coreID) if core == nil { - core = &ProcessorCore{Index: coreID, NumThreads: 1} + core = &ProcessorCore{ID: coreID, Index: len(proc.Cores), NumThreads: 1} proc.Cores = append(proc.Cores, core) proc.NumCores += 1 } else { (END)
I also thought of smth similar, but it looks like cores are groupped differently in that case.
For example, the following is the output for the program I posted above with the patch you showed:
processor ID=0
core id=0, index=0
log core ID=0
log core ID=16
core id=5, index=1
log core ID=10
log core ID=18
log core ID=2
core id=6, index=2
log core ID=12
log core ID=20
log core ID=4
core id=7, index=3
log core ID=14
log core ID=22
log core ID=6
core id=4, index=4
log core ID=24
log core ID=8
core id=5, index=5
log core ID=26
core id=6, index=6
log core ID=28
core id=7, index=7
log core ID=30
processor ID=1
core id=0, index=0
log core ID=1
log core ID=17
core id=5, index=1
log core ID=11
log core ID=19
log core ID=3
core id=6, index=2
log core ID=13
log core ID=21
log core ID=5
core id=7, index=3
log core ID=15
log core ID=23
log core ID=7
core id=4, index=4
log core ID=25
log core ID=9
core id=5, index=5
log core ID=27
core id=6, index=6
log core ID=29
core id=7, index=7
log core ID=31
cyga commented
from a quick look without any testing yet looks like this might be a fix:
diff --git a/pkg/cpu/cpu_linux.go b/pkg/cpu/cpu_linux.go index d4b048d..274ca05 100644 --- a/pkg/cpu/cpu_linux.go +++ b/pkg/cpu/cpu_linux.go @@ -138,7 +138,7 @@ func processorsGet(ctx *context.Context) []*Processor { coreID := util.SafeIntFromFile(ctx, coreIdPath) core := CoreByID(proc.Cores, coreID) if core == nil { - core = &ProcessorCore{Index: coreID, NumThreads: 1} + core = &ProcessorCore{ID: coreID, Index: len(proc.Cores), NumThreads: 1} proc.Cores = append(proc.Cores, core) proc.NumCores += 1 } else { (END)
It's more like:
diff --git a/pkg/cpu/cpu_linux.go b/pkg/cpu/cpu_linux.go
index d4b048d..3cec259 100644
--- a/pkg/cpu/cpu_linux.go
+++ b/pkg/cpu/cpu_linux.go
@@ -48,7 +48,7 @@ func ProcByID(procs []*Processor, id int) *Processor {
func CoreByID(cores []*ProcessorCore, id int) *ProcessorCore {
for cid := range cores {
- if cores[cid].Index == id {
+ if cores[cid].ID == id {
return cores[cid]
}
}
@@ -138,7 +138,11 @@ func processorsGet(ctx *context.Context) []*Processor {
coreID := util.SafeIntFromFile(ctx, coreIdPath)
core := CoreByID(proc.Cores, coreID)
if core == nil {
- core = &ProcessorCore{Index: coreID, NumThreads: 1}
+ core = &ProcessorCore{
+ ID: coreID,
+ Index: len(proc.Cores),
+ NumThreads: 1,
+ }
proc.Cores = append(proc.Cores, core)
proc.NumCores += 1
} else {
but the output is not exactly the same I need to better check it later.
jaypipes commented
I can indeed confirm that the indexing of logical processors changed from v0.10.0 to the latest commit on main (36ff37e):
jaypipes@thelio:~/src/github.com/jaypipes/ghw$ go run cmd/ghwc/main.go cpu
cpu (1 physical package, 8 cores, 16 hardware threads)
physical package #0 (8 cores, 16 hardware threads)
processor core #0 (2 threads), logical processors [0 8]
processor core #1 (2 threads), logical processors [1 9]
processor core #2 (2 threads), logical processors [10 2]
processor core #3 (2 threads), logical processors [11 3]
processor core #4 (2 threads), logical processors [12 4]
processor core #5 (2 threads), logical processors [13 5]
processor core #6 (2 threads), logical processors [14 6]
processor core #7 (2 threads), logical processors [15 7]
capabilities: [msr pae mce cx8 apic sep
mtrr pge mca cmov pat pse36
clflush mmx fxsr sse sse2 ht
syscall nx mmxext fxsr_opt pdpe1gb rdtscp
lm constant_tsc rep_good nopl nonstop_tsc cpuid
extd_apicid aperfmperf rapl pni pclmulqdq monitor
ssse3 fma cx16 sse4_1 sse4_2 movbe
popcnt aes xsave avx f16c rdrand
lahf_lm cmp_legacy svm extapic cr8_legacy abm
sse4a misalignsse 3dnowprefetch osvw skinit wdt
tce topoext perfctr_core perfctr_nb bpext perfctr_llc
mwaitx cpb hw_pstate ssbd ibpb vmmcall
fsgsbase bmi1 avx2 smep bmi2 rdseed
adx smap clflushopt sha_ni xsaveopt xsavec
xgetbv1 xsaves clzero irperf xsaveerptr arat
npt lbrv svm_lock nrip_save tsc_scale vmcb_clean
flushbyasid decodeassists pausefilter pfthreshold avic v_vmsave_vmload
vgif overflow_recov succor smca sev sev_es]
jaypipes@thelio:~/src/github.com/jaypipes/ghw$ git checkout v0.10.0
Note: switching to 'v0.10.0'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:
git switch -c <new-branch-name>
Or undo this operation with:
git switch -
Turn off this advice by setting config variable advice.detachedHead to false
HEAD is now at ae21148 Merge pull request #330 from jaypipes/issue-267
jaypipes@thelio:~/src/github.com/jaypipes/ghw$ go run cmd/ghwc/main.go cpu
cpu (1 physical package, 8 cores, 16 hardware threads)
physical package #0 (8 cores, 16 hardware threads)
processor core #0 (2 threads), logical processors [0 8]
processor core #1 (2 threads), logical processors [1 9]
processor core #2 (2 threads), logical processors [2 10]
processor core #3 (2 threads), logical processors [3 11]
processor core #4 (2 threads), logical processors [4 12]
processor core #5 (2 threads), logical processors [5 13]
processor core #6 (2 threads), logical processors [6 14]
processor core #7 (2 threads), logical processors [7 15]
capabilities: [msr pae mce cx8 apic sep
mtrr pge mca cmov pat pse36
clflush mmx fxsr sse sse2 ht
syscall nx mmxext fxsr_opt pdpe1gb rdtscp
lm constant_tsc rep_good nopl nonstop_tsc cpuid
extd_apicid aperfmperf rapl pni pclmulqdq monitor
ssse3 fma cx16 sse4_1 sse4_2 movbe
popcnt aes xsave avx f16c rdrand
lahf_lm cmp_legacy svm extapic cr8_legacy abm
sse4a misalignsse 3dnowprefetch osvw skinit wdt
tce topoext perfctr_core perfctr_nb bpext perfctr_llc
mwaitx cpb hw_pstate ssbd ibpb vmmcall
fsgsbase bmi1 avx2 smep bmi2 rdseed
adx smap clflushopt sha_ni xsaveopt xsavec
xgetbv1 xsaves clzero irperf xsaveerptr arat
npt lbrv svm_lock nrip_save tsc_scale vmcb_clean
flushbyasid decodeassists pausefilter pfthreshold avic v_vmsave_vmload
vgif overflow_recov succor smca sev sev_es]
Working on a fix now...
jaypipes commented
Never mind... the logical processors are the same. It's just that the slice of logical processor IDs is in a different order...
jaypipes commented
glimchb commented
on it @jaypipes
renovate already picked it up opiproject/opi-smbios-bridge#131