Skip to content

Commit

Permalink
Revert "Use update time to detect if kmem limits have been set"
Browse files Browse the repository at this point in the history
Revert: #935
Fixes: #946

I can reproduce #946 on some machines, the problem is on
some machines, it could be very fast that modify time
of `memory.kmem.limit_in_bytes` could be the same as
before it's modified.

And now we'll call `SetKernelMemory` twice on container
creation which cause the second time failure.

Revert this before we find a better solution.

Signed-off-by: Qiang Huang <[email protected]>
  • Loading branch information
hqhq committed Jul 21, 2016
1 parent 4dedd09 commit 15c93ee
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 42 deletions.
60 changes: 24 additions & 36 deletions libcontainer/cgroups/fs/memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@ package fs
import (
"bufio"
"fmt"
"math"
"os"
"path/filepath"
"strconv"
"strings"
"time"

"github.com/opencontainers/runc/libcontainer/cgroups"
"github.com/opencontainers/runc/libcontainer/configs"
"github.com/opencontainers/runc/libcontainer/system"
)

type MemoryGroup struct {
Expand All @@ -27,65 +28,51 @@ func (s *MemoryGroup) Apply(d *cgroupData) (err error) {
if err != nil && !cgroups.IsNotFound(err) {
return err
}
// reset error.
err = nil
if path == "" {
// Invalid input.
return fmt.Errorf("invalid path for memory cgroups: %+v", d)
}
defer func() {
if err != nil {
os.RemoveAll(path)
}
}()
if !cgroups.PathExists(path) {
if err = os.MkdirAll(path, 0755); err != nil {
return err
}
}
if memoryAssigned(d.config) {
if path != "" {
if err := os.MkdirAll(path, 0755); err != nil {
return err
}
}
// We have to set kernel memory here, as we can't change it once
// processes have been attached to the cgroup.
if err = s.SetKernelMemory(path, d.config); err != nil {
if err := s.SetKernelMemory(path, d.config); err != nil {
return err
}
}
defer func() {
if err != nil {
os.RemoveAll(path)
}
}()

// 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 _, jerr := d.join("memory"); jerr != nil && !cgroups.IsNotFound(jerr) {
err = jerr
_, err = d.join("memory")
if err != nil && !cgroups.IsNotFound(err) {
return err
}
return nil
}

func getModifyTime(path string) (time.Time, error) {
stat, err := os.Stat(path)
if err != nil {
return time.Time{}, fmt.Errorf("failed to get memory cgroups creation time: %v", err)
}
return stat.ModTime(), nil
}

func (s *MemoryGroup) SetKernelMemory(path string, cgroup *configs.Cgroup) error {
// This has to be done separately because it has special
// constraints (it can only be initialized before setting up a
// hierarchy or adding a task to the cgroups. However, if
// sucessfully initialized, it can be updated anytime afterwards)
if cgroup.Resources.KernelMemory != 0 {
kmemInitialized := false
// Is kmem.limit_in_bytes already set?
// memory.kmem.max_usage_in_bytes is a read-only file. Use it to get cgroups creation time.
kmemCreationTime, err := getModifyTime(filepath.Join(path, "memory.kmem.max_usage_in_bytes"))
kmemValue, err := getCgroupParamUint(path, "memory.kmem.limit_in_bytes")
if err != nil {
return err
}
kmemLimitsUpdateTime, err := getModifyTime(filepath.Join(path, "memory.kmem.limit_in_bytes"))
if err != nil {
return err
switch system.GetLongBit() {
case 32:
kmemInitialized = uint32(kmemValue) != uint32(math.MaxUint32)
case 64:
kmemInitialized = kmemValue != uint64(math.MaxUint64)
}
// kmem.limit_in_bytes has already been set if its update time is after that of creation time.
// We use `!=` op instead of `>` because updates are losing precision compared to creation.
kmemInitialized := !kmemLimitsUpdateTime.Equal(kmemCreationTime)
if !kmemInitialized {
// If there's already tasks in the cgroup, we can't change the limit either
tasks, err := getCgroupParamString(path, "tasks")
Expand All @@ -96,6 +83,7 @@ func (s *MemoryGroup) SetKernelMemory(path string, cgroup *configs.Cgroup) error
return fmt.Errorf("cannot set kmem.limit_in_bytes after task have joined this cgroup")
}
}

if err := writeFile(path, "memory.kmem.limit_in_bytes", strconv.FormatInt(cgroup.Resources.KernelMemory, 10)); err != nil {
return err
}
Expand Down
6 changes: 0 additions & 6 deletions libcontainer/cgroups/fs/memory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,12 +227,6 @@ func TestMemorySetKernelMemory(t *testing.T) {
helper.writeFileContents(map[string]string{
"memory.kmem.limit_in_bytes": strconv.Itoa(kernelMemoryBefore),
})
helper.writeFileContents(map[string]string{
"memory.kmem.max_usage_in_bytes": strconv.Itoa(kernelMemoryBefore),
})
helper.writeFileContents(map[string]string{
"tasks": "",
})

helper.CgroupData.config.Resources.KernelMemory = kernelMemoryAfter
memory := &MemoryGroup{}
Expand Down
19 changes: 19 additions & 0 deletions libcontainer/system/sysconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,28 @@ package system

/*
#include <unistd.h>
#include <limits.h>
int GetLongBit() {
#ifdef _SC_LONG_BIT
int longbits;
longbits = sysconf(_SC_LONG_BIT);
if (longbits < 0) {
longbits = (CHAR_BIT * sizeof(long));
}
return longbits;
#else
return (CHAR_BIT * sizeof(long));
#endif
}
*/
import "C"

func GetClockTicks() int {
return int(C.sysconf(C._SC_CLK_TCK))
}

func GetLongBit() int {
return int(C.GetLongBit())
}

0 comments on commit 15c93ee

Please sign in to comment.