Add OS provider tests for different bind modes #241
Conversation
DamianDuy
force-pushed
the
addTestsBindModes
branch
from
bbc3944 to
fac52b7
Compare
|
I don't know if this PR is supposed to cover entire #242 so feel free to address my comment in another PR: We should check that policies like 'preferred' actually fall back to other nodes in case there is not enough memory on the specified node. Also, for modes that should fail if there is not enough memory we should test they actually do fail. One way to do that would be to add some kind of env variable which, if set, we could use to specify NUMA node on which we want to allocate (and use it for all binding tests). Then, we could run those tests in an environment with some custom NUMA nodes (e.g. we could create a very small NUMA node on pmem to make sure the oom is triggered quickly) |
DamianDuy
force-pushed
the
addTestsBindModes
branch
3 times, most recently
from
e6201b5 to
6281cf1
Compare
DamianDuy
force-pushed
the
addTestsBindModes
branch
4 times, most recently
from
c234727 to
e63254b
Compare
DamianDuy
force-pushed
the
addTestsBindModes
branch
7 times, most recently
from
b098491 to
4596575
Compare
|
@igchor @PatKamin @kswiecicki please review. I think the rest tests required for #242 would be added in separate PRs |
| ASSERT_EQ(umf_result, UMF_RESULT_SUCCESS); | ||
| ASSERT_NE(ptr, nullptr); | ||
|
|
||
| int cpu = sched_getcpu(); |
There was a problem hiding this comment.
I think that this test shouldn't be run on a randomized cpu. We don't have a guarantee that this test will be run on different numa nodes.
Maybe you could try parameterizing this test with all available cpus on which the process can be ran and using sched_setaffinity() or sth similar to run this test on each of available cpus. This way this test will cover all possible numa nodes with cpus in a multi-numa-node server.
| // Test for allocations on numa nodes with interleave mode enabled. | ||
| // The page allocations are interleaved across the set of nodes specified in nodemask. | ||
| TEST_F(testNuma, checkModeInterleave) { | ||
| int num_page = 1024; |
There was a problem hiding this comment.
nit: Make it a constexpr. Also, pages_num (a plural form) or sth like that would better describe what this var is for.
| @@ -28,7 +30,17 @@ std::vector<int> get_available_numa_nodes_numbers() { | |||
| return available_numa_nodes_numbers; | |||
There was a problem hiding this comment.
This test is all about multiple numa nodes. I think there should be a check if the size of available numa nodes vector is greater than 1. Otherwise, we might think that we test umf on multiple numa nodes but actually testing just a single numa node.
| ::testing::ValuesIn(get_available_numa_nodes_numbers())); | ||
|
|
||
| // Test for allocations on numa nodes. This test will be executed for all numa nodes | ||
| // available on the system. The available nodes are returned in vector from the | ||
| // get_available_numa_nodes_numbers() function and passed to test as parameters. | ||
| TEST_P(testNumaNodes, checkNumaNodesAllocations) { | ||
| TEST_P(testNumaOnAllNodes, checkNumaNodesAllocations) { |
There was a problem hiding this comment.
I think it would be good to have a test case for BIND where you set multiple NUMA nodes in node mask. This is allowed according to the documentation.
|
|
||
| // Test for allocations on numa nodes with interleave mode enabled. | ||
| // The page allocations are interleaved across the set of nodes specified in nodemask. | ||
| TEST_F(testNuma, checkModeInterleave) { |
There was a problem hiding this comment.
Please also add a test where nodemask does not contain all NUMA nodes (e.g. a test with just one node).
DamianDuy
force-pushed
the
addTestsBindModes
branch
from
4596575 to
9e8e41c
Compare
DamianDuy
force-pushed
the
addTestsBindModes
branch
from
9e8e41c to
a9a953f
Compare
No description provided.