Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixups to scheduler/priority settings #4459

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kolyshkin
Copy link
Contributor

These commits amend commit bfbd030 (#3783) and 770728e (#4025). Should not result in any visible change, except:

  • invalid IOPriority.Class value will result in an error much earlier;
  • runc exec changes IO priority later, just before exec;
  • configs.IOPrioClassMapping map is removed from the public API.

Should be relatively easy to review.

@kolyshkin kolyshkin marked this pull request as ready for review October 21, 2024 22:46
@kolyshkin kolyshkin added the kind/refactor refactoring label Oct 21, 2024
@kolyshkin kolyshkin force-pushed the prio-nits branch 3 times, most recently from 6b830aa to 0a65a4d Compare October 23, 2024 19:21
@kolyshkin kolyshkin force-pushed the prio-nits branch 2 times, most recently from d5be617 to fcb9301 Compare November 7, 2024 21:16
Copy link
Contributor Author

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@opencontainers/runc-maintainers @AkihiroSuda @cyphar @thaJeztah @rata @lifubang can we please review/merge this? This will unblock my further work (adding CPU affinity).

Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Left two unimportant comments.

}

if err := setIOPriority(l.config.Config.IOPriority); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about change the param type to *configs.Config like in setupScheduler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense (although in case of setupScheduler we pass the whole config because we also need to access config.Cgroups).

I've also changed its name, as other similar functions have setup not set prefix.

The change:

 
-func setIOPriority(ioprio *configs.IOPriority) error {
+func setupIOPriority(config *configs.Config) error {
        const ioprioWhoPgrp = 1
 
+       ioprio := config.IOPriority
        if ioprio == nil {
                return nil
        }


switch config.IOPriority.Class {
case specs.IOPRIO_CLASS_RT, specs.IOPRIO_CLASS_BE, specs.IOPRIO_CLASS_IDLE:
// do nothing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this comment, but this is my personal preference. Maybe should change to // should return nil or move to an new function:

func checkIOPriorityClass(specs.IOPriorityClass) bool {
    switch config.IOPriority.Class {
	case specs.IOPRIO_CLASS_RT, specs.IOPRIO_CLASS_BE, specs.IOPRIO_CLASS_IDLE:
            return true
    }
    return false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the comment to // Valid class, do nothing., and the whole switch to

          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)                                             
          }

Move the nil check inside, simplifying the callers.

Fixes: bfbd030 ("Add I/O priority")
Fixes: 770728e ("Support `process.scheduler`")
Signed-off-by: Kir Kolyshkin <[email protected]>
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: bfbd030 ("Add I/O priority")
Fixes: 770728e ("Support `process.scheduler`")
Signed-off-by: Kir Kolyshkin <[email protected]>
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: bfbd030 ("Add I/O priority")
Signed-off-by: Kir Kolyshkin <[email protected]>
This is an internal implementation detail and should not be either
public or visible.

Amend setIOPriority to do own class conversion.

Fixes: bfbd030 ("Add I/O priority")
Signed-off-by: Kir Kolyshkin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants