Exposing memory.numa_stats#2278
Conversation
iwankgb
force-pushed
the
memory.numa_stats
branch
2 times, most recently
from
b136fac to
8a031e3
Compare
|
Code looks good, but needs rebase |
iwankgb
force-pushed
the
memory.numa_stats
branch
2 times, most recently
from
50dd50b to
b93ca20
Compare
|
@AkihiroSuda rebased :) |
|
LGTM |
|
@caniszczyk, @crosbymichael, @cyphar, @dqminh, @hqhq, @mrunalp, @rjnagal, @vmarmol - second opinion would be appreciated. |
| return cgroups.PageUsageByNUMA{}, err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
error checking for scanner is missing here
| numaStatColumnSliceLength = 2 | ||
| cgroupMemorySwapLimit = "memory.memsw.limit_in_bytes" | ||
| cgroupMemoryLimit = "memory.limit_in_bytes" | ||
| cgroupMemoryPagesByNuma = "memory.numa_stat" |
There was a problem hiding this comment.
Is this available in both cgroup v1 and v2?
| if err := blkioStatEntryEquals(expected.IoServiceBytesRecursive, actual.IoServiceBytesRecursive); err != nil { | ||
| logrus.Printf("blkio IoServiceBytesRecursive do not match - %s\n", err) | ||
| t.Fail() | ||
| t.Errorf("blkio IoServiceBytesRecursive do not match - %s\n", err) |
There was a problem hiding this comment.
Please move this cleanup out to a separate commit that goes first.
There was a problem hiding this comment.
Aren't all PR's commits squashed before merge anyway?
| } | ||
|
|
||
| if isNUMANode.MatchString(pagesByNode[numaStatTypeIndex]) { | ||
| nodeID, err := strconv.ParseUint(pagesByNode[numaStatTypeIndex][1:], 10, 8) |
There was a problem hiding this comment.
I think this check can be simplified by just checking for capital N plus no error from ParseUint.
| columns := strings.SplitN(scanner.Text(), numaStatColumnSeparator, numaStatMaxColumns) | ||
|
|
||
| for _, column := range columns { | ||
| pagesByNode := strings.SplitN(column, numaStatKeyValueSeparator, numaStatColumnSliceLength) |
There was a problem hiding this comment.
since numaStatKeyValueSeparator is only used once, it's better to inline it, e.g.
strings.SplitN(column, "=", 2)There was a problem hiding this comment.
All other constants are used once, I think. I still prefer them than magic strings.
Making information on page usage by type and NUMA node available Signed-off-by: Maciej "Iwan" Iwanowski <maciej.iwanowski@intel.com>
iwankgb
force-pushed
the
memory.numa_stats
branch
from
b93ca20 to
7fe0a98
Compare
|
@AkihiroSuda I would appreciate one more iteration of review. |
|
Travis build succeeded but status has not been reported: https://travis-ci.org/github/opencontainers/runc/builds/672578771. |
|
LGTM |
1 similar comment
|
LGTM |
4 participants
Making information on page usage by type and NUMA node available.