-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Exposing memory.numa_stats #2278
Exposing memory.numa_stats #2278
Conversation
b136fac
to
8a031e3
Compare
Code looks good, but needs rebase |
50dd50b
to
b93ca20
Compare
@AkihiroSuda rebased :) |
@caniszczyk, @crosbymichael, @cyphar, @dqminh, @hqhq, @mrunalp, @rjnagal, @vmarmol - second opinion would be appreciated. |
return cgroups.PageUsageByNUMA{}, err | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this available in both cgroup v1 and v2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it's v1-only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see it in v2.
@@ -26,98 +25,109 @@ func blkioStatEntryEquals(expected, actual []cgroups.BlkioStatEntry) error { | |||
|
|||
func expectBlkioStatsEquals(t *testing.T, expected, actual cgroups.BlkioStats) { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this cleanup out to a separate commit that goes first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All other constants are used once, I think. I still prefer them than magic strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments
Making information on page usage by type and NUMA node available Signed-off-by: Maciej "Iwan" Iwanowski <maciej.iwanowski@intel.com>
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. |
Making information on page usage by type and NUMA node available.