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/cgroups/fs: code cleanups #2498

Merged
merged 4 commits into from
Jul 9, 2020

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jul 3, 2020

this is separated out from #2438 in order to make review easier

This contains some minor cleanups and optimizations, but mostly just code removal.

TODO: add
* db45ce7
* 44a02d2
once #2496 is merged

@kolyshkin kolyshkin changed the title cgroup v1 code cleanups libct/cgroups/fs: code cleanups Jul 3, 2020
kolyshkin added 3 commits July 6, 2020 18:02
To my surprise, those are not used anywhere in the code.

Signed-off-by: Kir Kolyshkin <[email protected]>
Half of controllers' GetStats just return nil, and most of the others
ignore ENOENT on files, so it will be cheaper to not check that the
path exists in the main GetStats method, offloading that to the
controllers.

Drop PathExists check from GetStats, add it to those controllers'
GetStats where it was missing.

Signed-off-by: Kir Kolyshkin <[email protected]>
It does not add any value.

Signed-off-by: Kir Kolyshkin <[email protected]>
Instead of iterating over m.paths, iterate over subsystems and look up
the path for each. This is faster since a map lookup is faster than
iterating over the names in Get. A quick benchmark shows that the new
way is 2.5x faster than the old one.

Note though that this is not done to make things faster, as savings are
negligible, but to make things simpler by removing some code.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin marked this pull request as ready for review July 7, 2020 02:11
@kolyshkin kolyshkin added the kind/refactor refactoring label Jul 7, 2020
@kolyshkin
Copy link
Contributor Author

For the reference, here's the benchmark used for the last commit

package fs

import (
	"testing"
)

func initPaths() map[string]string {
	paths := make(map[string]string)

	for _, ss := range subsystems {
		paths[ss.Name()] = "/sys/fs/cgroup/" + ss.Name() + "/user.slice/user-1000.slice/session-881.scope/xx88"

	}

	return paths
}

func BenchmarkIterateOverPaths(b *testing.B) {
	paths := initPaths()

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		for name, path := range paths {
			ss, err := subsystems.Get(name)
			if err == errSubsystemDoesNotExist || path == "" || ss == nil {
				continue
			}
		}
	}
}

func BenchmarkIterateOverSubsystems(b *testing.B) {
	paths := initPaths()

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		for _, ss := range subsystems {
			path := paths[ss.Name()]
			if path == "" {
				continue
			}
		}
	}
}

type subsystem interface {
// Name returns the name of the subsystem.
Name() string
// Returns the stats, as 'stats', corresponding to the cgroup under 'path'.
GetStats(path string, stats *cgroups.Stats) error
// Removes the cgroup represented by 'cgroupData'.
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be of use to external clients and maybe later on if/when we factor this out into a cgroups library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Theoretically, yes.

Practically,

  1. I don't see it being used by kubernetes;
  2. It uses d.path which is expensive, it requires to parse mountinfo (this is actually the main point of it being removed);
  3. Its API is not in line with e.g. Set (which does not use d.path but the path is provided directly).

Also, if needed, it can be trivial to reimplement / re-add, as it doesn't do anything special, so I see no point in keeping it.

@mrunalp mrunalp merged commit cf1273a into opencontainers:master Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants