cgroups/v1: optimize path usage#2494
Merged
AkihiroSuda merged 3 commits intoSep 11, 2020
Merged
Conversation
20 tasks
Contributor
|
Needs rebase. |
kolyshkin
force-pushed
the
cgroupv1-optimize-path
branch
from
f0e3d48 to
703c29d
Compare
Contributor
Author
|
rebased |
kolyshkin
force-pushed
the
cgroupv1-optimize-path
branch
from
703c29d to
90e3e6b
Compare
kolyshkin
force-pushed
the
cgroupv1-optimize-path
branch
from
90e3e6b to
6d90b53
Compare
mrunalp
previously approved these changes
Aug 25, 2020
Contributor
Author
|
@AkihiroSuda PTAL |
kolyshkin
commented
Aug 25, 2020
When paths are set, we only need to place the PID into proper cgroups, and we do know all the paths already. Both fs/d.path() and systemd/v1/getSubsystemPath() parse /proc/self/mountinfo, and the only reason they are used here is to check whether the subsystem is available. Use a much simpler/faster check instead. Frankly, I am not sure why the check is needed at all. Maybe it should be dropped. Also, for fs driver, since d is no longer used in this code path, move its initialization to after it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We call joinCgroups() from Apply, and in there we iterate through the list of subsystems, calling getSubsystemPath() for each. This is expensive, since every getSubsystemPath() involves parsing mountinfo. At the end of Apply(), we iterate through the list of subsystems to fill the m.paths, again calling getSubsystemPath() for every subsystem. As a result, we parse mountinfo about 20 times here. Let's find the paths first and reuse m.paths in joinCgroups(). While at it, since join() is just two calls now, inline it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In all these cases, getSubsystemPath() was already called, and its result stored in m.paths map. It makes no sense to not reuse it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
cgroupv1-optimize-path
branch
from
6d90b53 to
940e154
Compare
AkihiroSuda
reviewed
Aug 25, 2020
| continue | ||
| } | ||
| return err | ||
| // XXX(kolyshkin@): why this check is needed? |
Member
There was a problem hiding this comment.
What happens without this check?
Contributor
Author
There was a problem hiding this comment.
As far as I can see, nothing bad. The paths map is then used to write pid to cgroups, and WriteCgroupProc has a check that path exists.
There may be other scenarios (with software that uses runc/libcontainer) I don't envision, and I have broken stuff in the past, so I am trying to be extra careful here.
So, ideally, I'd remove this, but maybe later, thus TODO.
The new check is already 100x better than the old one, so this patch is a big improvement -- I'm just not sure if the check is needed at all.
mrunalp
approved these changes
Aug 26, 2020
Contributor
Author
|
@AkihiroSuda PTAL |
AkihiroSuda
approved these changes
Sep 11, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this is separated out from #2438 in order to make review easier
Limit the use of
getSubsystemPath()andd.path()functions, reusing them.pathsinstead.See individual commit descriptions for details.
Number of times mountinfo was parsed during
runc --systemd-cgroup runof a busybox container runningsleep 1went down from 50 to 19 as a result of these changes.