From 2dc3ea4b87083b6c4b3737e8e8f9506e9be94853 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 26 Jun 2024 16:14:44 -0700 Subject: [PATCH 1/5] libct: simplify setIOPriority/setupScheduler calls Move the nil check inside, simplifying the callers. Fixes: bfbd0305 ("Add I/O priority") Fixes: 770728e1 ("Support `process.scheduler`") Signed-off-by: Kir Kolyshkin --- libcontainer/init_linux.go | 3 +++ libcontainer/process_linux.go | 9 +++++---- libcontainer/setns_init_linux.go | 6 ++---- libcontainer/standard_init_linux.go | 13 +++++-------- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index d23153e9b3d..cc897703a68 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -662,6 +662,9 @@ func setupRlimits(limits []configs.Rlimit, pid int) error { } func setupScheduler(config *configs.Config) error { + if config.Scheduler == nil { + return nil + } attr, err := configs.ToSchedAttr(config.Scheduler) if err != nil { return err diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index dde6aecf13a..7e17fe5117a 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -166,10 +166,8 @@ type setnsProcess struct { func (p *setnsProcess) start() (retErr error) { defer p.comm.closeParent() - if p.process.IOPriority != nil { - if err := setIOPriority(p.process.IOPriority); err != nil { - return err - } + if err := setIOPriority(p.process.IOPriority); err != nil { + return err } // get the "before" value of oom kill count @@ -912,6 +910,9 @@ func (p *Process) InitializeIO(rootuid, rootgid int) (i *IO, err error) { func setIOPriority(ioprio *configs.IOPriority) error { const ioprioWhoPgrp = 1 + if ioprio == nil { + return nil + } class, ok := configs.IOPrioClassMapping[ioprio.Class] if !ok { return fmt.Errorf("invalid io priority class: %s", ioprio.Class) diff --git a/libcontainer/setns_init_linux.go b/libcontainer/setns_init_linux.go index 92c6ef77030..db4aa1463a6 100644 --- a/libcontainer/setns_init_linux.go +++ b/libcontainer/setns_init_linux.go @@ -71,10 +71,8 @@ func (l *linuxSetnsInit) Init() error { unix.Umask(int(*l.config.Config.Umask)) } - if l.config.Config.Scheduler != nil { - if err := setupScheduler(l.config.Config); err != nil { - return err - } + if err := setupScheduler(l.config.Config); err != nil { + return err } // Tell our parent that we're ready to exec. This must be done before the diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go index 9f7fa45d533..fe6af5704d7 100644 --- a/libcontainer/standard_init_linux.go +++ b/libcontainer/standard_init_linux.go @@ -155,15 +155,12 @@ func (l *linuxStandardInit) Init() error { } } - if l.config.Config.Scheduler != nil { - if err := setupScheduler(l.config.Config); err != nil { - return err - } + if err := setupScheduler(l.config.Config); err != nil { + return err } - if l.config.Config.IOPriority != nil { - if err := setIOPriority(l.config.Config.IOPriority); err != nil { - return err - } + + if err := setIOPriority(l.config.Config.IOPriority); err != nil { + return err } // Tell our parent that we're ready to exec. This must be done before the From ec465d39f5b97ffb3b52539659f536fdf09027be Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 26 Jun 2024 16:17:36 -0700 Subject: [PATCH 2/5] utils: simplify newProcess This code is not in libcontainer, meaning it is only used by a short lived binary (runc start/run/exec). Unlike code in libcontainer (see CreateLibcontainerConfig), here we don't have to care about copying the structures supplied as input, meaning we can just reuse the pointers directly. Fixes: bfbd0305 ("Add I/O priority") Fixes: 770728e1 ("Support `process.scheduler`") Signed-off-by: Kir Kolyshkin --- utils_linux.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/utils_linux.go b/utils_linux.go index feb6ef80c4a..eef78ea3845 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -55,6 +55,8 @@ func newProcess(p specs.Process) (*libcontainer.Process, error) { Label: p.SelinuxLabel, NoNewPrivileges: &p.NoNewPrivileges, AppArmorProfile: p.ApparmorProfile, + Scheduler: p.Scheduler, + IOPriority: p.IOPriority, } if p.ConsoleSize != nil { @@ -62,16 +64,6 @@ func newProcess(p specs.Process) (*libcontainer.Process, error) { lp.ConsoleHeight = uint16(p.ConsoleSize.Height) } - if p.Scheduler != nil { - s := *p.Scheduler - lp.Scheduler = &s - } - - if p.IOPriority != nil { - ioPriority := *p.IOPriority - lp.IOPriority = &ioPriority - } - if p.Capabilities != nil { lp.Capabilities = &configs.Capabilities{} lp.Capabilities.Bounding = p.Capabilities.Bounding From 5d3942eec37a2608d20711ad2abb6065226a360f Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 26 Jun 2024 17:22:02 -0700 Subject: [PATCH 3/5] libct: unify IOPriority setting For some reason, io priority is set in different places between runc start/run and runc exec: - for runc start/run, it is done in the middle of (*linuxStandardInit).Init, close to the place where we exec runc init. - for runc exec, it is done much earlier, in (*setnsProcess) start(). Let's move setIOPriority call for runc exec to (*linuxSetnsInit).Init, so it is in the same logical place as for runc start/run. Also, move the function itself to init_linux.go as it's part of init. Should not have any visible effect, except part of runc init is run with a different I/O priority. While at it, rename setIOPriority to setupIOPriority, and make it accept the whole *configs.Config, for uniformity with other similar functions. Fixes: bfbd0305 ("Add I/O priority") Signed-off-by: Kir Kolyshkin --- libcontainer/init_linux.go | 22 ++++++++++++++++++++++ libcontainer/process_linux.go | 25 ------------------------- libcontainer/setns_init_linux.go | 3 +++ libcontainer/standard_init_linux.go | 2 +- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index cc897703a68..086f3125863 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -678,6 +678,28 @@ func setupScheduler(config *configs.Config) error { return nil } +func setupIOPriority(config *configs.Config) error { + const ioprioWhoPgrp = 1 + + ioprio := config.IOPriority + if ioprio == nil { + return nil + } + class, ok := configs.IOPrioClassMapping[ioprio.Class] + if !ok { + return fmt.Errorf("invalid io priority class: %s", ioprio.Class) + } + + // Combine class and priority into a single value + // https://github.com/torvalds/linux/blob/v5.18/include/uapi/linux/ioprio.h#L5-L17 + iop := (class << 13) | ioprio.Priority + _, _, errno := unix.RawSyscall(unix.SYS_IOPRIO_SET, ioprioWhoPgrp, 0, uintptr(iop)) + if errno != 0 { + return fmt.Errorf("failed to set io priority: %w", errno) + } + return nil +} + func setupPersonality(config *configs.Config) error { return system.SetLinuxPersonality(config.Personality.Domain) } diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index 7e17fe5117a..9b20f2d6695 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -166,10 +166,6 @@ type setnsProcess struct { func (p *setnsProcess) start() (retErr error) { defer p.comm.closeParent() - if err := setIOPriority(p.process.IOPriority); err != nil { - return err - } - // get the "before" value of oom kill count oom, _ := p.manager.OOMKillCount() err := p.cmd.Start() @@ -906,24 +902,3 @@ func (p *Process) InitializeIO(rootuid, rootgid int) (i *IO, err error) { } return i, nil } - -func setIOPriority(ioprio *configs.IOPriority) error { - const ioprioWhoPgrp = 1 - - if ioprio == nil { - return nil - } - class, ok := configs.IOPrioClassMapping[ioprio.Class] - if !ok { - return fmt.Errorf("invalid io priority class: %s", ioprio.Class) - } - - // Combine class and priority into a single value - // https://github.com/torvalds/linux/blob/v5.18/include/uapi/linux/ioprio.h#L5-L17 - iop := (class << 13) | ioprio.Priority - _, _, errno := unix.RawSyscall(unix.SYS_IOPRIO_SET, ioprioWhoPgrp, 0, uintptr(iop)) - if errno != 0 { - return fmt.Errorf("failed to set io priority: %w", errno) - } - return nil -} diff --git a/libcontainer/setns_init_linux.go b/libcontainer/setns_init_linux.go index db4aa1463a6..462662a84ed 100644 --- a/libcontainer/setns_init_linux.go +++ b/libcontainer/setns_init_linux.go @@ -75,6 +75,9 @@ func (l *linuxSetnsInit) Init() error { return err } + if err := setupIOPriority(l.config.Config); err != nil { + return err + } // Tell our parent that we're ready to exec. This must be done before the // Seccomp rules have been applied, because we need to be able to read and // write to a socket. diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go index fe6af5704d7..65444f38ae0 100644 --- a/libcontainer/standard_init_linux.go +++ b/libcontainer/standard_init_linux.go @@ -159,7 +159,7 @@ func (l *linuxStandardInit) Init() error { return err } - if err := setIOPriority(l.config.Config.IOPriority); err != nil { + if err := setupIOPriority(l.config.Config); err != nil { return err } From 7334ee01e606c1bb79bba1d62ee4d931cf495e9b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 21 Oct 2024 15:00:11 -0700 Subject: [PATCH 4/5] libct/configs: rm IOPrioClassMapping This is an internal implementation detail and should not be either public or visible. Amend setIOPriority to do own class conversion. Fixes: bfbd0305 ("Add I/O priority") Signed-off-by: Kir Kolyshkin --- libcontainer/configs/config.go | 6 ------ libcontainer/init_linux.go | 11 +++++++++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/libcontainer/configs/config.go b/libcontainer/configs/config.go index 22fe0f9b4c1..4436aaa0cfa 100644 --- a/libcontainer/configs/config.go +++ b/libcontainer/configs/config.go @@ -286,12 +286,6 @@ func ToSchedAttr(scheduler *Scheduler) (*unix.SchedAttr, error) { }, nil } -var IOPrioClassMapping = map[specs.IOPriorityClass]int{ - specs.IOPRIO_CLASS_RT: 1, - specs.IOPRIO_CLASS_BE: 2, - specs.IOPRIO_CLASS_IDLE: 3, -} - type IOPriority = specs.LinuxIOPriority type ( diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 086f3125863..b218a6cb126 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -685,8 +685,15 @@ func setupIOPriority(config *configs.Config) error { if ioprio == nil { return nil } - class, ok := configs.IOPrioClassMapping[ioprio.Class] - if !ok { + class := 0 + switch ioprio.Class { + case specs.IOPRIO_CLASS_RT: + class = 1 + case specs.IOPRIO_CLASS_BE: + class = 2 + case specs.IOPRIO_CLASS_IDLE: + class = 3 + default: return fmt.Errorf("invalid io priority class: %s", ioprio.Class) } From 57462491c1a3b02a513c667920995220c75eae25 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 21 Oct 2024 15:15:14 -0700 Subject: [PATCH 5/5] libct/configs/validate: add IOPriority.Class validation Signed-off-by: Kir Kolyshkin --- libcontainer/configs/validate/validator.go | 8 ++++++++ libcontainer/configs/validate/validator_test.go | 12 +++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/libcontainer/configs/validate/validator.go b/libcontainer/configs/validate/validator.go index 37ece0aebbd..7d17676ed18 100644 --- a/libcontainer/configs/validate/validator.go +++ b/libcontainer/configs/validate/validator.go @@ -406,5 +406,13 @@ func ioPriority(config *configs.Config) error { if priority < 0 || priority > 7 { return fmt.Errorf("invalid ioPriority.Priority: %d", priority) } + + switch class := config.IOPriority.Class; class { + case specs.IOPRIO_CLASS_RT, specs.IOPRIO_CLASS_BE, specs.IOPRIO_CLASS_IDLE: + // Valid class, do nothing. + default: + return fmt.Errorf("invalid ioPriority.Class: %q", class) + } + return nil } diff --git a/libcontainer/configs/validate/validator_test.go b/libcontainer/configs/validate/validator_test.go index b0b740a122d..d157feea5bc 100644 --- a/libcontainer/configs/validate/validator_test.go +++ b/libcontainer/configs/validate/validator_test.go @@ -847,15 +847,21 @@ func TestValidateIOPriority(t *testing.T) { testCases := []struct { isErr bool priority int + class specs.IOPriorityClass }{ - {isErr: false, priority: 0}, - {isErr: false, priority: 7}, - {isErr: true, priority: -1}, + {isErr: false, priority: 0, class: specs.IOPRIO_CLASS_IDLE}, + {isErr: false, priority: 7, class: specs.IOPRIO_CLASS_RT}, + {isErr: false, priority: 3, class: specs.IOPRIO_CLASS_BE}, + // Invalid priority. + {isErr: true, priority: -1, class: specs.IOPRIO_CLASS_BE}, + // Invalid class. + {isErr: true, priority: 3, class: specs.IOPriorityClass("IOPRIO_CLASS_WOW")}, } for _, tc := range testCases { ioPriroty := configs.IOPriority{ Priority: tc.priority, + Class: tc.class, } config := &configs.Config{ Rootfs: "/var",