From fad92bbffa9c13652c07f1966606089e28442a87 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 29 May 2020 17:06:36 -0700 Subject: [PATCH 1/3] cgroupv1/Apply: do not overuse d.path/getSubsystemPath 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 --- libcontainer/cgroups/fs/fs.go | 24 ++++++++++++------------ libcontainer/cgroups/systemd/v1.go | 15 +++++++-------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go index 5314260842e..98e1e342663 100644 --- a/libcontainer/cgroups/fs/fs.go +++ b/libcontainer/cgroups/fs/fs.go @@ -220,26 +220,26 @@ func (m *manager) Apply(pid int) (err error) { var c = m.cgroups - d, err := getCgroupData(m.cgroups, pid) - if err != nil { - return err - } - m.paths = make(map[string]string) if c.Paths != nil { + cgMap, err := cgroups.ParseCgroupFile("/proc/self/cgroup") + if err != nil { + return err + } for name, path := range c.Paths { - _, err := d.path(name) - if err != nil { - if cgroups.IsNotFound(err) { - continue - } - return err + // XXX(kolyshkin@): why this check is needed? + if _, ok := cgMap[name]; ok { + m.paths[name] = path } - m.paths[name] = path } return cgroups.EnterPid(m.paths, pid) } + d, err := getCgroupData(m.cgroups, pid) + if err != nil { + return err + } + for _, sys := range subsystems { p, err := d.path(sys.Name()) if err != nil { diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index aaf351df157..67870737898 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -105,16 +105,15 @@ func (m *legacyManager) Apply(pid int) error { defer m.mu.Unlock() if c.Paths != nil { paths := make(map[string]string) + cgMap, err := cgroups.ParseCgroupFile("/proc/self/cgroup") + if err != nil { + return err + } + // XXX(kolyshkin@): why this check is needed? for name, path := range c.Paths { - _, err := getSubsystemPath(m.cgroups, name) - if err != nil { - // Don't fail if a cgroup hierarchy was not found, just skip this subsystem - if cgroups.IsNotFound(err) { - continue - } - return err + if _, ok := cgMap[name]; ok { + paths[name] = path } - paths[name] = path } m.paths = paths return cgroups.EnterPid(m.paths, pid) From f075084a47fbdf638d0b424049bcba2950d4813b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 29 May 2020 17:59:03 -0700 Subject: [PATCH 2/3] cgroupv1/systemd: rework Apply/joinCgroups 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 --- libcontainer/cgroups/systemd/v1.go | 56 +++++++++++------------------- 1 file changed, 20 insertions(+), 36 deletions(-) diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index 67870737898..631265e3f3f 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -178,14 +178,16 @@ func (m *legacyManager) Apply(pid int) error { return err } - if err := joinCgroups(c, pid); err != nil { - return err - } - paths := make(map[string]string) for _, s := range legacySubsystems { subsystemPath, err := getSubsystemPath(m.cgroups, s.Name()) if err != nil { + // Even if it's `not found` error, we'll return err + // because devices cgroup is hard requirement for + // container security. + if s.Name() == "devices" { + return err + } // Don't fail if a cgroup hierarchy was not found, just skip this subsystem if cgroups.IsNotFound(err) { continue @@ -195,6 +197,11 @@ func (m *legacyManager) Apply(pid int) error { paths[s.Name()] = subsystemPath } m.paths = paths + + if err := m.joinCgroups(pid); err != nil { + return err + } + return nil } @@ -227,48 +234,25 @@ func (m *legacyManager) Path(subsys string) string { return m.paths[subsys] } -func join(c *configs.Cgroup, subsystem string, pid int) (string, error) { - path, err := getSubsystemPath(c, subsystem) - if err != nil { - return "", err - } - - if err := os.MkdirAll(path, 0755); err != nil { - return "", err - } - if err := cgroups.WriteCgroupProc(path, pid); err != nil { - return "", err - } - return path, nil -} - -func joinCgroups(c *configs.Cgroup, pid int) error { +func (m *legacyManager) joinCgroups(pid int) error { for _, sys := range legacySubsystems { name := sys.Name() switch name { case "name=systemd": // let systemd handle this case "cpuset": - path, err := getSubsystemPath(c, name) - if err != nil && !cgroups.IsNotFound(err) { - return err - } - s := &fs.CpusetGroup{} - if err := s.ApplyDir(path, c, pid); err != nil { - return err + if path, ok := m.paths[name]; ok { + s := &fs.CpusetGroup{} + if err := s.ApplyDir(path, m.cgroups, pid); err != nil { + return err + } } default: - _, err := join(c, name, pid) - if err != nil { - // Even if it's `not found` error, we'll return err - // because devices cgroup is hard requirement for - // container security. - if name == "devices" { + if path, ok := m.paths[name]; ok { + if err := os.MkdirAll(path, 0755); err != nil { return err } - // For other subsystems, omit the `not found` error - // because they are optional. - if !cgroups.IsNotFound(err) { + if err := cgroups.WriteCgroupProc(path, pid); err != nil { return err } } From 940e15479fa368720c97dbca23a0630f95a81da0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 29 May 2020 20:48:07 -0700 Subject: [PATCH 3/3] cgroupv1/systemd: (re)use m.paths 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 --- libcontainer/cgroups/systemd/v1.go | 33 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go index 631265e3f3f..cc2d19cd2f1 100644 --- a/libcontainer/cgroups/systemd/v1.go +++ b/libcontainer/cgroups/systemd/v1.go @@ -289,15 +289,14 @@ func getSubsystemPath(c *configs.Cgroup, subsystem string) (string, error) { } func (m *legacyManager) Freeze(state configs.FreezerState) error { - path, err := getSubsystemPath(m.cgroups, "freezer") - if err != nil { - return err + path, ok := m.paths["freezer"] + if !ok { + return errSubsystemDoesNotExist } prevState := m.cgroups.Resources.Freezer m.cgroups.Resources.Freezer = state freezer := &fs.FreezerGroup{} - err = freezer.Set(path, m.cgroups) - if err != nil { + if err := freezer.Set(path, m.cgroups); err != nil { m.cgroups.Resources.Freezer = prevState return err } @@ -305,17 +304,17 @@ func (m *legacyManager) Freeze(state configs.FreezerState) error { } func (m *legacyManager) GetPids() ([]int, error) { - path, err := getSubsystemPath(m.cgroups, "devices") - if err != nil { - return nil, err + path, ok := m.paths["devices"] + if !ok { + return nil, errSubsystemDoesNotExist } return cgroups.GetPids(path) } func (m *legacyManager) GetAllPids() ([]int, error) { - path, err := getSubsystemPath(m.cgroups, "devices") - if err != nil { - return nil, err + path, ok := m.paths["devices"] + if !ok { + return nil, errSubsystemDoesNotExist } return cgroups.GetAllPids(path) } @@ -386,9 +385,9 @@ func (m *legacyManager) Set(container *configs.Config) error { for _, sys := range legacySubsystems { // Get the subsystem path, but don't error out for not found cgroups. - path, err := getSubsystemPath(container.Cgroups, sys.Name()) - if err != nil && !cgroups.IsNotFound(err) { - return err + path, ok := m.paths[sys.Name()] + if !ok { + continue } if err := sys.Set(path, container.Cgroups); err != nil { return err @@ -430,9 +429,9 @@ func (m *legacyManager) GetCgroups() (*configs.Cgroup, error) { } func (m *legacyManager) GetFreezerState() (configs.FreezerState, error) { - path, err := getSubsystemPath(m.cgroups, "freezer") - if err != nil && !cgroups.IsNotFound(err) { - return configs.Undefined, err + path, ok := m.paths["freezer"] + if !ok { + return configs.Undefined, nil } freezer := &fs.FreezerGroup{} return freezer.GetState(path)