IODeviceWeight= configures bfq.io.weight too #21795

Werkov · 2021-12-16T17:05:13Z

This is a possible extension of the PR #21728. Opening separate PR since the per-device weights can't be configured when the particular device doesn't support (it's not configured) proportional control method.

keszybz

Looks reasonable.

src/core/cgroup.c

Werkov · 2022-01-10T17:40:28Z

Force pushed on top of changes.

I'm emphasizing a comment from the commit message:

Since per-device weight of a particular device cannot use both io.cost
policy and BFQ scheduler, at least one warning is always expected with
this approach (the method that is unconfigured).

yuwata

LGTM. One minor suggestion.

src/core/cgroup.c

yuwata · 2022-01-12T13:23:43Z

#21728 is merged. Please rebase.

src/test/test-cgroup-util.c

Werkov · 2022-01-12T17:47:27Z

Thanks for the review. Rebased and condition changed.

bluca · 2022-01-13T23:57:55Z

This patch extends to devices what is already done for default cgroup IO
weight in commit 2dbc45a ("cgroup: Also set io.bfq.weight"). This
is possible with kernels >= v5.4 with commit 795fe54c2a82 ("bfq: Add
per-device weight"). A warning is logged if the write fails on older kernels.

Is there any sensible and usable way to detect this? People tend to complain about warnings, especially if they are repeated. If there's an error, having something like static bool supported or such would make it already a bit nicer

yuwata · 2022-01-14T06:52:53Z

This patch extends to devices what is already done for default cgroup IO
weight in commit 2dbc45a ("cgroup: Also set io.bfq.weight"). This
is possible with kernels >= v5.4 with commit 795fe54c2a82 ("bfq: Add
per-device weight"). A warning is logged if the write fails on older kernels.

Is there any sensible and usable way to detect this? People tend to complain about warnings, especially if they are repeated. If there's an error, having something like static bool supported or such would make it already a bit nicer

What errno we get from old kernels?

Werkov · 2022-01-14T10:56:12Z

That should be good old -EINVAL. The warnings could be switched to debug level after a single warning with some limited reliability (similar to what is done with cgroup-compat logging).
(OTOH, this also means systems mixing kernel from 2019- with systemd from 2022+. So I don't know how much it's worth adding the compat code.)

bluca · 2022-01-14T11:06:58Z

if it's not too complicated I'd say it's worth it, 2019 is not that long ago

Werkov · 2022-01-14T17:07:34Z

I've added two more commits to the branch with a draft of possible kernel version aware logging. I admit I don't like it, so I'm ready drop the two commits again :-)
WDYT?

Werkov · 2022-01-14T17:15:09Z

And one more commit stacked to address @keszybz's testing concern.

bluca · 2022-01-14T18:08:56Z

src/core/cgroup.c

+#define LOG_LEVEL_CGROUP_WRITE(r, ...) (IN_SET(abs(r), ENOENT, EROFS, EACCES, EPERM, ##__VA_ARGS__) ? LOG_DEBUG : LOG_WARNING)
+
+static const char *cgroup_jump_versions[_CGROUP_JUMP_MAX] = {
+        "5.4", /* 795fe54c2a82 ("bfq: Add per-device weight"). */


We don't usually do version comparison as enterprise kernels backport all the stuff - I'd say a static bool supported = true/false would be enough? log once, then skip it forever

FTR, there's a precedent. I added the version check (not much excited about that neither) to properly handle EINVALs on the newer kernels.

OTOH (when checking the current kernel now), if we assume only the systemd well-formatted input, there should be possible only ENODEV, EOPNOTSUPP, ERANGE (avoided by scaling), rarely EBUSY or ENOMEM. I.e. EINVAL could be demoted to debug in any kernel.

I'll simplify the check then.

Sounds good, thanks

poettering · 2022-03-10T13:47:41Z

hmm, I really dislike the idea of that kernel version table. Please let's not do that. Why?

Quite frankly, we should have never merged that BFQ supports. It's such a mess, requiring myriads of work-arounds everywhere, because it always behaves differently than the regular scheduler. Please let's not add major infra for dealing with apparently incomplete stuff like this, in particular not kernel version checks stuff which so far we always fended off.

Werkov · 2022-03-10T14:10:34Z

Note that I share the sentiment towards kernel version checking and simpler solution should exist (not to pollute issue tracker, this should have likely been tagged as reviewed, needs rework before I get down to it).

There are two possible IO control mechanisms (io.cost and BFQ) that expose separate weights APIs. In systemd units we configure only a single device weight value so we need to write it to both attribute files. This patch extends to devices what is already done for default cgroup IO weight in commit 2dbc45a ("cgroup: Also set io.bfq.weight"). This is possible with kernels >= v5.4 with commit 795fe54c2a82 ("bfq: Add per-device weight"). A warning is logged if the write fails on older kernels. Since per-device weight of a particular device cannot use both io.cost policy and BFQ scheduler, at least one warning is always expected with this approach (the method that is unconfigured).

poettering · 2022-03-24T08:58:43Z

Anyway, the kernel version infra this adds is way over the top. find a different way, let's not add something complex like that.

I don't really grok the issue here though, i.e. there are two distinct attributes? why not write both, and only complain if both fail?

Also, why doesn't the kernel report the feature set in /sys/kernel/cgroup/features? That's what that file is for...

(As before: bfq appears a totally incomplete mess, I am tmpted to just remove all support for it. We shouldn't support something that pretty obviously is chaos and incomplete).

Werkov · 2022-03-24T13:24:53Z

Force pushed another iteration (it's also rebased):

kernel support is simplified to mere -EINVAL check,
error evaluation of two-path write to avoid unnecessary warnings,
(restored runtime tests of non-macro BFQ_WEIGHT by dropping a revert patch).

PTAL.

(I'm also not delighted by the state of BFQ APIs but that's what the world evolved into and doesn't seem to change. And since there are BFQ users, at some stage the mess must be harnessed. (I see no reason for not having the io.bfq.weight change captured in /sys/kernel/cgroup/features besides it was forgotten, which makes it less useful nowadays.))

src/core/cgroup.c

Use arithmetic over explicit list of existing property names.

Old kernels (795fe54c2a82 ("bfq: Add per-device weight") v5.4) won't allow io.bfq.weight per-device configuration, don't get too excited about it but give the user one warning message to be aware this settings don't apply.

A single device cannot have more than a single proportional control policy at the same time. So either io.bfq.weight or io.weight will always fail. Process the resulting values and emit a warning message only when none was really configured. This patch also drops warnings when setting [blk]io.bfq.weight fails, the code would be too convoluted otherwise.

Werkov · 2022-03-29T16:16:09Z

Force pushed changes:

property name (squashable),
kernel version comment,
branch conditions simplified,
aggregation function simplified.

lgtm-com · 2022-03-29T17:03:47Z

This pull request introduces 1 alert when merging 3e6eafd into b64f6d8 - view on LGTM.com

new alerts:

1 for FIXME comment

keszybz · 2022-04-07T15:23:11Z

Let's do this. The interface is so bad that no matter what we try to do, the code will need to be ugly.

Werkov mentioned this pull request Dec 16, 2021

bfq.io.weight followups and cleanup #21728

Merged

yuwata added cgroups pid1 labels Dec 16, 2021

keszybz reviewed Dec 20, 2021

View reviewed changes

src/core/cgroup.c Outdated Show resolved Hide resolved

keszybz added the good-to-merge/with-minor-suggestions label Dec 20, 2021

Werkov force-pushed the bfq-io-weight-2 branch from bcd3bc5 to 2caf333 Compare January 10, 2022 17:34

bluca added the please-review label Jan 12, 2022

yuwata approved these changes Jan 12, 2022

View reviewed changes

src/core/cgroup.c Outdated Show resolved Hide resolved

yuwata added needs-rebase and removed please-review labels Jan 12, 2022

keszybz requested changes Jan 12, 2022

View reviewed changes

src/test/test-cgroup-util.c Outdated Show resolved Hide resolved

Werkov force-pushed the bfq-io-weight-2 branch from 2caf333 to a3fd2ca Compare January 12, 2022 17:31

bluca added good-to-merge/waiting-for-reporter-feedback 👍 and removed good-to-merge/with-minor-suggestions needs-rebase labels Jan 13, 2022

bluca reviewed Jan 14, 2022

View reviewed changes

bluca added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-reporter-feedback 👍 labels Jan 19, 2022

poettering added needs-discussion 🤔 and removed good-to-merge/with-minor-suggestions labels Mar 10, 2022

Werkov force-pushed the bfq-io-weight-2 branch from d473255 to 9e93bc5 Compare March 24, 2022 13:07

bluca added please-review and removed needs-discussion 🤔 labels Mar 24, 2022

poettering requested changes Mar 24, 2022

View reviewed changes

src/core/cgroup.c Outdated Show resolved Hide resolved

src/core/cgroup.c Outdated Show resolved Hide resolved

src/core/cgroup.c Outdated Show resolved Hide resolved

src/core/cgroup.c Outdated Show resolved Hide resolved

src/core/cgroup.c Outdated Show resolved Hide resolved

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review labels Mar 24, 2022

Werkov added 3 commits March 29, 2022 17:46

core/cgroup: Refactor device weight property name

1cf4a68

Use arithmetic over explicit list of existing property names.

core/cgroup: Silence warnings IODeviceWeigth= on old kernels

8d75f60

Old kernels (795fe54c2a82 ("bfq: Add per-device weight") v5.4) won't allow io.bfq.weight per-device configuration, don't get too excited about it but give the user one warning message to be aware this settings don't apply.

Werkov force-pushed the bfq-io-weight-2 branch from 9e93bc5 to 3e6eafd Compare March 29, 2022 16:13

bluca added please-review and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Mar 29, 2022

systemd deleted a comment from lgtm-com bot Mar 29, 2022

keszybz removed the please-review label Apr 7, 2022

keszybz merged commit cbb6068 into systemd:main Apr 7, 2022

IODeviceWeight= configures bfq.io.weight too #21795

IODeviceWeight= configures bfq.io.weight too #21795

Werkov commented Dec 16, 2021

keszybz left a comment

Werkov commented Jan 10, 2022

yuwata left a comment

yuwata commented Jan 12, 2022

Werkov commented Jan 12, 2022

bluca commented Jan 13, 2022

yuwata commented Jan 14, 2022

Werkov commented Jan 14, 2022 •

edited

bluca commented Jan 14, 2022

Werkov commented Jan 14, 2022

Werkov commented Jan 14, 2022

bluca Jan 14, 2022

Werkov Jan 17, 2022

bluca Jan 19, 2022

poettering commented Mar 10, 2022

Werkov commented Mar 10, 2022

poettering commented Mar 24, 2022

Werkov commented Mar 24, 2022

Werkov commented Mar 29, 2022

lgtm-com bot commented Mar 29, 2022

keszybz commented Apr 7, 2022

IODeviceWeight= configures bfq.io.weight too #21795

IODeviceWeight= configures bfq.io.weight too #21795

Conversation

Werkov commented Dec 16, 2021

keszybz left a comment

Choose a reason for hiding this comment

Werkov commented Jan 10, 2022

yuwata left a comment

Choose a reason for hiding this comment

yuwata commented Jan 12, 2022

Werkov commented Jan 12, 2022

bluca commented Jan 13, 2022

yuwata commented Jan 14, 2022

Werkov commented Jan 14, 2022 • edited

bluca commented Jan 14, 2022

Werkov commented Jan 14, 2022

Werkov commented Jan 14, 2022

bluca Jan 14, 2022

Choose a reason for hiding this comment

Werkov Jan 17, 2022

Choose a reason for hiding this comment

bluca Jan 19, 2022

Choose a reason for hiding this comment

poettering commented Mar 10, 2022

Werkov commented Mar 10, 2022

poettering commented Mar 24, 2022

Werkov commented Mar 24, 2022

Werkov commented Mar 29, 2022

lgtm-com bot commented Mar 29, 2022

keszybz commented Apr 7, 2022

Werkov commented Jan 14, 2022 •

edited