[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