implement preferAlignByUncorecache within TakeByTopologyNUMAPacked and

test cases

Author: wongchar

pkg/kubelet/cm/cpumanager/cpu_assignment.go

@@ -95,10 +95,8 @@ func standardDeviation(xs []int) float64 {
 type numaOrSocketsFirstFuncs interface {
 	takeFullFirstLevel()
 	takeFullSecondLevel()
-	takeThirdLevel()
 	sortAvailableNUMANodes() []int
 	sortAvailableSockets() []int
-	sortAvailableUncoreCaches() []int
 	sortAvailableCores() []int
 }
 
@@ -120,18 +118,12 @@ func (n *numaFirst) takeFullSecondLevel() {
 	n.acc.takeFullSockets()
 }
 
-// In Split UncoreCache Topology, we take from the sets of UncoreCache as the third level
-func (n *numaFirst) takeThirdLevel() {
-	n.acc.takeUncoreCache()
-}
-
-// If NUMA nodes are higher in the memory hierarchy than sockets, then just
-// sort the NUMA nodes directly, and return them.
-func (n *numaFirst) sortAvailableUncoreCaches() []int {
+// Sort the UncoreCaches within the NUMA nodes.
+func (a *cpuAccumulator) sortAvailableUncoreCaches() []int {
 	var result []int
-	for _, socket := range n.acc.sortAvailableNUMANodes() {
-		uncore := n.acc.details.UncoreInNUMANodes(socket).UnsortedList()
-		n.acc.sort(uncore, n.acc.details.CPUsInUncoreCaches)
+	for _, numa := range a.sortAvailableNUMANodes() {
+		uncore := a.details.UncoreInNUMANodes(numa).UnsortedList()
+		a.sort(uncore, a.details.CPUsInUncoreCaches)
 		result = append(result, uncore...)
 	}
 	return result
@@ -182,10 +174,6 @@ func (s *socketsFirst) takeFullSecondLevel() {
 	s.acc.takeFullNUMANodes()
 }
 
-func (s *socketsFirst) takeThirdLevel() {
-	s.acc.takeUncoreCache()
-}
-
 // If sockets are higher in the memory hierarchy than NUMA nodes, then we need
 // to pull the set of NUMA nodes out of each sorted Socket, and accumulate the
 // partial order across them.
@@ -207,18 +195,6 @@ func (s *socketsFirst) sortAvailableSockets() []int {
 	return sockets
 }
 
-// If sockets  higher in the memory hierarchy than NUMA nodes, then UncoreCache
-// sit directly below NUMA Nodes in the memory hierchy
-func (s *socketsFirst) sortAvailableUncoreCaches() []int {
-	var result []int
-	for _, uncore := range s.acc.sortAvailableNUMANodes() {
-		uncore := s.acc.details.UncoreInNUMANodes(uncore).UnsortedList()
-		s.acc.sort(uncore, s.acc.details.CPUsInUncoreCaches)
-		result = append(result, uncore...)
-	}
-	return result
-}
-
 // If sockets are higher in the memory hierarchy than NUMA nodes, then cores
 // sit directly below NUMA Nodes in the memory hierarchy.
 func (s *socketsFirst) sortAvailableCores() []int {
@@ -353,8 +329,8 @@ func (a *cpuAccumulator) isSocketFree(socketID int) bool {
 	return a.details.CPUsInSockets(socketID).Size() == a.topo.CPUsPerSocket()
 }
 
-// Returns true if the supplied UnCoreCache is fully available in `a.details`.
-// "fully available" means that all the CPUs in it are free.
+// Returns true if the supplied UnCoreCache is fully available,
+// meaning all its CPUs are free in `a.details`.
 func (a *cpuAccumulator) isUncoreCacheFree(uncoreID int) bool {
 	return a.details.CPUsInUncoreCaches(uncoreID).Size() == a.topo.CPUDetails.CPUsInUncoreCaches(uncoreID).Size()
 }
@@ -390,19 +366,14 @@ func (a *cpuAccumulator) freeSockets() []int {
 // Returns free UncoreCache IDs as a slice sorted by sortAvailableUnCoreCache().
 func (a *cpuAccumulator) freeUncoreCache() []int {
 	free := []int{}
-	for _, uncore := range a.numaOrSocketsFirst.sortAvailableUncoreCaches() {
+	for _, uncore := range a.sortAvailableUncoreCaches() {
 		if a.isUncoreCacheFree(uncore) {
 			free = append(free, uncore)
 		}
 	}
 	return free
 }
 
-// Returns all UncoreCache IDs as a slice sorted by sortAvailableUncoreCache().
-func (a *cpuAccumulator) allUncoreCache() []int {
-	return a.numaOrSocketsFirst.sortAvailableUncoreCaches()
-}
-
 // Returns free core IDs as a slice sorted by sortAvailableCores().
 func (a *cpuAccumulator) freeCores() []int {
 	free := []int{}
@@ -575,7 +546,7 @@ func (a *cpuAccumulator) takeFullSockets() {
 		a.take(cpusInSocket)
 	}
 }
-func (a *cpuAccumulator) takeFullUnCore() {
+func (a *cpuAccumulator) takeFullUncore() {
 	for _, uncore := range a.freeUncoreCache() {
 		cpusInUncore := a.topo.CPUDetails.CPUsInUncoreCaches(uncore)
 		if !a.needsAtLeast(cpusInUncore.Size()) {
@@ -585,30 +556,34 @@ func (a *cpuAccumulator) takeFullUnCore() {
 	}
 }
 
+func (a *cpuAccumulator) takePartialUncore(uncoreID int) {
+	numCoresNeeded := a.numCPUsNeeded / a.topo.CPUsPerCore()
+	var freeCPUsInUncoreCache cpuset.CPUSet
+	freeCoresInUncoreCache := a.details.CoresNeededInUncoreCache(numCoresNeeded, uncoreID)
+	for _, coreID := range freeCoresInUncoreCache.List() {
+		freeCPUsInUncoreCache = freeCPUsInUncoreCache.Union(a.topo.CPUDetails.CPUsInCores(coreID))
+	}
+	klog.V(4).InfoS("freeCPUsInUncorecache  : ", "freeCPUsInUncorecache", freeCPUsInUncoreCache.String(), "freeCPUsInUnCoreCache", freeCPUsInUncoreCache.String())
+	if a.numCPUsNeeded == freeCPUsInUncoreCache.Size() {
+		a.take(freeCPUsInUncoreCache)
+	}
+}
+
 // First try to take full UncoreCache, if available and need is at least the size of the UncoreCache group.
 // Second try to take the partial UncoreCache if available and the request size can fit w/in the UncoreCache.
 func (a *cpuAccumulator) takeUncoreCache() {
-	for _, uncore := range a.allUncoreCache() {
-		numCoresNeeded := a.numCPUsNeeded / a.topo.CPUsPerCore()
-
-		// take full UncoreCache if the CPUs needed is greater a UncoreCache size
-		if a.numCPUsNeeded >= a.topo.NumCPUs/a.topo.NumUncoreCache {
-			a.takeFullUnCore()
+	cpusPerUncoreCache := a.topo.NumCPUs / a.topo.NumUncoreCache
+	for _, uncore := range a.sortAvailableUncoreCaches() {
+		// take full UncoreCache if the CPUs needed is greater than free UncoreCache size
+		if a.needsAtLeast(cpusPerUncoreCache) {
+			a.takeFullUncore()
 		}
 
-		var freeCPUsInUncoreCache cpuset.CPUSet
-		// need to get needed cores in UncoreCache
-		freeCoresInUncoreCache := a.details.CoresNeededInUncoreCache(numCoresNeeded, uncore)
-		klog.V(2).InfoS("free cores from a.details list: ", "freeCoresInUncorecache", freeCoresInUncoreCache)
-		for _, coreID := range freeCoresInUncoreCache.List() {
-			freeCPUsInUncoreCache = freeCPUsInUncoreCache.Union(a.topo.CPUDetails.CPUsInCores(coreID))
-		}
-		klog.V(2).InfoS("freeCPUsInUncorecache  : ", "freeCPUsInUncorecache", freeCPUsInUncoreCache)
-		if a.numCPUsNeeded == freeCPUsInUncoreCache.Size() {
-			klog.V(4).InfoS("takePartialUncore: claiming cores from Uncorecache ID", "uncore", uncore)
-			a.take(freeCPUsInUncoreCache)
+		if a.isSatisfied() {
+			return
 		}
 
+		a.takePartialUncore(uncore)
 		if a.isSatisfied() {
 			return
 		}
@@ -733,6 +708,14 @@ func (a *cpuAccumulator) iterateCombinations(n []int, k int, f func([]int) LoopC
 // or the remaining number of CPUs to take after having taken full sockets and NUMA nodes is less
 // than a whole NUMA node, the function tries to take whole physical cores (cores).
 //
+// If `PreferAlignByUncoreCache` is enabled, the function will try to optimally assign Uncorecaches.
+// If `numCPUs` is larger than or equal to the total number of CPUs in a Uncorecache, and there are
+// free (i.e. all CPUs within the Uncorecache are free) Uncorecaches, the function takes as many entire
+// cores from free Uncorecaches as possible. If/Once `numCPUs` is smaller than the total number of
+// CPUs in a free Uncorecache, the function scans each Uncorecache index in numerical order to assign
+// cores that will fit within the Uncorecache. If `numCPUs` cannot fit within any Uncorecache, the
+// function tries to take whole physical cores.
+//
 // If `numCPUs` is bigger than the total number of CPUs in a core, and there are
 // free (i.e. all CPUs in them are free) cores, the function takes as many entire free cores as possible.
 // The cores are taken from one socket at a time, and the sockets are considered by
@@ -754,7 +737,7 @@ func (a *cpuAccumulator) iterateCombinations(n []int, k int, f func([]int) LoopC
 // the least amount of free CPUs to the one with the highest amount of free CPUs (i.e. in ascending
 // order of free CPUs). For any NUMA node, the cores are selected from the ones in the socket with
 // the least amount of free CPUs to the one with the highest amount of free CPUs.
-func takeByTopologyNUMAPacked(topo *topology.CPUTopology, availableCPUs cpuset.CPUSet, numCPUs int, cpuSortingStrategy CPUSortingStrategy) (cpuset.CPUSet, error) {
+func takeByTopologyNUMAPacked(topo *topology.CPUTopology, availableCPUs cpuset.CPUSet, numCPUs int, cpuSortingStrategy CPUSortingStrategy, preferAlignByUncoreCache bool) (cpuset.CPUSet, error) {
 	acc := newCPUAccumulator(topo, availableCPUs, numCPUs, cpuSortingStrategy)
 	if acc.isSatisfied() {
 		return acc.result, nil
@@ -777,59 +760,16 @@ func takeByTopologyNUMAPacked(topo *topology.CPUTopology, availableCPUs cpuset.C
 		return acc.result, nil
 	}
 
-	// 2. Acquire whole cores, if available and the container requires at least
-	//    a core's-worth of CPUs.
-	//    If `CPUSortingStrategySpread` is specified, skip taking the whole core.
-	if cpuSortingStrategy != CPUSortingStrategySpread {
-		acc.takeFullCores()
+	// 2. If PreferAlignByUncoreCache is enabled, acquire whole UncoreCaches
+	//    if available and the container requires at least a UncoreCache's-worth
+	//    of CPUs. Otherwise, acquire CPUs from the least amount of UncoreCaches.
+	if preferAlignByUncoreCache {
+		acc.takeUncoreCache()
 		if acc.isSatisfied() {
 			return acc.result, nil
 		}
 	}
 
-	// 3. Acquire single threads, preferring to fill partially-allocated cores
-	//    on the same sockets as the whole cores we have already taken in this
-	//    allocation.
-	acc.takeRemainingCPUs()
-	if acc.isSatisfied() {
-		return acc.result, nil
-	}
-
-	return cpuset.New(), fmt.Errorf("failed to allocate cpus")
-}
-
-// takeByTopologyUnCoreCachePacked uses the "packed" sorting strategy similar to takeByTopologyNUMAPacked.
-// It includes an additional level of sorting by uncorecache
-func takeByTopologyUncoreCachePacked(topo *topology.CPUTopology, availableCPUs cpuset.CPUSet, numCPUs int, cpuSortingStrategy CPUSortingStrategy) (cpuset.CPUSet, error) {
-	acc := newCPUAccumulator(topo, availableCPUs, numCPUs, cpuSortingStrategy)
-	if acc.isSatisfied() {
-		return acc.result, nil
-	}
-	if acc.isFailed() {
-		return cpuset.New(), fmt.Errorf("not enough cpus available to satisfy request: requested=%d, available=%d", numCPUs, availableCPUs.Size())
-	}
-
-	// Algorithm: topology-aware best-fit
-	// 1. Acquire whole NUMA nodes and sockets, if available and the container
-	//    requires at least a NUMA node or socket's-worth of CPUs. If NUMA
-	//    Nodes map to 1 or more sockets, pull from NUMA nodes first.
-	//    Otherwise pull from sockets first.
-	acc.numaOrSocketsFirst.takeFullFirstLevel()
-	if acc.isSatisfied() {
-		return acc.result, nil
-	}
-	acc.numaOrSocketsFirst.takeFullSecondLevel()
-	if acc.isSatisfied() {
-		return acc.result, nil
-	}
-
-	// 2. Acquire partial uncorecache, if there are enough CPUs available to satisfy the container requirement
-	//    Acquire the full uncorecache, if available and the container requires at least all the CPUs in the uncorecache grouping
-	acc.numaOrSocketsFirst.takeThirdLevel()
-	if acc.isSatisfied() {
-		return acc.result, nil
-	}
-
 	// 3. Acquire whole cores, if available and the container requires at least
 	//    a core's-worth of CPUs.
 	//    If `CPUSortingStrategySpread` is specified, skip taking the whole core.
@@ -918,8 +858,10 @@ func takeByTopologyNUMADistributed(topo *topology.CPUTopology, availableCPUs cpu
 	// If the number of CPUs requested cannot be handed out in chunks of
 	// 'cpuGroupSize', then we just call out the packing algorithm since we
 	// can't distribute CPUs in this chunk size.
+	// PreferAlignByUncoreCache feature not implemented here yet and set to false.
+	// Support for PreferAlignByUncoreCache to be done at beta release.
 	if (numCPUs % cpuGroupSize) != 0 {
-		return takeByTopologyNUMAPacked(topo, availableCPUs, numCPUs, cpuSortingStrategy)
+		return takeByTopologyNUMAPacked(topo, availableCPUs, numCPUs, cpuSortingStrategy, false)
 	}
 
 	// Otherwise build an accumulator to start allocating CPUs from.
@@ -1102,7 +1044,7 @@ func takeByTopologyNUMADistributed(topo *topology.CPUTopology, availableCPUs cpu
 		// size 'cpuGroupSize' from 'bestCombo'.
 		distribution := (numCPUs / len(bestCombo) / cpuGroupSize) * cpuGroupSize
 		for _, numa := range bestCombo {
-			cpus, _ := takeByTopologyNUMAPacked(acc.topo, acc.details.CPUsInNUMANodes(numa), distribution, cpuSortingStrategy)
+			cpus, _ := takeByTopologyNUMAPacked(acc.topo, acc.details.CPUsInNUMANodes(numa), distribution, cpuSortingStrategy, false)
 			acc.take(cpus)
 		}
 
@@ -1117,7 +1059,7 @@ func takeByTopologyNUMADistributed(topo *topology.CPUTopology, availableCPUs cpu
 				if acc.details.CPUsInNUMANodes(numa).Size() < cpuGroupSize {
 					continue
 				}
-				cpus, _ := takeByTopologyNUMAPacked(acc.topo, acc.details.CPUsInNUMANodes(numa), cpuGroupSize, cpuSortingStrategy)
+				cpus, _ := takeByTopologyNUMAPacked(acc.topo, acc.details.CPUsInNUMANodes(numa), cpuGroupSize, cpuSortingStrategy, false)
 				acc.take(cpus)
 				remainder -= cpuGroupSize
 			}
@@ -1141,5 +1083,5 @@ func takeByTopologyNUMADistributed(topo *topology.CPUTopology, availableCPUs cpu
 
 	// If we never found a combination of NUMA nodes that we could properly
 	// distribute CPUs across, fall back to the packing algorithm.
-	return takeByTopologyNUMAPacked(topo, availableCPUs, numCPUs, cpuSortingStrategy)
+	return takeByTopologyNUMAPacked(topo, availableCPUs, numCPUs, cpuSortingStrategy, false)
 }

pkg/kubelet/cm/cpumanager/cpu_assignment_test.go

@@ -668,36 +668,16 @@ func TestTakeByTopologyNUMAPacked(t *testing.T) {
 			"",
 			mustParseCPUSet(t, "0-29,40-69,30,31,70,71"),
 		},
-	}...)
-
-	for _, tc := range testCases {
-		t.Run(tc.description, func(t *testing.T) {
-			strategy := CPUSortingStrategyPacked
-			if tc.opts.DistributeCPUsAcrossCores {
-				strategy = CPUSortingStrategySpread
-			}
-
-			result, err := takeByTopologyNUMAPacked(tc.topo, tc.availableCPUs, tc.numCPUs, strategy)
-			if tc.expErr != "" && err != nil && err.Error() != tc.expErr {
-				t.Errorf("expected error to be [%v] but it was [%v]", tc.expErr, err)
-			}
-			if !result.Equals(tc.expResult) {
-				t.Errorf("expected result [%s] to equal [%s]", result, tc.expResult)
-			}
-		})
-	}
-}
-
-func TestTakeByTopologyUncoreCachePacked(t *testing.T) {
-	testCases := []struct {
-		description   string
-		topo          *topology.CPUTopology
-		opts          StaticPolicyOptions
-		availableCPUs cpuset.CPUSet
-		numCPUs       int
-		expErr        string
-		expResult     cpuset.CPUSet
-	}{
+		// Test cases for PreferAlignByUncoreCache
+		{
+			"take cpus from two full UncoreCaches and partial from a single UncoreCache",
+			topoUncoreSingleSocketNoSMT,
+			StaticPolicyOptions{PreferAlignByUncoreCacheOption: true},
+			mustParseCPUSet(t, "1-15"),
+			10,
+			"",
+			cpuset.New(1, 2, 4, 5, 6, 7, 8, 9, 10, 11),
+		},
 		{
 			"take one cpu from dual socket with HT - core from Socket 0",
 			topoDualSocketHT,
@@ -761,7 +741,7 @@ func TestTakeByTopologyUncoreCachePacked(t *testing.T) {
 			"",
 			mustParseCPUSet(t, "4-7,12-15,1,9"),
 		},
-	}
+	}...)
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
@@ -770,7 +750,7 @@ func TestTakeByTopologyUncoreCachePacked(t *testing.T) {
 				strategy = CPUSortingStrategySpread
 			}
 
-			result, err := takeByTopologyUncoreCachePacked(tc.topo, tc.availableCPUs, tc.numCPUs, strategy)
+			result, err := takeByTopologyNUMAPacked(tc.topo, tc.availableCPUs, tc.numCPUs, strategy, tc.opts.PreferAlignByUncoreCacheOption)
 			if tc.expErr != "" && err != nil && err.Error() != tc.expErr {
 				t.Errorf("expected error to be [%v] but it was [%v]", tc.expErr, err)
 			}
@@ -871,7 +851,7 @@ func TestTakeByTopologyWithSpreadPhysicalCPUsPreferredOption(t *testing.T) {
 		if tc.opts.DistributeCPUsAcrossCores {
 			strategy = CPUSortingStrategySpread
 		}
-		result, err := takeByTopologyNUMAPacked(tc.topo, tc.availableCPUs, tc.numCPUs, strategy)
+		result, err := takeByTopologyNUMAPacked(tc.topo, tc.availableCPUs, tc.numCPUs, strategy, tc.opts.PreferAlignByUncoreCacheOption)
 		if tc.expErr != "" && err.Error() != tc.expErr {
 			t.Errorf("testCase %q failed, expected error to be [%v] but it was [%v]", tc.description, tc.expErr, err)
 		}

pkg/kubelet/cm/cpumanager/policy_static.go

@@ -509,12 +509,7 @@ func (p *staticPolicy) takeByTopology(availableCPUs cpuset.CPUSet, numCPUs int)
 		return takeByTopologyNUMADistributed(p.topology, availableCPUs, numCPUs, cpuGroupSize, cpuSortingStrategy)
 	}
 
-	if p.options.PreferAlignByUncoreCacheOption {
-
-		return takeByTopologyUncoreCachePacked(p.topology, availableCPUs, numCPUs, cpuSortingStrategy)
-
-	}
-	return takeByTopologyNUMAPacked(p.topology, availableCPUs, numCPUs, cpuSortingStrategy)
+	return takeByTopologyNUMAPacked(p.topology, availableCPUs, numCPUs, cpuSortingStrategy, p.options.PreferAlignByUncoreCacheOption)
 }
 
 func (p *staticPolicy) GetTopologyHints(s state.State, pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint {