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

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 <[email protected]>
> ---
> 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-IBi9RG/[email protected]

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