Simulate numa nodes #4428
Simulate numa nodes #4428
Conversation
|
Skipping CI for Draft Pull Request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the capability to simulate NUMA nodes in QEMU for external tests. The changes involve adding a NumaNodes option and plumbing it through various layers of the test harness and platform configuration. The core logic for generating QEMU arguments for NUMA is in mantle/platform/qemu.go.
I've identified a couple of issues in the implementation within mantle/platform/qemu.go that could lead to incorrect resource allocation for the simulated NUMA nodes. My review includes suggestions to fix these bugs.
mantle/kola/register/register.go
Outdated
| Timeout time.Duration // the duration for which a test will be allowed to run | ||
| RequiredTag string // if specified, test is filtered by default unless tag is provided -- defaults to none | ||
| Description string // test description | ||
| NumaNodes int // Number of numa nodes to simulate |
There was a problem hiding this comment.
Should we make this an on/off flag where we hardcode 2 NUMA nodes as I don't think we'll test with more than that and maybe that should simplify the code a bit?
There was a problem hiding this comment.
I think this remove the for loop in baseNumaQemuArgs function, but most of the checks would still be quite similar.
Would it be useful to also allow the numaNodes flag with the cosa run command for debugging? If so, allowing an integer may be nice.
If making it a boolean would keep it simpler, and the above is not worth implementing, then I am happy to change it to use a boolean flag.
ravanelli
left a comment
ravanelli
left a comment
There was a problem hiding this comment.
Great job, it is looking good, let's squash the commits and follow the suggestion to simply it with using on/off with 2 NUMA nodes
mantle/kola/harness.go
Outdated
| Description string `json:"description" yaml:"description"` | ||
| } | ||
|
|
||
| // FIXME: This is not decoding the numa nodes flag properly... |
There was a problem hiding this comment.
Let's squash the commits, there are a lot of commits for the same feature and context, for example we have this one adding this comment and another one removing the same thing.
| } | ||
|
|
||
| builder.NumaNodes = options.NumaNodes | ||
|
|
mantle/platform/qemu.go
Outdated
| // Number of numa nodes to simulate, if 0 then no numa eumulation will take place. | ||
| // | ||
| // The machines memory and processors will be divided evenly between all the numa nodes. |
There was a problem hiding this comment.
| // Number of numa nodes to simulate, if 0 then no numa eumulation will take place. | |
| // | |
| // The machines memory and processors will be divided evenly between all the numa nodes. | |
| // Number of NUMA nodes to simulate (0 disables NUMA emulation). | |
| // Memory and CPUs are evenly split across nodes. |
mantle/platform/qemu.go
Outdated
| kvm = false | ||
| } | ||
| machineArg += "," + accel | ||
| func platformQemuArgs(arch, machineArg string, kvm bool) ([]string, error) { |
There was a problem hiding this comment.
You did a split in this function and also created baseNumaQemuArgs that shares a lot of the same code, let's try to merge the duplication
mantle/platform/qemu.go
Outdated
| memoryPerNode := memoryMiB / numaNodes | ||
| cpusPerNode := cpus / numaNodes | ||
|
|
||
| for nodeId := range numaNodes { |
There was a problem hiding this comment.
+1 for keeping it simple, with on/off with a fixed number of 2
angelcerveraroldan
force-pushed
the
numa-nodes-option
branch
from
52601f4 to
774b30c
Compare
|
/test all |
angelcerveraroldan
force-pushed
the
numa-nodes-option
branch
from
774b30c to
5590322
Compare
New boolean flag "numaNodes" will simulate two NUMA nodes. They will divide the memory and cpus between them.
angelcerveraroldan
force-pushed
the
numa-nodes-option
branch
from
5590322 to
aafd091
Compare
3 participants
Add an option for external tests to be executed by a machine with multiple numa nodes.