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/fs2: set per-device io weight if available #3022

Merged
merged 3 commits into from
Jun 17, 2021

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jun 11, 2021

  1. A bugfix to be able to set per-device weights on cgroup v1 with BFQ.
  2. Set per-device weights for v2 with BFQ.
  3. A test case for the above two.

I accidentally found out that per-device weight is supported
in BFQ scheduler since kernel v5.4 (kernel commit 795fe54c2a8),
so let's set those if supplied.

Next, I added a test case for it and decided to not make is v2-specific,
as the test for cgroup v1 would look very much the same. This test found a bug
in v1 (see #3010 (comment)), so I fixed it as well.

This closes a small gap in cgroup v2 feature parity with cgroup v1.

Proposed changelog entry

  • cgroup v2: set per-device io weights if BFQ IO scheduler is available.

@kolyshkin kolyshkin changed the title libct/cg/fs2: set io weight if possible libct/cg/fs2: set per-device io weight if available Jun 11, 2021
@kolyshkin kolyshkin force-pushed the fs2-weight-device branch from 8271ed2 to 1222ce7 Compare June 11, 2021 02:40
@kolyshkin
Copy link
Contributor Author

@askervin PTAL

@kolyshkin kolyshkin force-pushed the fs2-weight-device branch from 1222ce7 to 462301a Compare June 11, 2021 19:48
@kolyshkin kolyshkin added this to the 1.1.0 milestone Jun 11, 2021
@kolyshkin
Copy link
Contributor Author

This is a feature, not a bug (as I see it), so adding to 1.1 milestone

cyphar
cyphar previously requested changes Jun 13, 2021
libcontainer/cgroups/fs2/io.go Outdated Show resolved Hide resolved
@kolyshkin kolyshkin force-pushed the fs2-weight-device branch 3 times, most recently from 8a2746e to dfdab37 Compare June 14, 2021 01:08
@kolyshkin kolyshkin modified the milestones: 1.1.0, 1.0.0 Jun 14, 2021
@kolyshkin
Copy link
Contributor Author

The test case for cgroup v2 reveal an issue with v1 blkio, so I am adding a bug fix for it, and changing the milestone to 1.0.0 (as it is now a bug fix).

I can split this PR into two -- a bug fix (for cgroup v1) and a new feature (for cgroup v2), but as the test case is the same I'd rather keep this together.

@kolyshkin kolyshkin marked this pull request as draft June 14, 2021 01:14
@kolyshkin

This comment has been minimized.

@kolyshkin kolyshkin force-pushed the fs2-weight-device branch 2 times, most recently from 487f620 to c3bec6e Compare June 14, 2021 05:59
@kolyshkin kolyshkin force-pushed the fs2-weight-device branch 2 times, most recently from d186a46 to 6a7517a Compare June 14, 2021 06:19
@kolyshkin kolyshkin marked this pull request as ready for review June 14, 2021 06:26
@kolyshkin kolyshkin requested review from cyphar and AkihiroSuda June 14, 2021 06:26
For per-device weight, you can set weight and/or leaf weight.
The problem is, with the recent fix to use BFQ on cgroup v1,
if per-device weights are set, the code tries to set device
weight to blkio.bfq.weight, and the leaf weight to
blkio.leaf_weight_device. The latter file does not exist on
kernels v5.0, meaning one can not set any per-device weights
at all.

The fix is to only set weights if they are non-zero (i.e. set).

The test case will come in a following commit.

Fixes: 6339d8a
Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin force-pushed the fs2-weight-device branch from 09607a8 to 81530bc Compare June 14, 2021 19:53
@kolyshkin
Copy link
Contributor Author

Rebased on top of merged #3024.

Copy link
Contributor

@askervin askervin left a comment

Choose a reason for hiding this comment

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

@kolyshkin , great test & finding that leaf_device_weight was being written in cgroups v1 every time when setting device weights!

I did some manual testing with this cgroups v2 code, just to make sure that sequences like

  • open-write-seek-read-write
  • open-seek-read-write
    on io.bfq.weight indeed do what we expect. And it does.

This looks good and works fine. Only a super-nit: a typo in a comment.

libcontainer/cgroups/fs2/io.go Outdated Show resolved Hide resolved
@kolyshkin kolyshkin force-pushed the fs2-weight-device branch from 81530bc to 0ac70e4 Compare June 15, 2021 09:09
cyphar
cyphar previously approved these changes Jun 15, 2021
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM.

askervin
askervin previously approved these changes Jun 15, 2021
Copy link
Contributor

@askervin askervin left a comment

Choose a reason for hiding this comment

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

LGTM. Special thanks for fixing the issue in PR #3010! :)

return false
}
_, _ = bfq.Seek(0, 0)
exp := []byte("default ")
Copy link
Member

Choose a reason for hiding this comment

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

Is this really guaranteed to be in the first line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And before torvalds/linux@795fe54 it was a number in there.

Copy link
Member

Choose a reason for hiding this comment

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

Future version of the kernel may have another line before "default 100".
So I guess we have to scan all the lines?

Copy link
Member

Choose a reason for hiding this comment

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

In theory that would be an ABI breakage, but yeah it seems a bit risky to depend on the exact order of lines in a kernel file when the alternative is pretty inexpensive. (And in the past cgroup ABIs haven't exactly been completely free of ABI breakages.)

Copy link
Contributor Author

@kolyshkin kolyshkin Jun 16, 2021

Choose a reason for hiding this comment

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

I disagree (that the relaxed check is to look for a line starting with default anywhere in the file).

So far we have to distinguish between two versions of the bfq.io.weight API. Let's call them version 1 and version 2. Version 1 only allows to get/set the default weight. Version 2 also allows to get/set per-device weights.

Now, if the first line of bfq.io.weight does read back as default XXX, we do have version 2. Otherwise it's something else. Either version 1 which does not support setting per-device weights, or perhaps a version 3 we don't know anything about.

But let's assume that write format for version 3 is backward-compatible with version 2 (same as version 2 write is backward-compatible with version 1, although the read is not). In this case we need to check not for version 2, but for anything other than version 1.

Such a check would be to read the entire file and see that it's not a version 1 file, i.e. not a single number followed by a newline.

I will update the commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. The new check specifically checks for v1 file, not v2 file, and is thus more future-proof (assuming that in v3 file format will expand and not go back to same as it was in v1).

@kolyshkin kolyshkin requested a review from AkihiroSuda June 15, 2021 19:17
@kolyshkin kolyshkin dismissed stale reviews from askervin and cyphar via 2916817 June 16, 2021 11:46
@kolyshkin kolyshkin force-pushed the fs2-weight-device branch from 0ac70e4 to 2916817 Compare June 16, 2021 11:46
Per-device weight is supported since kernel v5.4 (kernel commit
795fe54c2a8), so let's set those if supplied.

[v2: implement a more relaxed check in bfqDeviceWeightSupported]

Signed-off-by: Kir Kolyshkin <[email protected]>
This works for both cgroup v1 and v2.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin requested a review from cyphar June 16, 2021 18:59
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM. With this I'll prep a release.

@cyphar cyphar closed this in 47d37b3 Jun 17, 2021
@cyphar cyphar merged commit 47d37b3 into opencontainers:master Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants