Skip to content

Unify behavior for memory cgroup #343

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.

Sign up for GitHub

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

Merged
mrunalp merged 1 commit into from
Nov 2, 2015
Merged

Unify behavior for memory cgroup #343

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions libcontainer/cgroups/fs/memory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,19 @@ type MemoryGroup struct {

func (s *MemoryGroup) Apply(d *data) (err error) {
path, err := d.path("memory")
if err != nil {
if cgroups.IsNotFound(err) {
return nil
}
if err != nil && !cgroups.IsNotFound(err) {
return err
}
if err := os.MkdirAll(path, 0755); err != nil {
return err
if memoryAssigned(d.c) {
if path != "" {
if err := os.MkdirAll(path, 0755); err != nil {
Copy link
Contributor

@rajasec rajasec Oct 26, 2015

Choose a reason for hiding this comment

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

@hqhq
if path not equal to empty( which means path exists), code is creating mkdir again with "path"
is that intentional ?

Copy link
Contributor Author

@hqhq hqhq Oct 26, 2015

Choose a reason for hiding this comment

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

How "again"? It's the first time we create "path".

return err
}
}

if err := s.Set(path, d.c); err != nil {
Copy link
Contributor

@rajasec rajasec Oct 20, 2015

Choose a reason for hiding this comment

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

@hqhq
if memory subsystem is not mounted, empty path is passed in set interface ? is that correct ?

Copy link
Contributor Author

@hqhq hqhq Oct 20, 2015

Choose a reason for hiding this comment

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

Yes, it's intended, so that we can get error in writeFile function.

return err
}
}

defer func() {
Expand All @@ -35,13 +40,10 @@ func (s *MemoryGroup) Apply(d *data) (err error) {
}
}()

if err := s.Set(path, d.c); err != nil {
return err
}

// We need to join memory cgroup after set memory limits, because
// kmem.limit_in_bytes can only be set when the cgroup is empty.
if _, err = d.join("memory"); err != nil {
_, err = d.join("memory")
Copy link
Contributor

@rajasec rajasec Oct 20, 2015

Choose a reason for hiding this comment

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

Earlier mkdir was called explicitly in line number 27, now via join again mkdir gets called. Looks like mkdir is called twice
is than redundant ?

Copy link
Contributor Author

@hqhq hqhq Oct 20, 2015

Choose a reason for hiding this comment

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

It is possible we mkdir twice, but I don't think that hurts, use join to enter a cgroup is simple and clear.
We can avoid this redundant, I'm not sure it's worth doing.

if err != nil && !cgroups.IsNotFound(err) {
return err
}

Expand Down Expand Up @@ -132,6 +134,15 @@ func (s *MemoryGroup) GetStats(path string, stats *cgroups.Stats) error {
return nil
}

func memoryAssigned(cgroup *configs.Cgroup) bool {
return cgroup.Memory != 0 ||
cgroup.MemoryReservation != 0 ||
cgroup.MemorySwap > 0 ||
cgroup.KernelMemory > 0 ||
cgroup.OomKillDisable ||
cgroup.MemorySwappiness != -1
}

func getMemoryData(path, name string) (cgroups.MemoryData, error) {
memoryData := cgroups.MemoryData{}

Expand Down