[LTP] [PATCH v2] mem/min_free_kbytes: Fix incorrect pass/fail accounting

[LTP] [PATCH v2] mem/min_free_kbytes: Fix incorrect pass/fail accounting

[Sachin Sant]

When check_monitor() detected a violation (MemFree < min_free_kbytes) it
called tst_res(TFAIL, ...) from the child process, which atomically
incremented the shared results->failed counter, and then the child exited
with status 0. Back in the parent, min_free_kbytes_test() checked the
child exit status, found it was 0, and fell through to the unconditional
tst_res(TPASS, ...) at the end of the function.

This produced a misleading summary of 'passed 1 / failed 1' on violation:
the TFAIL from the monitor child was correctly counted, but the
unconditional TPASS that followed also added to the pass count regardless
of the violation.

Fix this by moving TPASS/TFAIL reporting into check_monitor() in the child
process. Emit tst_res(TFAIL, ...) immediately on each individual violation
so that the TINFO diagnostic and its corresponding TFAIL are always paired;
the 'violated' flag is kept to suppress the final TPASS when any breach was
seen. Emit tst_res(TPASS, ...) only when no violation was observed across
the entire monitoring run.
The parent's wait-result block is changed to guard against unexpected
termination (signal death or non-zero exit) only; it emits no result for
the monitor outcome since that is already reported by the child. The
original condition WIFEXITED && WEXITSTATUS != 0 missed signal death;
the corrected condition is !WIFEXITED || WEXITSTATUS != 0.

After the fix the summary correctly reflects the outcome:
- No violation:  passed 1 / failed 0
- Violation:     passed 0 / failed N  (one TFAIL per breached sample)

Signed-off-by: Sachin Sant <sachinp@linux.ibm.com>
---
v1 -> v2:
- Addressed review comments by moving TPASS/TFAIL reporting
  into check_monitor() in the child process.

---
 testcases/kernel/mem/tunable/min_free_kbytes.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/testcases/kernel/mem/tunable/min_free_kbytes.c b/testcases/kernel/mem/tunable/min_free_kbytes.c
index bdc9126c2..132c8fe70 100644
--- a/testcases/kernel/mem/tunable/min_free_kbytes.c
+++ b/testcases/kernel/mem/tunable/min_free_kbytes.c
@@ -68,11 +68,9 @@ static void min_free_kbytes_test(void)
 	SAFE_KILL(pid, SIGUSR1);
 	SAFE_WAITPID(pid, &status, WUNTRACED | WCONTINUED);
 
-	if (WIFEXITED(status) && WEXITSTATUS(status) != 0)
-		tst_res(TFAIL, "check_monitor child exit with status: %s",
+	if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
+		tst_res(TFAIL, "check_monitor child failed: %s",
 			tst_strstatus(status));
-
-	tst_res(TPASS, "min_free_kbytes test pass");
 }
 
 static void test_tune(unsigned long overcommit_policy)
@@ -173,6 +171,7 @@ static int eatup_mem(unsigned long overcommit_policy)
 
 static void check_monitor(void)
 {
+	int violated = 0;
 	unsigned long tune;
 	unsigned long memfree;
 
@@ -182,12 +181,16 @@ static void check_monitor(void)
 
 		if (memfree < tune) {
 			tst_res(TINFO, "MemFree is %lu kB, "
-				 "min_free_kbytes is %lu kB", memfree, tune);
+				"min_free_kbytes is %lu kB", memfree, tune);
 			tst_res(TFAIL, "MemFree < min_free_kbytes");
+			violated = 1;
 		}
 
 		sleep(2);
 	}
+
+	if (!violated)
+		tst_res(TPASS, "min_free_kbytes test pass");
 }
 
 static void sighandler(int signo LTP_ATTRIBUTE_UNUSED)
-- 
2.39.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] mem/min_free_kbytes: Fix incorrect pass/fail accounting

[linuxtestproject.agent]

Hi Sachin,

On Thu Jul 9 09:43:23 2026 +0530, Sachin Sant wrote:
> mem/min_free_kbytes: Fix incorrect pass/fail accounting

Verdict - Reviewed

---
Pre-existing issues:

In check_monitor(), the flag variable used for signal-handler
communication is declared as:

  volatile int end;

Strict C (C99 and later) requires volatile sig_atomic_t for variables
read/written by a signal handler.  This is pre-existing and harmless on
common architectures, but worth fixing in a follow-up.

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] mem/min_free_kbytes: Fix incorrect pass/fail accounting

[Andrea Cervesato via ltp]

Hi Sachin,

I think with this patch we are working around the problem. Which is..
the test is badly written and it needs a complete refactoring, since
it seems like a mix of old and new API.

For instance, we could heavily simplify the test_tune(), adding a helper
for setting the min free kb, then using SAFE_FORK():

static void set_min_free_kbytes(int i)
{
	unsigned long memfree, memtotal, tune;

	switch (i) {
	case 0:
		tune = default_tune;
		break;
	case 1:
		tune = 2 * default_tune;
		break;
	default:
		memfree = SAFE_READ_MEMINFO("MemFree:");
		memtotal = SAFE_READ_MEMINFO("MemTotal:");
		tune = MIN(memfree / 20, memtotal / 50);
		break;
	}

	TST_SYS_CONF_LONG_SET(PATH_VM_MIN_FREE_KBYTES, tune, 1);
}

and then handling SAFE_WAITPID() only once for all.

Anyway..below there are some considerations about the patch:

> When check_monitor() detected a violation (MemFree < min_free_kbytes) it
> called tst_res(TFAIL, ...) from the child process, which atomically
> incremented the shared results->failed counter, and then the child exited
> with status 0. Back in the parent, min_free_kbytes_test() checked the
> child exit status, found it was 0, and fell through to the unconditional
> tst_res(TPASS, ...) at the end of the function.
> 
> This produced a misleading summary of 'passed 1 / failed 1' on violation:
> the TFAIL from the monitor child was correctly counted, but the
> unconditional TPASS that followed also added to the pass count regardless
> of the violation.
> 
> Fix this by moving TPASS/TFAIL reporting into check_monitor() in the child
> process. Emit tst_res(TFAIL, ...) immediately on each individual violation
> so that the TINFO diagnostic and its corresponding TFAIL are always paired;
> the 'violated' flag is kept to suppress the final TPASS when any breach was
> seen. Emit tst_res(TPASS, ...) only when no violation was observed across
> the entire monitoring run.
> The parent's wait-result block is changed to guard against unexpected
> termination (signal death or non-zero exit) only; it emits no result for
> the monitor outcome since that is already reported by the child. The
> original condition WIFEXITED && WEXITSTATUS != 0 missed signal death;
> the corrected condition is !WIFEXITED || WEXITSTATUS != 0.

Commit message is really verbose and a bit hard to follow without reading
the code. It's good practice to describe why patch has been created in a
short way so everyone will understand the patch by reading it at the very
first look.

> 
> After the fix the summary correctly reflects the outcome:
> - No violation:  passed 1 / failed 0
> - Violation:     passed 0 / failed N  (one TFAIL per breached sample)
> 
> Signed-off-by: Sachin Sant <sachinp@linux.ibm.com>
> ---
> v1 -> v2:
> - Addressed review comments by moving TPASS/TFAIL reporting
>   into check_monitor() in the child process.
> 
> ---
>  testcases/kernel/mem/tunable/min_free_kbytes.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/testcases/kernel/mem/tunable/min_free_kbytes.c b/testcases/kernel/mem/tunable/min_free_kbytes.c
> index bdc9126c2..132c8fe70 100644
> --- a/testcases/kernel/mem/tunable/min_free_kbytes.c
> +++ b/testcases/kernel/mem/tunable/min_free_kbytes.c
> @@ -68,11 +68,9 @@ static void min_free_kbytes_test(void)
>  	SAFE_KILL(pid, SIGUSR1);
>  	SAFE_WAITPID(pid, &status, WUNTRACED | WCONTINUED);
>  
> -	if (WIFEXITED(status) && WEXITSTATUS(status) != 0)
> -		tst_res(TFAIL, "check_monitor child exit with status: %s",
> +	if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
> +		tst_res(TFAIL, "check_monitor child failed: %s",
>  			tst_strstatus(status));
> -
> -	tst_res(TPASS, "min_free_kbytes test pass");

We can simply remove the whole check on the exit status, since LTP
already calls tst_reap_children().

>  }
>  
>  static void test_tune(unsigned long overcommit_policy)
> @@ -173,6 +171,7 @@ static int eatup_mem(unsigned long overcommit_policy)
>  
>  static void check_monitor(void)
>  {
> +	int violated = 0;
>  	unsigned long tune;
>  	unsigned long memfree;
>  
> @@ -182,12 +181,16 @@ static void check_monitor(void)
>  
>  		if (memfree < tune) {
>  			tst_res(TINFO, "MemFree is %lu kB, "
> -				 "min_free_kbytes is %lu kB", memfree, tune);
> +				"min_free_kbytes is %lu kB", memfree, tune);
>  			tst_res(TFAIL, "MemFree < min_free_kbytes");
> +			violated = 1;
>  		}
>  
>  		sleep(2);
>  	}
> +
> +	if (!violated)
> +		tst_res(TPASS, "min_free_kbytes test pass");

This is correct and good practice. tst_reap_children() will take care of it.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] mem/min_free_kbytes: Fix incorrect pass/fail accounting

[Sachin Sant]

On 09/07/26 10:39 pm, Andrea Cervesato wrote:
> Hi Sachin,
>
> I think with this patch we are working around the problem. Which is..
> the test is badly written and it needs a complete refactoring, since
> it seems like a mix of old and new API.

Hi Andrea,

Thank you for the detailed review.

You are right — the original patch was a workaround rather than a proper
fix.  The test was mixing old bare-fork patterns with the newer LTP API,
and patching around the symptom without addressing the structural problem
was the wrong approach.

I will rework the patch as a full refactor addressing all points raised:

1. Extract a set_min_free_kbytes() helper that consolidates the three
    tune-selection cases using a switch statement and MIN() as suggested,
    replacing the open-coded if/else if/else block in test_tune().

2. Replace all bare fork() calls with SAFE_FORK() throughout.  The
    eatup_mem children in test_tune() are still waited with SAFE_WAITPID
    and their exit status is still inspected per-policy, because
    overcommit_policy=1 expects SIGKILL — delegating those to
    tst_reap_children() would incorrectly produce TBROK on a valid
    outcome.

3. Drop the SAFE_WAITPID on the monitor child and its status check
    from min_free_kbytes_test() entirely.  The monitor child reports its
    own TPASS/TFAIL via tst_res() and always exits 0, so the framework's
    tst_reap_children() (called automatically after test_all returns)
    handles it correctly without any duplicate logic in the test.

4. Revise the commit message to be short and self-explanatory without
    re-narrating the diff.

I will send a v3 with the changes included.

-- 
Thanks
- Sachin



-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH] mem/min_free_kbytes: Fix incorrect pass/fail accounting

[Sachin Sant]

When check_monitor() detected a violation (MemFree < min_free_kbytes) it
called tst_res(TFAIL, ...) from the child process, which atomically
incremented the shared results->failed counter, and then the child exited
with status 0. Back in the parent, min_free_kbytes_test() checked the
child exit status, found it was 0, and fell through to the unconditional
tst_res(TPASS, ...) at the end of the function.

This produced a misleading summary of 'passed 1 / failed 1' on violation:
the TFAIL from the monitor child was correctly counted, but the
unconditional TPASS that followed also added to the pass count regardless
of the violation.

Fix this by:
- Removing the tst_res(TFAIL, ...) from check_monitor() and replacing it
  with a local 'violated' flag; the TINFO diagnostic is kept so the exact
  MemFree and tune values are still logged.
- Returning 'violated' from check_monitor() and passing it directly to
  exit(), so the child exits non-zero when a violation was detected.
- Restructuring the parent's wait-result check so that TPASS is only
  emitted when the monitor child exited normally with status 0; any other
  termination (signal death, unexpected non-zero exit) maps to TFAIL.
  A violation exit (status == 1) emits a semantically accurate message;
  all other abnormal exits fall back to tst_strstatus() for diagnostics.

After the fix the summary correctly reflects the outcome:
  - No violation:  passed 1 / failed 0
  - Violation:     passed 0 / failed 1

Signed-off-by: Sachin Sant <sachinp@linux.ibm.com>
---
 .../kernel/mem/tunable/min_free_kbytes.c      | 28 +++++++++++--------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/testcases/kernel/mem/tunable/min_free_kbytes.c b/testcases/kernel/mem/tunable/min_free_kbytes.c
index bdc9126c2..5bed61db5 100644
--- a/testcases/kernel/mem/tunable/min_free_kbytes.c
+++ b/testcases/kernel/mem/tunable/min_free_kbytes.c
@@ -41,7 +41,7 @@ static unsigned long total_mem;
 
 static void test_tune(unsigned long overcommit_policy);
 static int eatup_mem(unsigned long overcommit_policy);
-static void check_monitor(void);
+static int check_monitor(void);
 static void sighandler(int signo LTP_ATTRIBUTE_UNUSED);
 
 static void min_free_kbytes_test(void)
@@ -57,8 +57,7 @@ static void min_free_kbytes_test(void)
 	pid = SAFE_FORK();
 	if (pid == 0) {
 		/* startup the check monitor */
-		check_monitor();
-		exit(0);
+		exit(check_monitor());
 	}
 
 	test_tune(2);
@@ -68,11 +67,15 @@ static void min_free_kbytes_test(void)
 	SAFE_KILL(pid, SIGUSR1);
 	SAFE_WAITPID(pid, &status, WUNTRACED | WCONTINUED);
 
-	if (WIFEXITED(status) && WEXITSTATUS(status) != 0)
-		tst_res(TFAIL, "check_monitor child exit with status: %s",
-			tst_strstatus(status));
-
-	tst_res(TPASS, "min_free_kbytes test pass");
+	if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+		if (WIFEXITED(status) && WEXITSTATUS(status) == 1)
+			tst_res(TFAIL, "MemFree dropped below min_free_kbytes");
+		else
+			tst_res(TFAIL, "check_monitor child failed: %s",
+				tst_strstatus(status));
+	} else {
+		tst_res(TPASS, "min_free_kbytes test pass");
+	}
 }
 
 static void test_tune(unsigned long overcommit_policy)
@@ -171,8 +174,9 @@ static int eatup_mem(unsigned long overcommit_policy)
 	return ret;
 }
 
-static void check_monitor(void)
+static int check_monitor(void)
 {
+	int violated = 0;
 	unsigned long tune;
 	unsigned long memfree;
 
@@ -182,12 +186,14 @@ static void check_monitor(void)
 
 		if (memfree < tune) {
 			tst_res(TINFO, "MemFree is %lu kB, "
-				 "min_free_kbytes is %lu kB", memfree, tune);
-			tst_res(TFAIL, "MemFree < min_free_kbytes");
+				"min_free_kbytes is %lu kB", memfree, tune);
+			violated = 1;
 		}
 
 		sleep(2);
 	}
+
+	return violated;
 }
 
 static void sighandler(int signo LTP_ATTRIBUTE_UNUSED)
-- 
2.39.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] mem/min_free_kbytes: Fix incorrect pass/fail accounting

[linuxtestproject.agent]

Hi Sachin,

On Wed, 08 Jul 2026 18:12:05 +0530, Sachin Sant wrote:
> mem/min_free_kbytes: Fix incorrect pass/fail accounting

> @@ -57,8 +57,7 @@ static void min_free_kbytes_test(void)
>  	pid = SAFE_FORK();
>  	if (pid == 0) {
>  		/* startup the check monitor */
> -		check_monitor();
> -		exit(0);
> +		exit(check_monitor());
>  	}

> +	if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
> +		if (WIFEXITED(status) && WEXITSTATUS(status) == 1)
> +			tst_res(TFAIL, "MemFree dropped below min_free_kbytes");
> +		else
> +			tst_res(TFAIL, "check_monitor child failed: %s",
> +				tst_strstatus(status));
> +	} else {
> +		tst_res(TPASS, "min_free_kbytes test pass");
> +	}

The fix works in practice, but the design trades one problem for another:
encoding pass/fail as a child exit code and interpreting it in the parent
is explicitly the pattern LTP asks to avoid.

The test already sets .forks_child = 1, so the framework's shared-memory
result propagation is active.  tst_res() calls made inside check_monitor()
are automatically visible to the parent's result accounting -- no exit-code
relay needed.

> -static void check_monitor(void)
> +static int check_monitor(void)
>  {
> +	int violated = 0;

The 'violated' flag is fine for deciding when to emit the TINFO line, but
the TPASS/TFAIL themselves should be emitted by the child rather than
signalled through exit():

  if (violated)
      tst_res(TFAIL, "MemFree dropped below min_free_kbytes");
  else
      tst_res(TPASS, "min_free_kbytes test pass");

With that, check_monitor() returns void and the child exits 0 always.
The parent's wait block only needs to guard against truly unexpected
termination (signal, unknown non-zero exit), and must not re-emit a
TPASS/TFAIL for the violation outcome:

  SAFE_KILL(pid, SIGUSR1);
  SAFE_WAITPID(pid, &status, WUNTRACED | WCONTINUED);

  if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
      tst_res(TFAIL, "check_monitor child failed: %s",
          tst_strstatus(status));
  /* pass/fail already reported by the child */

This also solves the original double-accounting bug because the parent
no longer emits any result for the monitor outcome at all.

Verdict - Needs revision

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp