The compaction_test memory selftest introduces fragmentation in memory and then tries to allocate as many hugepages as possible. This series addresses some problems.
First off, correctly set the number of hugepages to zero before trying to set a large number of them.
Now, consider a situation in which, at the start of the test, a non-zero number of hugepages have been already set (while running the entire selftests/mm suite, or manually by the admin). The test operates on 80% of memory to avoid OOM-killer invocation, and because some memory is already blocked by hugepages, it would increase the chance of OOM-killing. Also, since mem_free used in check_compaction() is the value before we set nr_hugepages to zero, the chance that the compaction_index will be small is very high if the preset nr_hugepages was high, leading to a bogus test success.
This series applies on top of the stable 6.9 kernel.
Dev Jain (2): selftests/mm: compaction_test: Fix incorrect write of zero to nr_hugepages selftests/mm: compaction_test: Fix trivial test success and reduce probability of OOM-killer invocation
tools/testing/selftests/mm/compaction_test.c | 70 ++++++++++++++------ 1 file changed, 50 insertions(+), 20 deletions(-)
nr_hugepages is not set to zero because the file offset has not been reset after read(). Fix that using lseek().
Fixes: bd67d5c15cc1 ("Test compaction of mlocked memory") Cc: stable@vger.kernel.org Signed-off-by: Dev Jain dev.jain@arm.com --- Merge dependency: https://lore.kernel.org/all/20240513082842.4117782-1-dev.jain@arm.com/ Andrew, does it sound reasonable to have the fixes tag in the above patch too, along with this series?
tools/testing/selftests/mm/compaction_test.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c index 533999b6c284..c5be395f8363 100644 --- a/tools/testing/selftests/mm/compaction_test.c +++ b/tools/testing/selftests/mm/compaction_test.c @@ -107,6 +107,8 @@ int check_compaction(unsigned long mem_free, unsigned int hugepage_size) goto close_fd; }
+ lseek(fd, 0, SEEK_SET); + /* Start with the initial condition of 0 huge pages*/ if (write(fd, "0", sizeof(char)) != sizeof(char)) { ksft_print_msg("Failed to write 0 to /proc/sys/vm/nr_hugepages: %s\n",
On Wed, 15 May 2024 15:06:32 +0530 Dev Jain dev.jain@arm.com wrote:
nr_hugepages is not set to zero because the file offset has not been reset after read(). Fix that using lseek().
Please fully describe the runtime effects of this bug.
On 5/20/24 05:30, Andrew Morton wrote:
On Wed, 15 May 2024 15:06:32 +0530 Dev Jain dev.jain@arm.com wrote:
nr_hugepages is not set to zero because the file offset has not been reset after read(). Fix that using lseek().
Please fully describe the runtime effects of this bug.
This is not a "bug", but a discrepancy; the following comment
by the author says "Start with the initial condition of 0 huge
pages", I am just ensuring that that is actually done. Although,
I am not sure about the utility of doing this in the first place,
since we are anyways trying to increase hugepages after that.
In the second patch, I have moved away this entire logic of
setting nr_hugepages to zero, to the place before we start
filling up memory; if you feel that this patch is unnecessary,
we may squash it.
Reset nr_hugepages to zero before the start of the test.
If a non-zero number of hugepages is already set before the start of the test, the following problems arise:
- The probability of the test getting OOM-killed increases. Proof: The test wants to run on 80% of available memory to prevent OOM-killing (see original code comments). Let the value of mem_free at the start of the test, when nr_hugepages = 0, be x. In the other case, when nr_hugepages > 0, let the memory consumed by hugepages be y. In the former case, the test operates on 0.8 * x of memory. In the latter, the test operates on 0.8 * (x - y) of memory, with y already filled, hence, memory consumed is y + 0.8 * (x - y) = 0.8 * x + 0.2 * y > 0.8 * x. Q.E.D
- The probability of a bogus test success increases. Proof: Let the memory consumed by hugepages be greater than 25% of x, with x and y defined as above. The definition of compaction_index is c_index = (x - y)/z where z is the memory consumed by hugepages after trying to increase them again. In check_compaction(), we set the number of hugepages to zero, and then increase them back; the probability that they will be set back to consume at least y amount of memory again is very high (since there is not much delay between the two attempts of changing nr_hugepages). Hence, z >= y > (x/4) (by the 25% assumption). Therefore, c_index = (x - y)/z <= (x - y)/y = x/y - 1 < 4 - 1 = 3 hence, c_index can always be forced to be less than 3, thereby the test succeeding always. Q.E.D
NOTE: This patch depends on the previous one.
Fixes: bd67d5c15cc1 ("Test compaction of mlocked memory") Cc: stable@vger.kernel.org Signed-off-by: Dev Jain dev.jain@arm.com --- tools/testing/selftests/mm/compaction_test.c | 72 ++++++++++++++------ 1 file changed, 50 insertions(+), 22 deletions(-)
diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c index c5be395f8363..2ae059989771 100644 --- a/tools/testing/selftests/mm/compaction_test.c +++ b/tools/testing/selftests/mm/compaction_test.c @@ -82,12 +82,15 @@ int prereq(void) return -1; }
-int check_compaction(unsigned long mem_free, unsigned int hugepage_size) +int check_compaction(unsigned long mem_free, unsigned int hugepage_size, + int initial_nr_hugepages) { int fd, ret = -1; int compaction_index = 0; - char initial_nr_hugepages[10] = {0}; char nr_hugepages[10] = {0}; + char init_nr_hugepages[10] = {0}; + + sprintf(init_nr_hugepages, "%d", initial_nr_hugepages);
/* We want to test with 80% of available memory. Else, OOM killer comes in to play */ @@ -101,23 +104,6 @@ int check_compaction(unsigned long mem_free, unsigned int hugepage_size) goto out; }
- if (read(fd, initial_nr_hugepages, sizeof(initial_nr_hugepages)) <= 0) { - ksft_print_msg("Failed to read from /proc/sys/vm/nr_hugepages: %s\n", - strerror(errno)); - goto close_fd; - } - - lseek(fd, 0, SEEK_SET); - - /* Start with the initial condition of 0 huge pages*/ - if (write(fd, "0", sizeof(char)) != sizeof(char)) { - ksft_print_msg("Failed to write 0 to /proc/sys/vm/nr_hugepages: %s\n", - strerror(errno)); - goto close_fd; - } - - lseek(fd, 0, SEEK_SET); - /* Request a large number of huge pages. The Kernel will allocate as much as it can */ if (write(fd, "100000", (6*sizeof(char))) != (6*sizeof(char))) { @@ -140,8 +126,8 @@ int check_compaction(unsigned long mem_free, unsigned int hugepage_size)
lseek(fd, 0, SEEK_SET);
- if (write(fd, initial_nr_hugepages, strlen(initial_nr_hugepages)) - != strlen(initial_nr_hugepages)) { + if (write(fd, init_nr_hugepages, strlen(init_nr_hugepages)) + != strlen(init_nr_hugepages)) { ksft_print_msg("Failed to write value to /proc/sys/vm/nr_hugepages: %s\n", strerror(errno)); goto close_fd; @@ -165,6 +151,42 @@ int check_compaction(unsigned long mem_free, unsigned int hugepage_size) return ret; }
+int set_zero_hugepages(int *initial_nr_hugepages) +{ + int fd, ret = -1; + char nr_hugepages[10] = {0}; + + fd = open("/proc/sys/vm/nr_hugepages", O_RDWR | O_NONBLOCK); + if (fd < 0) { + ksft_print_msg("Failed to open /proc/sys/vm/nr_hugepages: %s\n", + strerror(errno)); + goto out; + } + + if (read(fd, nr_hugepages, sizeof(nr_hugepages)) <= 0) { + ksft_print_msg("Failed to read from /proc/sys/vm/nr_hugepages: %s\n", + strerror(errno)); + goto close_fd; + } + + lseek(fd, 0, SEEK_SET); + + /* Start with the initial condition of 0 huge pages */ + if (write(fd, "0", sizeof(char)) != sizeof(char)) { + ksft_print_msg("Failed to write 0 to /proc/sys/vm/nr_hugepages: %s\n", + strerror(errno)); + goto close_fd; + } + + *initial_nr_hugepages = atoi(nr_hugepages); + ret = 0; + + close_fd: + close(fd); + + out: + return ret; +}
int main(int argc, char **argv) { @@ -175,6 +197,7 @@ int main(int argc, char **argv) unsigned long mem_free = 0; unsigned long hugepage_size = 0; long mem_fragmentable_MB = 0; + int initial_nr_hugepages;
ksft_print_header();
@@ -183,6 +206,10 @@ int main(int argc, char **argv)
ksft_set_plan(1);
+ /* start the test without hugepages reducing mem_free */ + if (set_zero_hugepages(&initial_nr_hugepages)) + return ksft_exit_fail(); + lim.rlim_cur = RLIM_INFINITY; lim.rlim_max = RLIM_INFINITY; if (setrlimit(RLIMIT_MEMLOCK, &lim)) @@ -226,7 +253,8 @@ int main(int argc, char **argv) entry = entry->next; }
- if (check_compaction(mem_free, hugepage_size) == 0) + if (check_compaction(mem_free, hugepage_size, + initial_nr_hugepages) == 0) return ksft_exit_pass();
return ksft_exit_fail();
On Wed, 15 May 2024 15:06:33 +0530 Dev Jain dev.jain@arm.com wrote:
Reset nr_hugepages to zero before the start of the test.
If a non-zero number of hugepages is already set before the start of the test, the following problems arise:
- The probability of the test getting OOM-killed increases.
Proof: The test wants to run on 80% of available memory to prevent OOM-killing (see original code comments). Let the value of mem_free at the start of the test, when nr_hugepages = 0, be x. In the other case, when nr_hugepages > 0, let the memory consumed by hugepages be y. In the former case, the test operates on 0.8 * x of memory. In the latter, the test operates on 0.8 * (x - y) of memory, with y already filled, hence, memory consumed is y + 0.8 * (x - y) = 0.8 * x + 0.2 * y > 0.8 * x. Q.E.D
- The probability of a bogus test success increases.
Proof: Let the memory consumed by hugepages be greater than 25% of x, with x and y defined as above. The definition of compaction_index is c_index = (x - y)/z where z is the memory consumed by hugepages after trying to increase them again. In check_compaction(), we set the number of hugepages to zero, and then increase them back; the probability that they will be set back to consume at least y amount of memory again is very high (since there is not much delay between the two attempts of changing nr_hugepages). Hence, z >= y > (x/4) (by the 25% assumption). Therefore, c_index = (x - y)/z <= (x - y)/y = x/y - 1 < 4 - 1 = 3 hence, c_index can always be forced to be less than 3, thereby the test succeeding always. Q.E.D
NOTE: This patch depends on the previous one.
-int check_compaction(unsigned long mem_free, unsigned int hugepage_size) +int check_compaction(unsigned long mem_free, unsigned int hugepage_size,
int initial_nr_hugepages){ int fd, ret = -1; int compaction_index = 0;
- char initial_nr_hugepages[10] = {0}; char nr_hugepages[10] = {0};
- char init_nr_hugepages[10] = {0};
- sprintf(init_nr_hugepages, "%d", initial_nr_hugepages);
Well, [10] isn't really large enough. "-1111111111" requires 12 chars, with the trailing \0. And I'd suggest an unsigned type and a %u - negative initial_nr_hugepages doesn't make a lot of sense.
+int set_zero_hugepages(int *initial_nr_hugepages) +{
- int fd, ret = -1;
- char nr_hugepages[10] = {0};
Ditto?
- fd = open("/proc/sys/vm/nr_hugepages", O_RDWR | O_NONBLOCK);
- if (fd < 0) {
ksft_print_msg("Failed to open /proc/sys/vm/nr_hugepages: %s\n",strerror(errno));goto out;- }
- if (read(fd, nr_hugepages, sizeof(nr_hugepages)) <= 0) {
ksft_print_msg("Failed to read from /proc/sys/vm/nr_hugepages: %s\n",strerror(errno));goto close_fd;- }
- lseek(fd, 0, SEEK_SET);
- /* Start with the initial condition of 0 huge pages */
- if (write(fd, "0", sizeof(char)) != sizeof(char)) {
ksft_print_msg("Failed to write 0 to /proc/sys/vm/nr_hugepages: %s\n",strerror(errno));goto close_fd;- }
- *initial_nr_hugepages = atoi(nr_hugepages);
- ret = 0;
- close_fd:
- close(fd);
- out:
- return ret;
+}
On 5/20/24 05:33, Andrew Morton wrote:
On Wed, 15 May 2024 15:06:33 +0530 Dev Jain dev.jain@arm.com wrote:
Reset nr_hugepages to zero before the start of the test.
If a non-zero number of hugepages is already set before the start of the test, the following problems arise:
- The probability of the test getting OOM-killed increases.
Proof: The test wants to run on 80% of available memory to prevent OOM-killing (see original code comments). Let the value of mem_free at the start of the test, when nr_hugepages = 0, be x. In the other case, when nr_hugepages > 0, let the memory consumed by hugepages be y. In the former case, the test operates on 0.8 * x of memory. In the latter, the test operates on 0.8 * (x - y) of memory, with y already filled, hence, memory consumed is y + 0.8 * (x - y) = 0.8 * x + 0.2 * y > 0.8 * x. Q.E.D
- The probability of a bogus test success increases.
Proof: Let the memory consumed by hugepages be greater than 25% of x, with x and y defined as above. The definition of compaction_index is c_index = (x - y)/z where z is the memory consumed by hugepages after trying to increase them again. In check_compaction(), we set the number of hugepages to zero, and then increase them back; the probability that they will be set back to consume at least y amount of memory again is very high (since there is not much delay between the two attempts of changing nr_hugepages). Hence, z >= y > (x/4) (by the 25% assumption). Therefore, c_index = (x - y)/z <= (x - y)/y = x/y - 1 < 4 - 1 = 3 hence, c_index can always be forced to be less than 3, thereby the test succeeding always. Q.E.D
NOTE: This patch depends on the previous one.
-int check_compaction(unsigned long mem_free, unsigned int hugepage_size) +int check_compaction(unsigned long mem_free, unsigned int hugepage_size,
int initial_nr_hugepages)
- char initial_nr_hugepages[10] = {0}; char nr_hugepages[10] = {0};
- char init_nr_hugepages[10] = {0};
- sprintf(init_nr_hugepages, "%d", initial_nr_hugepages);
Well, [10] isn't really large enough. "-1111111111" requires 12 chars, with the trailing \0. And I'd suggest an unsigned type and a %u - negative initial_nr_hugepages doesn't make a lot of sense.
+int set_zero_hugepages(int *initial_nr_hugepages) +{
- int fd, ret = -1;
- char nr_hugepages[10] = {0};
Ditto?
Sure, makes sense. I'll just change that to 20 and make it unsigned.
- fd = open("/proc/sys/vm/nr_hugepages", O_RDWR | O_NONBLOCK);
- if (fd < 0) {
ksft_print_msg("Failed to open /proc/sys/vm/nr_hugepages: %s\n",strerror(errno));goto out;- }
- if (read(fd, nr_hugepages, sizeof(nr_hugepages)) <= 0) {
ksft_print_msg("Failed to read from /proc/sys/vm/nr_hugepages: %s\n",strerror(errno));goto close_fd;- }
- lseek(fd, 0, SEEK_SET);
- /* Start with the initial condition of 0 huge pages */
- if (write(fd, "0", sizeof(char)) != sizeof(char)) {
ksft_print_msg("Failed to write 0 to /proc/sys/vm/nr_hugepages: %s\n",strerror(errno));goto close_fd;- }
- *initial_nr_hugepages = atoi(nr_hugepages);
- ret = 0;
- close_fd:
- close(fd);
- out:
- return ret;
+}
linux-kselftest-mirror@lists.linaro.org
-
Andrew Morton -
Dev Jain