Support `process.scheduler` by utam0k · Pull Request #3962 · opencontainers/runc

utam0k · 2023-08-04T02:15:11Z

Spec: opencontainers/runtime-spec#1188
Fix: #3895

Spec: opencontainers/runtime-spec#1188 Fix: opencontainers#3895 Signed-off-by: utam0k <k0ma@utam0k.jp>

AkihiroSuda · 2023-08-04T04:51:23Z

libcontainer/utils/utils_unix.go

+		SchedDeadline: scheduler.Deadline,
+		SchedPeriod:   scheduler.Period,
+	}
+	_, _, errno := syscall.Syscall(C.SYS_sched_setattr, uintptr(pid), uintptr(unsafe.Pointer(attr)), uintptr(0))


Suggested change

_, _, errno := syscall.Syscall(C.SYS_sched_setattr, uintptr(pid), uintptr(unsafe.Pointer(attr)), uintptr(0))

_, _, errno := syscall.Syscall(unix.SYS_SCHED_SETATTR, uintptr(pid), uintptr(unsafe.Pointer(attr)), uintptr(0))

AkihiroSuda · 2023-08-04T04:52:50Z

tests/integration/scheduler.bats

+	[ "$status" -eq 0 ]
+	[[ "${lines[0]}" == *"scheduling policy: SCHED_DEADLINE"* ]]
+	[[ "${lines[1]}" == *"priority: 0"* ]]
+	[[ "${lines[2]}" == *"runtime/deadline/period parameters: 42000/1000000/1000000"* ]]


* should be avoided for comparing numbers

Suggested change

[[ "${lines[2]}" == *"runtime/deadline/period parameters: 42000/1000000/1000000"* ]]

[[ "${lines[2]}" == "runtime/deadline/period parameters: 42000/1000000/1000000" ]]

cyphar · 2023-08-04T14:34:18Z

libcontainer/utils/utils_unix.go

 	return os.NewFile(uintptr(fds[1]), name+"-p"), os.NewFile(uintptr(fds[0]), name+"-c"), nil
 }
+
+type schedAttr struct {


I would want to be very sure that the padding is handled here exactly the same as with C. Can we make this a CGo structure? Or does Go have padding guarantees that match C?

cyphar · 2023-08-04T14:35:00Z

libcontainer/utils/utils_unix.go

+// SetSchedAttr sets the scheduler attributes for the process with the given pid.
+// Please refer to the following link for kernel-specific values:
+// https://github.com/torvalds/linux/blob/c1a515d3c0270628df8ae5f5118ba859b85464a2/include/uapi/linux/sched.h#L111-L134
+func SetSchedAttr(pid int, scheduler *configs.Scheduler) error {


Is there a reason golang.org/x/sys/unix doesn't have a wrapper for this? This syscall is fairly old -- first added in Linux 3.14.

My best guess is "no one bothered to". Let me give it a try.

Here's my attempt https://go-review.googlesource.com/c/sys/+/516756

(my best guess is "because of this ugly linux/glibc header conflict" now)

@utam0k the above PR is now merged; can you please vendor latest (@Head) x/sys/unix and use unix.SetSchedAttr?

Alas, this PR will have to be marked as draft until x/sys/unix makes a release.

#4006 comes with SchedSetAttr

lifubang · 2023-08-05T07:15:51Z

libcontainer/utils/utils_unix.go

+	}
+	_, _, errno := syscall.Syscall(C.SYS_sched_setattr, uintptr(pid), uintptr(unsafe.Pointer(attr)), uintptr(0))
+	if errno != 0 {
+		return errno


I think we should wrap an new error to indicate what's the error's exact means.
There is a case, if cpus in config.json doesn't include all CPUs in the system, sched_setattr(2) will fail with EPERM, but the error msg of runc is:

ERRO[0000] runc run failed: unable to start container process: error during container init: error setting scheduler: operation not permitted

Most of us may not know what happened.

Please refer: https://man7.org/linux/man-pages/man2/sched_setattr.2.html,

lifubang · 2023-08-05T07:23:27Z

libcontainer/configs/validate/validator.go

 	return (st1.Dev == st2.Dev) && (st1.Ino == st2.Ino), nil
 }
+
+func scheduler(config *configs.Config) error {


Shall we need to check the conflict with CPUs?

EPERM The CPU affinity mask of the thread specified by pid does not include all CPUs in the system

lifubang · 2023-08-11T01:03:13Z

ping @utam0k Could you please rebase and resolve the conflicting files?

kolyshkin · 2023-08-11T01:33:54Z

libcontainer/configs/config.go

+// Scheduler is based on the Linux sched_setattr(2) syscall.
+type Scheduler struct {
+	Policy   specs.LinuxSchedulerPolicy `json:"policy"`
+	Nice     int32                      `json:"nice,omitempty"`
+	Priority int32                      `json:"priority,omitempty"`
+	Flags    []specs.LinuxSchedulerFlag `json:"flags,omitempty"`
+	Runtime  uint64                     `json:"runtime,omitempty"`
+	Deadline uint64                     `json:"deadline,omitempty"`
+	Period   uint64                     `json:"period,omitempty"`


So, can we maybe do type Scheduler = specs.Scheduler here?

kolyshkin · 2023-08-11T01:36:58Z

libcontainer/configs/config.go

 	NoMountFallback bool `json:"no_mount_fallback,omitempty"`
+
+	// Scheduler represents the scheduling attributes for a process.
+	Scheduler *Scheduler `json:"shceduler,omitempty"`


typo in json attribute name

kolyshkin · 2023-08-11T19:31:33Z

libcontainer/specconv/spec_linux.go

+		if spec.Process.Scheduler != nil {
+			config.Scheduler = &configs.Scheduler{
+				Policy:   spec.Process.Scheduler.Policy,
+				Nice:     spec.Process.Scheduler.Nice,
+				Priority: spec.Process.Scheduler.Priority,
+				Flags:    spec.Process.Scheduler.Flags,
+				Runtime:  spec.Process.Scheduler.Runtime,
+				Deadline: spec.Process.Scheduler.Deadline,
+				Period:   spec.Process.Scheduler.Period,
+			}
+		}


Since spec.Process.Scheduler is of the same type as config.Scheduler, you can just assign it directly, no need to go field-by-field.

lifubang · 2023-09-24T12:42:20Z

Needs rebase.
@utam0k Do you still work on this PR? If not, I can handle this one to push it forward.

utam0k · 2023-09-24T12:46:06Z

@lifubang I have been away from this PR. If you are interested, please do 🙏

lifubang · 2023-09-25T10:58:48Z

@lifubang I have been away from this PR. If you are interested, please do 🙏

#4025

utam0k · 2023-09-25T11:06:31Z

#4025

utam0k force-pushed the scheduler branch from 0f88b67 to 8eba1cc Compare August 4, 2023 02:18

Support process.scheduler

83c45c6

Spec: opencontainers/runtime-spec#1188 Fix: opencontainers#3895 Signed-off-by: utam0k <k0ma@utam0k.jp>

utam0k force-pushed the scheduler branch from 8eba1cc to 83c45c6 Compare August 4, 2023 02:33

AkihiroSuda reviewed Aug 4, 2023

View reviewed changes

cyphar reviewed Aug 4, 2023

View reviewed changes

lifubang requested changes Aug 5, 2023

View reviewed changes

lifubang reviewed Aug 5, 2023

View reviewed changes

kolyshkin reviewed Aug 11, 2023

View reviewed changes

kolyshkin mentioned this pull request Sep 4, 2023

build(deps): bump golang.org/x/sys from 0.11.0 to 0.12.0 #4006

Merged

lifubang mentioned this pull request Sep 25, 2023

[Carry 3962] Support process.scheduler #4025

Merged

utam0k closed this Sep 25, 2023

	_, _, errno := syscall.Syscall(C.SYS_sched_setattr, uintptr(pid), uintptr(unsafe.Pointer(attr)), uintptr(0))
	_, _, errno := syscall.Syscall(unix.SYS_SCHED_SETATTR, uintptr(pid), uintptr(unsafe.Pointer(attr)), uintptr(0))

	[[ "${lines[2]}" == "runtime/deadline/period parameters: 42000/1000000/1000000" ]]
	[[ "${lines[2]}" == "runtime/deadline/period parameters: 42000/1000000/1000000" ]]

Conversation

utam0k commented Aug 4, 2023

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!

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!

lifubang commented Aug 11, 2023

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!

lifubang commented Sep 24, 2023

Uh oh!

utam0k commented Sep 24, 2023

Uh oh!

lifubang commented Sep 25, 2023

Uh oh!

utam0k commented Sep 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants