Skip to content

Commit

Permalink
Merge pull request #3318 from kishen-v/cpupath-fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@iwankgb
iwankgb authored Jun 18, 2023
2 parents a85a001 + b7e6727 commit e9068e3
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 40 deletions.
6 changes: 3 additions & 3 deletions machine/machine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ var (
swapCapacityRegexp = regexp.MustCompile(`SwapTotal:\s*([0-9]+) kB`)
vendorIDRegexp = regexp.MustCompile(`vendor_id\s*:\s*(\w+)`)

cpuBusPath = "/sys/bus/cpu/devices/"
cpuAttributesPath = "/sys/devices/system/cpu/"
isMemoryController = regexp.MustCompile("mc[0-9]+")
isDimm = regexp.MustCompile("dimm[0-9]+")
machineArch = getMachineArch()
Expand Down Expand Up @@ -76,7 +76,7 @@ func GetPhysicalCores(procInfo []byte) int {
if numCores == 0 {
// read number of cores from /sys/bus/cpu/devices/cpu*/topology/core_id to deal with processors
// for which 'core id' is not available in /proc/cpuinfo
numCores = sysfs.GetUniqueCPUPropertyCount(cpuBusPath, sysfs.CPUCoreID)
numCores = sysfs.GetUniqueCPUPropertyCount(cpuAttributesPath, sysfs.CPUCoreID)
}
if numCores == 0 {
klog.Errorf("Cannot read number of physical cores correctly, number of cores set to %d", numCores)
Expand All @@ -90,7 +90,7 @@ func GetSockets(procInfo []byte) int {
if numSocket == 0 {
// read number of sockets from /sys/bus/cpu/devices/cpu*/topology/physical_package_id to deal with processors
// for which 'physical id' is not available in /proc/cpuinfo
numSocket = sysfs.GetUniqueCPUPropertyCount(cpuBusPath, sysfs.CPUPhysicalPackageID)
numSocket = sysfs.GetUniqueCPUPropertyCount(cpuAttributesPath, sysfs.CPUPhysicalPackageID)
}
if numSocket == 0 {
klog.Errorf("Cannot read number of sockets correctly, number of sockets set to %d", numSocket)
Expand Down
30 changes: 15 additions & 15 deletions machine/topology_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,12 @@ func TestPhysicalCores(t *testing.T) {
}

func TestPhysicalCoresReadingFromCpuBus(t *testing.T) {
origCPUBusPath := cpuBusPath
origCPUAttributesPath := cpuAttributesPath
defer func() {
cpuBusPath = origCPUBusPath
cpuAttributesPath = origCPUAttributesPath
}()
cpuBusPath = "./testdata/sysfs_cpus/" // overwriting package variable to mock sysfs
testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without core id
cpuAttributesPath = "./testdata/sysfs_cpus/" // overwriting package variable to mock sysfs
testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without core id

testcpuinfo, err := os.ReadFile(testfile)
assert.Nil(t, err)
Expand All @@ -57,12 +57,12 @@ func TestPhysicalCoresReadingFromCpuBus(t *testing.T) {
}

func TestPhysicalCoresFromWrongSysFs(t *testing.T) {
origCPUBusPath := cpuBusPath
origCPUAttributesPath := cpuAttributesPath
defer func() {
cpuBusPath = origCPUBusPath
cpuAttributesPath = origCPUAttributesPath
}()
cpuBusPath = "./testdata/wrongsysfs" // overwriting package variable to mock sysfs
testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without core id
cpuAttributesPath = "./testdata/wrongsysfs" // overwriting package variable to mock sysfs
testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without core id

testcpuinfo, err := os.ReadFile(testfile)
assert.Nil(t, err)
Expand All @@ -84,12 +84,12 @@ func TestSockets(t *testing.T) {
}

func TestSocketsReadingFromCpuBus(t *testing.T) {
origCPUBusPath := cpuBusPath
origCPUAttributesPath := cpuAttributesPath
defer func() {
cpuBusPath = origCPUBusPath
cpuAttributesPath = origCPUAttributesPath
}()
cpuBusPath = "./testdata/wrongsysfs" // overwriting package variable to mock sysfs
testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without physical id
cpuAttributesPath = "./testdata/wrongsysfs" // overwriting package variable to mock sysfs
testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without physical id

testcpuinfo, err := os.ReadFile(testfile)
assert.Nil(t, err)
Expand All @@ -103,11 +103,11 @@ func TestSocketsReadingFromWrongSysFs(t *testing.T) {
path, err := filepath.Abs("./testdata/sysfs_cpus/")
assert.NoError(t, err)

origCPUBusPath := cpuBusPath
origCPUAttributesPath := cpuAttributesPath
defer func() {
cpuBusPath = origCPUBusPath
cpuAttributesPath = origCPUAttributesPath
}()
cpuBusPath = path // overwriting package variable to mock sysfs
cpuAttributesPath = path // overwriting package variable to mock sysfs
testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without physical id

testcpuinfo, err := os.ReadFile(testfile)
Expand Down
37 changes: 20 additions & 17 deletions utils/sysfs/sysfs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,14 @@ type SysFs interface {
IsCPUOnline(dir string) bool
}

type realSysFs struct{}
type realSysFs struct {
cpuPath string
}

func NewRealSysFs() SysFs {
return &realSysFs{}
return &realSysFs{
cpuPath: "/sys/devices/system/cpu",
}
}

func (fs *realSysFs) GetNodesPaths() ([]string, error) {
Expand Down Expand Up @@ -402,19 +406,19 @@ func (fs *realSysFs) GetSystemUUID() (string, error) {
}

func (fs *realSysFs) IsCPUOnline(cpuPath string) bool {
onlinePath, err := filepath.Abs(cpuPath + "/../online")
cpuOnlinePath, err := filepath.Abs(fs.cpuPath + "/online")
if err != nil {
klog.V(1).Infof("Unable to get absolute path for %s", cpuPath)
return false
}

// Quick check to determine if file exists: if it does not then kernel CPU hotplug is disabled and all CPUs are online.
_, err = os.Stat(onlinePath)
_, err = os.Stat(cpuOnlinePath)
if err != nil && os.IsNotExist(err) {
return true
}
if err != nil {
klog.V(1).Infof("Unable to stat %s: %s", onlinePath, err)
klog.V(1).Infof("Unable to stat %s: %s", cpuOnlinePath, err)
}

cpuID, err := getCPUID(cpuPath)
Expand All @@ -423,7 +427,7 @@ func (fs *realSysFs) IsCPUOnline(cpuPath string) bool {
return false
}

isOnline, err := isCPUOnline(onlinePath, cpuID)
isOnline, err := isCPUOnline(cpuOnlinePath, cpuID)
if err != nil {
klog.V(1).Infof("Unable to get online CPUs list: %s", err)
return false
Expand Down Expand Up @@ -475,10 +479,9 @@ func isCPUOnline(path string, cpuID uint16) (bool, error) {
if min > max {
return false, fmt.Errorf("invalid values in %s", path)
}
for i := min; i <= max; i++ {
if uint16(i) == cpuID {
return true, nil
}
// Return true, if the CPU under consideration is in the range of online CPUs.
if cpuID >= uint16(min) && cpuID <= uint16(max) {
return true, nil
}
case 1:
value, err := strconv.ParseUint(s, 10, 16)
Expand All @@ -496,21 +499,21 @@ func isCPUOnline(path string, cpuID uint16) (bool, error) {

// Looks for sysfs cpu path containing given CPU property, e.g. core_id or physical_package_id
// and returns number of unique values of given property, exemplary usage: getting number of CPU physical cores
func GetUniqueCPUPropertyCount(cpuBusPath string, propertyName string) int {
absCPUBusPath, err := filepath.Abs(cpuBusPath)
func GetUniqueCPUPropertyCount(cpuAttributesPath string, propertyName string) int {
absCPUAttributesPath, err := filepath.Abs(cpuAttributesPath)
if err != nil {
klog.Errorf("Cannot make %s absolute", cpuBusPath)
klog.Errorf("Cannot make %s absolute", cpuAttributesPath)
return 0
}
pathPattern := absCPUBusPath + "/cpu*[0-9]"
pathPattern := absCPUAttributesPath + "/cpu*[0-9]"
sysCPUPaths, err := filepath.Glob(pathPattern)
if err != nil {
klog.Errorf("Cannot find files matching pattern (pathPattern: %s), number of unique %s set to 0", pathPattern, propertyName)
return 0
}
onlinePath, err := filepath.Abs(cpuBusPath + "/online")
cpuOnlinePath, err := filepath.Abs(cpuAttributesPath + "/online")
if err != nil {
klog.V(1).Infof("Unable to get absolute path for %s", cpuBusPath+"/../online")
klog.V(1).Infof("Unable to get absolute path for %s", cpuAttributesPath+"/../online")
return 0
}

Expand All @@ -525,7 +528,7 @@ func GetUniqueCPUPropertyCount(cpuBusPath string, propertyName string) int {
klog.V(1).Infof("Unable to get CPU ID from path %s: %s", sysCPUPath, err)
return 0
}
isOnline, err := isCPUOnline(onlinePath, cpuID)
isOnline, err := isCPUOnline(cpuOnlinePath, cpuID)
if err != nil && !os.IsNotExist(err) {
klog.V(1).Infof("Unable to determine CPU online state: %s", err)
continue
Expand Down
20 changes: 15 additions & 5 deletions utils/sysfs/sysfs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,9 @@ func TestGetHugePagesNrWhenFileIsMissing(t *testing.T) {
}

func TestIsCPUOnline(t *testing.T) {
sysFs := NewRealSysFs()
sysFs := &realSysFs{
cpuPath: "./testdata_epyc7402_nohyperthreading",
}
online := sysFs.IsCPUOnline("./testdata_epyc7402_nohyperthreading/cpu14")
assert.True(t, online)

Expand All @@ -141,7 +143,9 @@ func TestIsCPUOnlineNoFileAndCPU0MustBeOnline(t *testing.T) {
}()
isX86 = false

sysFs := NewRealSysFs()
sysFs := &realSysFs{
cpuPath: "./testdata/missing_online/node0",
}
online := sysFs.IsCPUOnline("./testdata/missing_online/node0/cpu33")
assert.True(t, online)
}
Expand All @@ -167,7 +171,9 @@ func TestCPU0OfflineOnNotx86(t *testing.T) {
}()
isX86 = false

sysFs := NewRealSysFs()
sysFs := &realSysFs{
cpuPath: "./testdata_graviton2",
}
online := sysFs.IsCPUOnline("./testdata_graviton2/cpu0")
assert.False(t, online)
}
Expand All @@ -180,7 +186,9 @@ func TestIsCpuOnlineRaspberryPi4(t *testing.T) {
}()
isX86 = false

sysFS := NewRealSysFs()
sysFS := &realSysFs{
cpuPath: "./testdata_rpi4",
}
online := sysFS.IsCPUOnline("./testdata_rpi4/cpu0")
assert.True(t, online)

Expand All @@ -202,7 +210,9 @@ func TestIsCpuOnlineGraviton2(t *testing.T) {
}()
isX86 = false

sysFS := NewRealSysFs()
sysFS := &realSysFs{
cpuPath: "./testdata_graviton2",
}
online := sysFS.IsCPUOnline("./testdata_graviton2/cpu0")
assert.False(t, online)

Expand Down

0 comments on commit e9068e3

Please sign in to comment.