From 5c6eed8e7fbb2ae7545cb1d2838eccacfba7be42 Mon Sep 17 00:00:00 2001 From: Markus Lehtonen Date: Fri, 5 Jan 2024 14:06:21 +0200 Subject: [PATCH] pkg/sysfs: drop discovery flags Simplify the code by dropping unused functionality. Effectively, we always used the default flags. --- pkg/cpuallocator/cpuallocator_test.go | 4 +- .../builtin/topology-aware/mocks_test.go | 2 +- pkg/sysfs/system.go | 151 ++---------------- 3 files changed, 16 insertions(+), 141 deletions(-) diff --git a/pkg/cpuallocator/cpuallocator_test.go b/pkg/cpuallocator/cpuallocator_test.go index d6d19a53c..0a5c69bfa 100644 --- a/pkg/cpuallocator/cpuallocator_test.go +++ b/pkg/cpuallocator/cpuallocator_test.go @@ -37,9 +37,7 @@ func TestAllocatorHelper(t *testing.T) { } // Discover mock system from the testdata - sys, err := sysfs.DiscoverSystemAt( - path.Join(tmpdir, "sysfs", "2-socket-4-node-40-core", "sys"), - sysfs.DiscoverCPUTopology, sysfs.DiscoverMemTopology) + sys, err := sysfs.DiscoverSystemAt(path.Join(tmpdir, "sysfs", "2-socket-4-node-40-core", "sys")) if err != nil { t.Fatalf("failed to discover mock system: %v", err) } diff --git a/pkg/cri/resource-manager/policy/builtin/topology-aware/mocks_test.go b/pkg/cri/resource-manager/policy/builtin/topology-aware/mocks_test.go index 674f46f41..9df06f99f 100644 --- a/pkg/cri/resource-manager/policy/builtin/topology-aware/mocks_test.go +++ b/pkg/cri/resource-manager/policy/builtin/topology-aware/mocks_test.go @@ -183,7 +183,7 @@ func (fake *mockSystem) CPUCount() int { } return fake.cpuCount } -func (fake *mockSystem) Discover(flags system.DiscoveryFlag) error { +func (fake *mockSystem) Discover() error { return nil } func (fake *mockSystem) Package(idset.ID) system.CPUPackage { diff --git a/pkg/sysfs/system.go b/pkg/sysfs/system.go index 0616b137b..8d8fd5878 100644 --- a/pkg/sysfs/system.go +++ b/pkg/sysfs/system.go @@ -41,26 +41,6 @@ const ( sysfsNumaNodePath = "devices/system/node" ) -// DiscoveryFlag controls what hardware details to discover. -type DiscoveryFlag uint - -const ( - // DiscoverCPUTopology requests discovering CPU topology details. - DiscoverCPUTopology DiscoveryFlag = 1 << iota - // DiscoverMemTopology requests discovering memory topology details. - DiscoverMemTopology - // DiscoverCache requests discovering CPU cache details. - DiscoverCache - // DiscoverSst requests discovering details of Intel Speed Select Technology - DiscoverSst - // DiscoverNone is the zero value for discovery flags. - DiscoverNone DiscoveryFlag = 0 - // DiscoverAll requests full supported discovery. - DiscoverAll DiscoveryFlag = 0xffffffff - // DiscoverDefault is the default set of discovery flags. - DiscoverDefault DiscoveryFlag = (DiscoverCPUTopology | DiscoverMemTopology | DiscoverSst) -) - // MemoryType is an enum for the Node memory type MemoryType int @@ -75,7 +55,7 @@ const ( // System devices type System interface { - Discover(flags DiscoveryFlag) error + Discover() error SetCpusOnline(online bool, cpus idset.IDSet) (idset.IDSet, error) SetCPUFrequencyLimits(min, max uint64, cpus idset.IDSet) error PackageIDs() []idset.ID @@ -98,7 +78,6 @@ type System interface { // System devices type system struct { logger.Logger // our logger instance - flags DiscoveryFlag // system discovery flags path string // sysfs mount point packages map[idset.ID]*cpuPackage // physical packages nodes map[idset.ID]*node // NUMA nodes @@ -254,25 +233,14 @@ func DiscoverSystem() (System, error) { } // DiscoverSystemAt performs discovery of the running systems details from sysfs mounted at path. -func DiscoverSystemAt(path string, args ...DiscoveryFlag) (System, error) { - var flags DiscoveryFlag - - if len(args) < 1 { - flags = DiscoverDefault - } else { - flags = DiscoverNone - for _, flag := range args { - flags |= flag - } - } - +func DiscoverSystemAt(path string) (System, error) { sys := &system{ Logger: logger.NewLogger("sysfs"), path: path, offline: idset.NewIDSet(), } - if err := sys.Discover(flags); err != nil { + if err := sys.Discover(); err != nil { return nil, err } @@ -280,32 +248,20 @@ func DiscoverSystemAt(path string, args ...DiscoveryFlag) (System, error) { } // Discover performs system/hardware discovery. -func (sys *system) Discover(flags DiscoveryFlag) error { - sys.flags |= (flags &^ DiscoverCache) - - if (sys.flags & (DiscoverCPUTopology | DiscoverCache | DiscoverSst)) != 0 { - if err := sys.discoverCPUs(); err != nil { - return err - } - if err := sys.discoverNodes(); err != nil { - return err - } - if err := sys.discoverPackages(); err != nil { - return err - } +func (sys *system) Discover() error { + if err := sys.discoverCPUs(); err != nil { + return err } - - if (sys.flags & DiscoverSst) != 0 { - if err := sys.discoverSst(); err != nil { - // Just consider SST unsupported if our detection fails for some reason - sys.Warn("%v", err) - } + if err := sys.discoverNodes(); err != nil { + return err + } + if err := sys.discoverPackages(); err != nil { + return err } - if (sys.flags & DiscoverMemTopology) != 0 { - if err := sys.discoverNodes(); err != nil { - return err - } + if err := sys.discoverSst(); err != nil { + // Just consider SST unsupported if our detection fails for some reason + sys.Warn("%v", err) } if len(sys.nodes) > 0 { @@ -625,15 +581,6 @@ func (sys *system) discoverCPU(path string) error { sys.cpus[cpu.id] = cpu - if (sys.flags & DiscoverCache) != 0 { - entries, _ := filepath.Glob(filepath.Join(path, "cache/index[0-9]*")) - for _, entry := range entries { - if err := sys.discoverCache(entry); err != nil { - return err - } - } - } - return nil } @@ -1062,76 +1009,6 @@ func (p *cpuPackage) SstInfo() *sst.SstPackageInfo { return p.sstInfo } -// Discover cache associated with the given CPU. -// -// Notes: -// -// I'm not sure how to interpret the cache information under sysfs. -// This code is now effectively disabled by forcing the associated -// discovery bit off in the discovery flags. -func (sys *system) discoverCache(path string) error { - var id idset.ID - - if _, err := readSysfsEntry(path, "id", &id); err != nil { - return sysfsError(path, "can't read cache id: %v", err) - } - - if sys.cache == nil { - sys.cache = make(map[idset.ID]*Cache) - } - - if _, found := sys.cache[id]; found { - return nil - } - - c := &Cache{id: id} - - if _, err := readSysfsEntry(path, "level", &c.level); err != nil { - return sysfsError(path, "can't read cache level: %v", err) - } - if _, err := readSysfsEntry(path, "shared_cpu_list", &c.cpus, ","); err != nil { - return sysfsError(path, "can't read shared CPUs: %v", err) - } - kind := "" - if _, err := readSysfsEntry(path, "type", &kind); err != nil { - return sysfsError(path, "can't read cache type: %v", err) - } - switch kind { - case "Data": - c.kind = DataCache - case "Instruction": - c.kind = InstructionCache - case "Unified": - c.kind = UnifiedCache - default: - return sysfsError(path, "unknown cache type: %s", kind) - } - - size := "" - if _, err := readSysfsEntry(path, "size", &size); err != nil { - return sysfsError(path, "can't read cache size: %v", err) - } - - base := size[0 : len(size)-1] - suff := size[len(size)-1] - unit := map[byte]uint64{'K': 1 << 10, 'M': 1 << 20, 'G': 1 << 30} - - val, err := strconv.ParseUint(base, 10, 0) - if err != nil { - return sysfsError(path, "can't parse cache size '%s': %v", size, err) - } - - if u, ok := unit[suff]; ok { - c.size = val * u - } else { - c.size = val*1000 + u - '0' - } - - sys.cache[c.id] = c - - return nil -} - // eppStrings initialized this way to better catch changes in the enum var eppStrings = func() [EPPUnknown]string { var e [EPPUnknown]string