Skip to content

Conversation

@poettering
Copy link
Member

Let's remove some old cruft, in preparation for merging #10507

Copy link
Member

@cdown cdown left a comment

Choose a reason for hiding this comment

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

This looks really good. Thanks a lot for your work on this!

I have no idea on the SMACK stuff, but perhaps nobody else really does either, so... ;-)

@keszybz keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 16, 2018
@keszybz
Copy link
Member

keszybz commented Nov 16, 2018

I'm setting the flag in the sense "needs response to comments". The comments are mostly questions.

This removes the ability to configure which cgroup controllers to mount
together. Instead, we'll now hardcode that "cpu" and "cpuacct" are
mounted together as well as "net_cls" and "net_prio".

The concept of mounting controllers together has no future as it does
not exist to cgroupsv2. Moreover, the current logic is systematically
broken, as revealed by the discussions in systemd#10507. Also, we surveyed Red
Hat customers and couldn't find a single user of the concept (which
isn't particularly surprising, as it is broken...)

This reduced the (already way too complex) cgroup handling for us, since
we now know whenever we make a change to a cgroup for one controller to
which other controllers it applies.
… mask according to cpu/cpuacct joint mounting

Note that for cgroup_context_get_mask() this doesn't actually change
much, but it does prepare the ground for systemd#10507 later on.
… take jointly mounted controlelrs into account

If we create a cgroup in one controller it might already have been
created in another too, if we have jointly mounted controllers. Take
that into consideration.
@poettering poettering removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 16, 2018
@poettering
Copy link
Member Author

I force pushed a new version. Addresses everything mentioned, modulo my comments above. I also dropped the if clean-up patch, it's now part of #10803 instead. No other changes. PTAL

@cdown
Copy link
Member

cdown commented Nov 16, 2018

Looks good to me from cgroups side.

@poettering
Copy link
Member Author

Thanks for the review! I think that should be enough to set the green label (in particular as @keszybz pretty much liked it too already.)

@poettering poettering added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Nov 16, 2018
@poettering poettering merged commit 6415fec into systemd:master Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cgroups good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed pid1

Development

Successfully merging this pull request may close these issues.

3 participants