cgroups: Add support for `cgroup.kill` added in Linux Kernel 5.14 #3199

flouthoc · 2021-09-05T08:16:19Z

Following PR adds support for cgroup.kill to kill all the processes in
a cgroup instead of iterating over each process to save cost if
possible.

Linux kernel 5.14 adds support for cgroup.kill.

Ref

flouthoc · 2021-09-05T08:21:13Z

@giuseppe @AkihiroSuda @kolyshkin PTAL

flouthoc · 2021-09-05T08:22:09Z

PS: this is already added in crun by @giuseppe via containers/crun#724

libcontainer/init_linux.go

libcontainer/cgroups/cgroups.go

libcontainer/cgroups/fs/fs.go

libcontainer/init_linux.go

kolyshkin · 2021-09-06T19:16:44Z

This would fix #3135

libcontainer/init_linux.go

flouthoc · 2021-09-07T09:32:56Z

@cyphar @AkihiroSuda @kolyshkin Addressed all the feedback points in latest commit. Could you please take a look again.

libcontainer/cgroups/cgroups.go

libcontainer/cgroups/fs/fs.go

libcontainer/cgroups/fscommon/kill.go

kolyshkin · 2021-09-08T02:15:00Z

libcontainer/init_linux.go

 	if err := m.Freeze(configs.Frozen); err != nil {
 		logrus.Warn(err)
 	}


Again, the whole point of introducing cgroup.kill is you don't have to freeze/thaw, and yet you do that.

@kolyshkin ah i moved it after "cgroup.kill" in latest commit but as per man-pages it says

+ Killing a cgroup tree will deal with concurrent forks appropriately and + is protected against migrations. If callers require strict guarantees + they can issue the cgroup.kill request after a freezing the cgroup via + cgroup.freeze.```

Please see the discussion between @brauner and @rgushchin, which stars here.

In short, the sentence you quote was only in v1 of the patchset and it was dropped later. IOW freezing is not required, and it's one of the "selling points" of this feature.

@kolyshkin thanks for pointing out this discussion this has been fixed in last commit.

Marking this block as resolved since this was fixed.

libcontainer/init_linux.go

kolyshkin · 2021-09-08T02:19:32Z

libcontainer/cgroups/systemd/v1.go

+	defer m.mu.Unlock()
+	// Since "cgroup.kill" is not a subsystem and we don't want to regenerate path using pid
+	// strip dir path from any available subsystem, most likely devices since its always there for v1
+	dirPath, _ := filepath.Split(m.paths["devices"])


So, why are you using filepath.Split here?

@kolyshkin We want path to tree where we are going to write "cgroup.kill" for v1 , example /sys/fs/cgroup/containerID/cgroup.kill

The path you provided as an example does not make sense for cgroup v1.

The path that will be produced as a result of filepath.Split would be different from your example (it will actually be pointing to the parent cgroup, and I don't think it's a good idea to use it).

So, it looks like you're not testing this code at all (at least for v1). Do you?

@kolyshkin ah my bad i didn't spent time testing this on v1 but I followed the path construction which was being created on crun let me harden this for v1 and if its not supported i will remove it. But if its not available for v1 i think man-pages should be updated.

@kolyshkin I am unable to write file cgroup.kill when working with v1 under any of the paths even the base path but works when cgroup_hierarchy=1 also tried with root. But since its returning error for v1 so code fallbacks to original slow process killing. PS: did not test this with using v1 + hybrid

@kolyshkin Now i am thinking should v1 managers just return error("Not supported") but i am not sure if it works with v1+hybrid or not.

Todo: switch this with m.paths[""] to get the path for v1+hybrid. Wait for : #2087

kolyshkin · 2021-09-08T03:23:56Z

@flouthoc on what distro are you testing this? Problem is, we don't have 5.14+ kernel in CI yet.

flouthoc · 2021-09-08T05:04:50Z

@kolyshkin I am using ubuntu 21.04 but 5.14 is not shipped by default but its available on kernel.ubuntu.com mainline.

kolyshkin · 2021-09-09T00:33:16Z

I have booted 5.14 myself and, as I was suspecting, the feature is only available for cgroup v2 (or when using v1 + hybrid hierarchy).

So, it does not make sense to add to v1 managers (unless they gain hybrid support), and the code you're using to get paths to cgroup.kill in v1 managers is probably not working.

Given the fact that this is not ready, and we don't have kernel 5.14 in CI, I think we have to postpone it to 1.2.0.

flouthoc · 2021-09-09T04:59:13Z

@kolyshkin fair points i think we can hold on to this till kernel 5.14 and I'll confirm if this works for v1 at all , if not i will remove the case for v1. Will add a .bats test for v2 does that sounds good ?

kolyshkin · 2021-09-13T20:43:20Z

and I'll confirm if this works for v1 at all , if not i will remove the case for v1.

There's no cgroup.kill for v1, but it is there in case hybrid hierarchy is used. Support for hybrid hierarchy is added by various PRs. In this particular case, we need #2087, and once it's in, we can use Path("") to get path to the hybrid hierarchy, if available.

flouthoc · 2021-09-14T04:46:26Z

@kolyshkin Sure makes sense lets wait for hybrid hierarchy.

kolyshkin · 2021-09-15T22:29:33Z

Marked as draft as we need

cgroups/systemd: add cgroup-v2 path to the list when using hybrid mode #2087 (or runc exec --cgroup #3059 which carries it) for v1 + hybrid unified path support;
kernel 5.14 in CI;

Problem is, #3059 needs criu 3.16 which is not yet released.

flouthoc · 2021-09-21T16:18:16Z

@kolyshkin @cyphar Just curious how would m.paths[""] would look like for hybrid+v1 could you please state with an example. I'll try setting up a hybrid+v1 in spare time for myself to work on this till other PR gets merged.

flouthoc · 2021-09-21T16:21:54Z

Just want to do this testing myself here cause i think it will take some time to get new kernel in CI meanwhile i can just do testing on my own and we can keep PR on hold.

flouthoc · 2021-09-21T17:01:10Z

Nvm i'll wait for dependent PRs then will pick this up again. I think would be hard to plumb things temporarily into code.

flouthoc · 2021-09-21T17:28:35Z

Just putting mock code so its easier to revisit, please don't review yet.

kolyshkin · 2022-05-31T23:46:35Z

@flouthoc can you revisit this? I almost ended up rewriting this from scratch (which would be a colossal waste of time).

flouthoc · 2022-06-01T03:48:10Z

@kolyshkin Sure I'll revisit this. Thanks for the reminder.

cyphar · 2022-06-01T03:51:40Z

I completely forgot about this patch. Ping me to review it when you're done rebasing. :D

kolyshkin · 2022-12-01T18:53:19Z

@flouthoc can you take a look and refresh this PR? This would be nice to have in 1.2.0

Following PR adds support for `cgroup.kill` to kill all the processes in a cgroup instead of iterating over each process to save cost if possible. Linux kernel 5.14 adds support for `cgroup.kill`. Ref * https://lwn.net/Articles/855049/ * https://lwn.net/Articles/855924/ Signed-off-by: Aditya Rajan <[email protected]> Signed-off-by: Aditya R <[email protected]>

kolyshkin · 2023-04-13T01:13:53Z

I am working on an alternative, implemented without adding a new public method to cgroup managers, and modelled after cgroup.kill kernel tests. Still a WIP.

flouthoc · 2023-04-13T03:51:02Z

@kolyshkin Thanks a lot for taking this over, i am closing this one.

flouthoc force-pushed the cgroup-kill-kernel-5-14 branch from e5e673a to b79fade Compare September 5, 2021 08:20

flouthoc force-pushed the cgroup-kill-kernel-5-14 branch from b79fade to 67bfe95 Compare September 5, 2021 08:23

AkihiroSuda added area/cgroupv2 enhancement impact/changelog labels Sep 5, 2021

AkihiroSuda reviewed Sep 5, 2021

View reviewed changes

libcontainer/init_linux.go Outdated Show resolved Hide resolved

AkihiroSuda added this to the 1.1.0 milestone Sep 5, 2021

flouthoc force-pushed the cgroup-kill-kernel-5-14 branch from 67bfe95 to b89bae4 Compare September 5, 2021 10:09