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

memory: topology: expose per-numa memory #268

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

ffromani
Copy link
Collaborator

@ffromani ffromani commented Aug 26, 2021

This patch wants to add memory informations on topology.Node,
so we can have per-NUMA-zone memory information.

WIP: tests are broken (and more needs to come), doc is missing,
API is provisional. The basic concepts are there, though.

Signed-off-by: Francesco Romani [email protected]

@ffromani ffromani mentioned this pull request Aug 26, 2021
Copy link
Owner

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Hi @fromanirh! Left some comments/questions/suggestions inline :)

pkg/memory/memory.go Outdated Show resolved Hide resolved
pkg/memory/memory_hugepages.go Outdated Show resolved Hide resolved
pkg/memory/memory_linux.go Show resolved Hide resolved
pkg/memory/memory_linux.go Outdated Show resolved Hide resolved
pkg/topology/topology_linux_test.go Outdated Show resolved Hide resolved
@ffromani
Copy link
Collaborator Author

Thanks for your early feedback @jaypipes ! you raise very valid points and this is totally consider moving forward. At very least I'll most likely remove the hugepage portion from this PR - it wasn't requested by the issue I'm trying to fix anyway.
The more I explore this area the more I see the issues you are highligthing. Once I'm confident the code does a reasonnable thing I'll lift the WIP.

@ffromani ffromani force-pushed the numa-memory-info branch 3 times, most recently from 2e702ed to 8f0a67d Compare September 13, 2021 09:57
@ffromani ffromani changed the title WIP: memory: topology: expose per-numa memory memory: topology: expose per-numa memory Sep 13, 2021
@ffromani
Copy link
Collaborator Author

@jaypipes I think this PR is ready for another round of review. I removed all the hugepage reporting (it was at very least too premature, sorry for the noise) and fixed the pending items I had.

@ffromani
Copy link
Collaborator Author

@jaypipes - not urgent at all, but I think this PR is ready for review, so just keeping it on our radar

pkg/memory/memory_linux.go Outdated Show resolved Hide resolved
This patch wants to add memory informations on topology.Node,
so we can have per-NUMA-zone memory information.
In order to do so, we extract a `memory.Area` struct from `memory.Info`
to describe the basic properties of a memory zone, and
we add it to `topology.Node`.
In the future, we may want to add (or move) more fields
to this basic building block, and we will get them for free
in both system-wide and per-numa memory info.

Fixes: jaypipes#200
Signed-off-by: Francesco Romani <[email protected]>
We can use `ctx.Warn` instead of raw calls to Fprintf.
This patch does that in order to make the code base more regular.

Signed-off-by: Francesco Romani <[email protected]>
Copy link
Owner

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Great work, @fromanirh :) 👍

@jaypipes jaypipes merged commit c46f890 into jaypipes:main Nov 2, 2021
@ffromani
Copy link
Collaborator Author

ffromani commented Nov 2, 2021

Thanks!

@ffromani ffromani deleted the numa-memory-info branch November 2, 2021 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants