[PATCH 0/2] Fixes for compaction_test

[PATCH 0/2] Fixes for compaction_test

[Dev Jain]

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(-)

-- 
2.30.2

[PATCH 1/2] selftests/mm: compaction_test: Fix incorrect write of zero to nr_hugepages

[Dev Jain]

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",
-- 
2.30.2

Re: [PATCH 1/2] selftests/mm: compaction_test: Fix incorrect write of zero to nr_hugepages

[Andrew Morton]

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.

Re: [PATCH 1/2] selftests/mm: compaction_test: Fix incorrect write of zero to nr_hugepages

[Dev Jain]

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.

[PATCH 2/2] selftests/mm: compaction_test: Fix trivial test success and reduce probability of OOM-killer invocation

[Dev Jain]

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();
-- 
2.30.2

Re: [PATCH 2/2] selftests/mm: compaction_test: Fix trivial test success and reduce probability of OOM-killer invocation

[Andrew Morton]

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;
> +}

Re: [PATCH 2/2] selftests/mm: compaction_test: Fix trivial test success and reduce probability of OOM-killer invocation

[Dev Jain]

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)
>>   {
>>   	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?


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;
>> +}