fix: cpu affinity #4815

ningmingxiao · 2025-07-16T07:42:24Z

Old kernels do that automatically, but new kernels remember the affinity that was set before the cgroup move due to

https://lore.kernel.org/lkml/[email protected]

This is undesirable for containers, because they inherit the systemd affinity when the should really move to the container space cpus.

ref:#4041
this patch https://lore.kernel.org/lkml/[email protected]/ doesn't merge in kernel but merged in redhat I want to find another way to fix it.

AkihiroSuda · 2025-07-16T08:56:08Z

What was the issue?
How to test this PR?

libcontainer/process_linux.go

rata

@AkihiroSuda +1, it would be great if we can have some test for this.

haircommander · 2025-07-16T17:15:35Z

hmm I wonder if we can gather the cpuset from the container spec rather than inheriting what runc is being called with...

ningmingxiao · 2025-07-17T02:03:22Z

I try to get it form cpuset but default is empty.

ningmingxiao · 2025-07-17T02:15:32Z

I add an if,it only happen on linux.

if runtime.GOOS == "linux" {
}

libcontainer/process_linux.go

libcontainer/utils/utils.go

rata

Thanks for the PR! I left several comments, but I guess we want also tests in tests/integration :)

libcontainer/utils/utils.go

libcontainer/utils/utils_test.go

rata · 2025-07-17T09:45:02Z

libcontainer/utils/utils.go

+			break
+		}
+		line := string(data)
+		if line[:3] != "cpu" {


I think the comment that all cpu* lines are at the beginning belongs here.

Why did you mark this as solved? Am I missing something?

rata · 2025-07-17T09:45:51Z

libcontainer/utils/utils.go

+		if line[:3] != "cpu" {
+			break
+		}
+		if '0' <= line[3] && line[3] <= '9' {


Here we need a commet explaining that there is a "cpu" line that we should ignore and only count cpu lines followed by a number.

rata · 2025-07-17T09:48:29Z

libcontainer/process_linux.go

+	if err := setAffinityAll(p.pid()); err != nil {
+		return err
+	}
 	// Set final CPU affinity right after the process is moved into container's cgroup.
 	if err := p.setFinalCPUAffinity(); err != nil {


Sorry, I ignore this, but why do we need to set the affinity to all cpus and then to what is in the config? I don't follow from the link in the description why this step is needed.

Why was this marked as solved? Did I miss something?

libcontainer/process_linux.go

rata · 2025-07-17T15:09:28Z

@ningmingxiao ping when this is ready for another round of reviews (and the tests are green, hopefully :))

AkihiroSuda · 2025-07-18T03:17:48Z

libcontainer/process_linux.go

+	cpuset := unix.CPUSet{}
+	for i := 0; i < int(cpus); i++ {
+		cpuset.Set(i)
+	}


Was this tested with nested containers?
cpus here can be different from the number of the available cpus

AkihiroSuda · 2025-07-18T03:18:08Z

.github/workflows/test.yml

@@ -168,6 +168,7 @@ jobs:

    - name: integration test (systemd driver)
      run: |
+        sudo taskset -pc 0-1 1


Should be moved to the bats script

rata · 2025-07-21T08:59:27Z

@ningmingxiao if you can ping here when it's ready for review it would be great. Marking conversations as solved in github doesn't send any notification. And it's hard to know when it's ready for review again if you don't say anything or request another review in github.

ningmingxiao · 2025-07-21T09:11:47Z

ok ！ it can be reviewed now, thanks @rata @AkihiroSuda

Signed-off-by: ningmingxiao <[email protected]>

rata

Thanks, left a few more comments. But there are several open comments already, please have a look :)

rata · 2025-07-21T14:19:34Z

libcontainer/process_linux.go

-	aff := p.config.CPUAffinity
-	if aff == nil || aff.Final == nil {
-		return nil
-	}
-	if err := unix.SchedSetaffinity(p.pid(), aff.Final); err != nil {
+	if err := unix.SchedSetaffinity(p.pid(), p.config.CPUAffinity.Final); err != nil {


Why is the nil check not needed anymore?

rata · 2025-07-21T14:20:54Z

libcontainer/process_linux.go

-	// Set final CPU affinity right after the process is moved into container's cgroup.
-	if err := p.setFinalCPUAffinity(); err != nil {
-		return err
+
+	if aff := p.config.CPUAffinity; aff == nil || aff.Final == nil {
+		if err := setAffinityAll(p.pid()); err != nil {
+			return err
+		}
+	} else {
+		// Set final CPU affinity right after the process is moved into container's cgroup.
+		if err := p.setFinalCPUAffinity(); err != nil {
+			return err
+		}


Sorry, it's not obvious to me why if the cpu affinity fails, we should call the set finalCPUAffinity. Can you elaborate?

rata · 2025-07-21T14:21:11Z

libcontainer/process_linux.go

@@ -615,6 +618,11 @@ func (p *initProcess) start() (retErr error) {
 			return fmt.Errorf("unable to apply cgroup configuration: %w", err)
 		}
 	}
+
+	if err := setAffinityAll(p.pid()); err != nil {


And why this here again?

AkihiroSuda reviewed Jul 16, 2025

View reviewed changes

libcontainer/process_linux.go Outdated Show resolved Hide resolved

rata reviewed Jul 16, 2025

View reviewed changes

ningmingxiao force-pushed the fix_annify branch from 23fe833 to 880afcb Compare July 17, 2025 02:07

ningmingxiao force-pushed the fix_annify branch from 880afcb to 308ed19 Compare July 17, 2025 02:30

ningmingxiao changed the title ~~fix:cpu cpu affinity~~ fix: cpu affinity Jul 17, 2025

ningmingxiao force-pushed the fix_annify branch from 308ed19 to 3cc2c4b Compare July 17, 2025 02:59

AkihiroSuda reviewed Jul 17, 2025

View reviewed changes

libcontainer/process_linux.go Outdated Show resolved Hide resolved

ningmingxiao force-pushed the fix_annify branch 2 times, most recently from cab02c9 to d72f7b9 Compare July 17, 2025 04:41

AkihiroSuda reviewed Jul 17, 2025

View reviewed changes

libcontainer/utils/utils.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jul 17, 2025

View reviewed changes

libcontainer/utils/utils.go Show resolved Hide resolved

ningmingxiao force-pushed the fix_annify branch from d72f7b9 to 284a680 Compare July 17, 2025 07:56

rata reviewed Jul 17, 2025

View reviewed changes

ningmingxiao force-pushed the fix_annify branch 2 times, most recently from ec7d0c0 to 2904639 Compare July 17, 2025 13:04

ningmingxiao force-pushed the fix_annify branch 3 times, most recently from be17d50 to 81e87d8 Compare July 18, 2025 02:46

AkihiroSuda reviewed Jul 18, 2025

View reviewed changes

ningmingxiao force-pushed the fix_annify branch from 85bf7eb to d16cdeb Compare July 19, 2025 07:00

ningmingxiao mentioned this pull request Jul 21, 2025

K8s pods (non guaranteed QoS) are CPU confined to systemd's CPUAffinity when containerd runs as a systemd service containerd/containerd#11345

Closed

fix:cpu affinity

2460950

Signed-off-by: ningmingxiao <[email protected]>

ningmingxiao force-pushed the fix_annify branch from d16cdeb to 2460950 Compare July 21, 2025 09:35

rata reviewed Jul 21, 2025

View reviewed changes

fix: cpu affinity #4815

Are you sure you want to change the base?

fix: cpu affinity #4815

Uh oh!

Conversation

ningmingxiao commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Jul 16, 2025

Uh oh!

Uh oh!

rata left a comment

Choose a reason for hiding this comment

Uh oh!

haircommander commented Jul 16, 2025

Uh oh!

ningmingxiao commented Jul 17, 2025

Uh oh!

ningmingxiao commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rata commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rata commented Jul 21, 2025

Uh oh!

ningmingxiao commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rata left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ningmingxiao commented Jul 16, 2025 •

edited

Loading

ningmingxiao commented Jul 17, 2025 •

edited

Loading

ningmingxiao commented Jul 21, 2025 •

edited

Loading