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: add disable_cgroup_devices build tag #3421

Closed

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Mar 18, 2022

The idea is to use this tag for software which wants to use
libcontainer/cgroups packages, but does not require the functionality
to manage devices in cgroups, which brings some additional overhead,
especially in fs2 where it exports cilium/ebpf. Using this build tag
enables that software to save about 586 Kb (text+data+bss), which
reduces runc binary size by about 6% (using go 1.18.0):

       text    data     bss     dec     hex filename
    5800416 3745184  229768 9775368  952908 runc.dev
    5440984 3505336  228904 9175224  8c00b8 runc.no-dev

(The difference is more dramatic then using older versions of golang, e.g. 1.17.x)

Obviously, we do not want runc to be compiled with this tag, so add
a guard to main.go and libcontainer itself.

NOTE than the tag is used, the packages still create a directory under
devices cgroup root, and it is used by Exists/GetPids/GetAllPids,
but otherwise nothing is done with devices. The effect is similar to
that of Resources.SkipDevices and Resources.SkipFreezeOnSet
(for systemd cgroup v1) both set to true, except it is a compile-time thing.

Document the tag in README, and add a unit tests of libct/cg with this tag set.

@kolyshkin kolyshkin changed the title libct/cg: support disable_cgroup_devices build tag libct/cg: add disable_cgroup_devices build tag Mar 18, 2022
@kolyshkin kolyshkin force-pushed the add-disable-cgroup-devices branch 2 times, most recently from f54b538 to 6a897a1 Compare March 18, 2022 19:40
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

I don't think we should complicate tags.

Can we just move fs2.setDevices() to factory_linux.go so as to decouple fs2 pkg from the ebpf pkg?

@kolyshkin kolyshkin force-pushed the add-disable-cgroup-devices branch 2 times, most recently from 1d3cca9 to ec2297f Compare March 23, 2022 00:28
@kolyshkin
Copy link
Contributor Author

Can we just move fs2.setDevices() to factory_linux.go so as to decouple fs2 pkg from the ebpf pkg?

I looked at it, and it seems this will require more changes. Currently devices are an integral part of cgroup manager, and while I can see how it is decoupled, it seems it's going to look ugly.

@kolyshkin kolyshkin force-pushed the add-disable-cgroup-devices branch 3 times, most recently from b81426f to 67d2fb7 Compare March 23, 2022 01:39
@kolyshkin
Copy link
Contributor Author

Can we just move fs2.setDevices() to factory_linux.go so as to decouple fs2 pkg from the ebpf pkg?

I took a look, and it seems more complicated than that. Devices is the integral part of fs manager, and they can definitely be separated out, but that looks like a medium-sized refactoring, which I don't quite know how to do yet.

In the meantime I updated this PR to be more reviewable -- first commit merely moves the code around (no code or functionality changes), and the second adds the tags, tests, and docs.

@kolyshkin kolyshkin marked this pull request as ready for review March 23, 2022 02:08
This moves the functionality related to devices, SkipDevices, and
SkipFreezeOnSet to a separate file, in preparation for the next commit.

No code changes.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The idea is to use this tag for software which wants to use
libcontainer/cgroups packages, but does not require the functionality
to manage devices in cgroups, which brings some additional overhead,
especially in fs2 where it exports cilium/ebpf. Using this  build tag
enables that software to save over 900 Kb (text+data+bss), which
reduces runc binary size by more than 8%:

   text	   data	    bss	    dec	    hex	filename
7016344	3938048	 229704	11184096	 aaa7e0	runc.devices
6423082	3597960	 228808	10249850	 9c667a	runc.no-devices

Obviously, we do not want runc to be compiled with this tag, so add
a guard to main.go and libcontainer itself.

NOTE than the tag is used for fs, we still create a directory under
devices cgroup root, and it is used by Exists/GetPids/GetAllPids,
but otherwise nothing is done with devices. The effect is similar to
that of Resources.SkipDevices.

Document the tag in README, and test this tag in unittest target.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@cdoern
Copy link
Contributor

cdoern commented Mar 31, 2022

@kolyshkin this seems to be the preferable option in podman with the tag as is

@kolyshkin
Copy link
Contributor Author

Can we just move fs2.setDevices() to factory_linux.go so as to decouple fs2 pkg from the ebpf pkg?

So, I took another look at this and it seems this is more complicated than that. Doing this properly requires removing devices management from the cgroup managers (basically, creating a separate "devices" manager which is now part of cgroups.

Ultimately it might be the best thing to do (for one thing, we can remove all these ugly SkipDevices and SkipFreezeOnSet flags), but it is nevertheless a big change, whereas adding a build tag (this PR) seems not overly complicated.

I don't know. Should I try to separate devices out of cgroup manager instead of what this PR does, @opencontainers/runc-maintainers ?

@kolyshkin
Copy link
Contributor Author

I don't know. Should I try to separate devices out of cgroup manager instead of what this PR does, @opencontainers/runc-maintainers ?

Implemented the separation in #3452, PTAL

@kolyshkin
Copy link
Contributor Author

We went with #3452 instead, so closing this one

@kolyshkin kolyshkin closed this Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants