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

libct/cg: write unified resources line by line #4186

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 34 additions & 0 deletions libcontainer/cgroups/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,40 @@ func WriteFile(dir, file, data string) error {
return nil
}

// WriteFileByLine is the same as WriteFile, except if data contains newlines,
// is is written line by line.
func WriteFileByLine(dir, file, data string) error {
i := strings.Index(data, "\n")
if i == -1 {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this special block.
Others LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lifubang theoretically, yes. Practically, I added it here because I was overly cautious to not break anything.

With this block in place, if the input does not contain newlines, we use the very same function as we used before. In other words, unless there's a newline in data, this commit changes nothing (and it is easy to validate).

It is also easier to read this way (when you debug it, and write a single line, which is the most common scenario, you don't have to read through the code below).

return WriteFile(dir, file, data)
}

fd, err := OpenFile(dir, file, unix.O_WRONLY)
if err != nil {
return err
}
defer fd.Close()
start := 0
for {
var line string
if i == -1 {
line = data[start:]
} else {
line = data[start : start+i+1]
}
_, err := fd.WriteString(line)
if err != nil {
return fmt.Errorf("failed to write %q: %w", line, err)
}
if i == -1 {
break
}
start += i + 1
i = strings.Index(data[start:], "\n")
}
return nil
}

const (
cgroupfsDir = "/sys/fs/cgroup"
cgroupfsPrefix = cgroupfsDir + "/"
Expand Down
22 changes: 22 additions & 0 deletions libcontainer/cgroups/file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,25 @@ func TestOpenat2(t *testing.T) {
fd.Close()
}
}

func BenchmarkWriteFile(b *testing.B) {
TestMode = true
defer func() { TestMode = false }()

dir := b.TempDir()
tc := []string{
"one",
"one\ntwo\nthree",
"10:200 foo=bar boo=far\n300:1200 something=other\ndefault 45000\n",
"\n\n\n\n\n\n\n\n",
}

b.ResetTimer()
for i := 0; i < b.N; i++ {
for _, val := range tc {
if err := WriteFileByLine(dir, "file", val); err != nil {
b.Fatal(err)
}
}
}
}
2 changes: 1 addition & 1 deletion libcontainer/cgroups/fs2/fs2.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ func (m *Manager) setUnified(res map[string]string) error {
if strings.Contains(k, "/") {
return fmt.Errorf("unified resource %q must be a file name (no slashes)", k)
}
if err := cgroups.WriteFile(m.dirPath, k, v); err != nil {
if err := cgroups.WriteFileByLine(m.dirPath, k, v); err != nil {
// Check for both EPERM and ENOENT since O_CREAT is used by WriteFile.
if errors.Is(err, os.ErrPermission) || errors.Is(err, os.ErrNotExist) {
// Check if a controller is available,
Expand Down
32 changes: 32 additions & 0 deletions tests/integration/cgroups.bats
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,38 @@ function setup() {
[[ "$weights" == *"$major:$minor 444"* ]]
}

@test "runc run (per-device multiple iops via unified)" {
requires root cgroups_v2

dd if=/dev/zero of=backing1.img bs=4096 count=1
dev1=$(losetup --find --show backing1.img) || skip "unable to create a loop device"

# Second device.
dd if=/dev/zero of=backing2.img bs=4096 count=1
dev2=$(losetup --find --show backing2.img) || skip "unable to create a loop device"

set_cgroups_path

IFS=$' \t:' read -r major1 minor1 <<<"$(lsblk -nd -o MAJ:MIN "$dev1")"
IFS=$' \t:' read -r major2 minor2 <<<"$(lsblk -nd -o MAJ:MIN "$dev2")"
update_config ' .linux.devices += [
{path: "'"$dev1"'", type: "b", major: '"$major1"', minor: '"$minor1"'},
{path: "'"$dev2"'", type: "b", major: '"$major2"', minor: '"$minor2"'}
]
| .linux.resources.unified |=
{"io.max": "'"$major1"':'"$minor1"' riops=333 wiops=444\n'"$major2"':'"$minor2"' riops=555 wiops=666\n"}'
runc run -d --console-socket "$CONSOLE_SOCKET" test_dev_weight
[ "$status" -eq 0 ]

# The loop devices are no longer needed.
losetup -d "$dev1"
losetup -d "$dev2"

weights=$(get_cgroup_value "io.max")
grep "^$major1:$minor1 .* riops=333 wiops=444$" <<<"$weights"
grep "^$major2:$minor2 .* riops=555 wiops=666$" <<<"$weights"
}

@test "runc run (cpu.idle)" {
requires cgroups_cpu_idle
[ $EUID -ne 0 ] && requires rootless_cgroup
Expand Down