[LTP] [PATCH 0/3] Fix ADSP074 timeouts

[LTP] [PATCH 0/3] Fix ADSP074 timeouts

[Martin Doucha]

ADSP074 runs dio_sparse with extreme number of children which can result
in severe system overload and random test timeouts simply because
the main test process gets blocked by scheduler for 30+ seconds. Make
io_read() aware of max_runtime and redesign child failure detection to fix
the timeouts.

Also fixes a bug in io_read() which could have caused false negatives.

Martin Doucha (3):
  Add tst_validate_children() helper function
  Make io_read() runtime-aware
  dio_sparse: Fix child exit code

 include/tst_test.h                            |  8 ++++
 lib/tst_res.c                                 | 37 +++++++++++++++++++
 .../kernel/io/ltp-aiodio/aiodio_sparse.c      |  9 ++---
 testcases/kernel/io/ltp-aiodio/common.h       |  6 +--
 testcases/kernel/io/ltp-aiodio/dio_sparse.c   |  8 +---
 5 files changed, 53 insertions(+), 15 deletions(-)

-- 
2.37.2


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

[LTP] [PATCH 1/3] Add tst_validate_children() helper function

[Martin Doucha]

The function waits for given number of child processes and validates
that they have all exited without error.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 include/tst_test.h |  8 ++++++++
 lib/tst_res.c      | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/tst_test.h b/include/tst_test.h
index ac52f268c..69e649651 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -362,6 +362,14 @@ void tst_set_max_runtime(int max_runtime);
  */
 char *tst_get_tmpdir(void);
 
+/*
+ * Validates exit status of child processes
+ */
+int tst_validate_children_(const char *file, const int lineno,
+	unsigned int count);
+#define tst_validate_children(child_count) \
+	tst_validate_children_(__FILE__, __LINE__, (child_count))
+
 #ifndef TST_NO_DEFAULT_MAIN
 
 static struct tst_test test;
diff --git a/lib/tst_res.c b/lib/tst_res.c
index e0896eb05..cac7484e7 100644
--- a/lib/tst_res.c
+++ b/lib/tst_res.c
@@ -613,3 +613,40 @@ void tst_require_root(void)
 	if (geteuid() != 0)
 		tst_brkm(TCONF, NULL, "Test needs to be run as root");
 }
+
+int tst_validate_children_(const char *file, const int lineno,
+	unsigned int count)
+{
+	unsigned int i;
+	int status;
+
+	for (i = 0; i < count; i++) {
+		SAFE_WAITPID(NULL, -1, &status, 0);
+
+		if (WIFSIGNALED(status)) {
+			tst_res_(file, lineno, TFAIL,
+				"Child killed by signal %d %s",
+				WTERMSIG(status), tst_strsig(WTERMSIG(status)));
+			return 1;
+		}
+
+		if (WCOREDUMP(status)) {
+			tst_res_(file, lineno, TFAIL, "Child crashed");
+			return 1;
+		}
+
+		if (!WIFEXITED(status)) {
+			tst_res_(file, lineno, TFAIL,
+				"Unexpected child status");
+			return 1;
+		}
+
+		if (WEXITSTATUS(status)) {
+			tst_res_(file, lineno, TFAIL, "Child returned error %d",
+				WEXITSTATUS(status));
+			return 1;
+		}
+	}
+
+	return 0;
+}
-- 
2.37.2


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

Re: [LTP] [PATCH 1/3] Add tst_validate_children() helper function

[Cyril Hrubis]

Hi!
> The function waits for given number of child processes and validates
> that they have all exited without error.
> 
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  include/tst_test.h |  8 ++++++++
>  lib/tst_res.c      | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/include/tst_test.h b/include/tst_test.h
> index ac52f268c..69e649651 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -362,6 +362,14 @@ void tst_set_max_runtime(int max_runtime);
>   */
>  char *tst_get_tmpdir(void);
>  
> +/*
> + * Validates exit status of child processes
> + */
> +int tst_validate_children_(const char *file, const int lineno,
> +	unsigned int count);
> +#define tst_validate_children(child_count) \
> +	tst_validate_children_(__FILE__, __LINE__, (child_count))
> +
>  #ifndef TST_NO_DEFAULT_MAIN
>  
>  static struct tst_test test;
> diff --git a/lib/tst_res.c b/lib/tst_res.c
> index e0896eb05..cac7484e7 100644
> --- a/lib/tst_res.c
> +++ b/lib/tst_res.c
> @@ -613,3 +613,40 @@ void tst_require_root(void)
>  	if (geteuid() != 0)
>  		tst_brkm(TCONF, NULL, "Test needs to be run as root");
>  }
> +
> +int tst_validate_children_(const char *file, const int lineno,
> +	unsigned int count)
> +{
> +	unsigned int i;
> +	int status;
> +
> +	for (i = 0; i < count; i++) {
> +		SAFE_WAITPID(NULL, -1, &status, 0);
> +
> +		if (WIFSIGNALED(status)) {
> +			tst_res_(file, lineno, TFAIL,
> +				"Child killed by signal %d %s",
> +				WTERMSIG(status), tst_strsig(WTERMSIG(status)));
> +			return 1;
> +		}
> +
> +		if (WCOREDUMP(status)) {
> +			tst_res_(file, lineno, TFAIL, "Child crashed");
> +			return 1;
> +		}

WCOREDUMP() is subset of WIFSIGNALED() and as such it will be never
triggered here.

> +		if (!WIFEXITED(status)) {
> +			tst_res_(file, lineno, TFAIL,
> +				"Unexpected child status");
> +			return 1;
> +		}
> +
> +		if (WEXITSTATUS(status)) {
> +			tst_res_(file, lineno, TFAIL, "Child returned error %d",
> +				WEXITSTATUS(status));
> +			return 1;
> +		}

And we do have tst_strstatus() that actually does all of this so why not
just:

	SAFE_WAITPID(NULL, -1, &status, 0);

	if (!WIFEXITED(status) || WEXITSTATUS(status)) {
		tst_res_(file, lineno, TFAIL, "Child %s", tst_strstatus(status);
		return 1;
	}

-- 
Cyril Hrubis
chrubis@suse.cz

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

[LTP] [PATCH 2/3] Make io_read() runtime-aware

[Martin Doucha]

Running dio_sparse with too many children can cause test timeouts
due to severe system overload. Make the children runtime aware and
switch to exit code validation.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 testcases/kernel/io/ltp-aiodio/aiodio_sparse.c | 9 +++------
 testcases/kernel/io/ltp-aiodio/common.h        | 2 +-
 testcases/kernel/io/ltp-aiodio/dio_sparse.c    | 8 ++------
 3 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c b/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c
index 88aec7952..595c76226 100644
--- a/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c
+++ b/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c
@@ -188,7 +188,6 @@ static void cleanup(void)
 static void run(void)
 {
 	char *filename = "file.bin";
-	int status;
 	int i, pid;
 
 	*run_child = 1;
@@ -222,12 +221,10 @@ static void run(void)
 		}
 	}
 
-	if (SAFE_WAITPID(-1, &status, WNOHANG))
-		tst_res(TFAIL, "Non zero bytes read");
-	else
-		tst_res(TPASS, "All bytes read were zeroed");
-
 	*run_child = 0;
+
+	if (!tst_validate_children(numchildren))
+		tst_res(TPASS, "All bytes read were zeroed");
 }
 
 static struct tst_test test = {
diff --git a/testcases/kernel/io/ltp-aiodio/common.h b/testcases/kernel/io/ltp-aiodio/common.h
index d9cbd8611..68465dc54 100644
--- a/testcases/kernel/io/ltp-aiodio/common.h
+++ b/testcases/kernel/io/ltp-aiodio/common.h
@@ -85,7 +85,7 @@ static inline void io_read(const char *filename, int filesize, volatile int *run
 				offset += r;
 			}
 
-			if (!*run_child)
+			if (!*run_child || !tst_remaining_runtime())
 				goto exit;
 		}
 	}
diff --git a/testcases/kernel/io/ltp-aiodio/dio_sparse.c b/testcases/kernel/io/ltp-aiodio/dio_sparse.c
index b08d2ea1e..1b5834ed4 100644
--- a/testcases/kernel/io/ltp-aiodio/dio_sparse.c
+++ b/testcases/kernel/io/ltp-aiodio/dio_sparse.c
@@ -96,7 +96,6 @@ static void cleanup(void)
 static void run(void)
 {
 	char *filename = "dio_sparse";
-	int status;
 	int fd;
 	int i;
 
@@ -113,13 +112,10 @@ static void run(void)
 	}
 
 	dio_sparse(fd, alignment, filesize, writesize, offset);
+	*run_child = 0;
 
-	if (SAFE_WAITPID(-1, &status, WNOHANG))
-		tst_res(TFAIL, "Non zero bytes read");
-	else
+	if (!tst_validate_children(numchildren))
 		tst_res(TPASS, "All bytes read were zeroed");
-
-	*run_child = 0;
 }
 
 static struct tst_test test = {
-- 
2.37.2


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

Re: [LTP] [PATCH 2/3] Make io_read() runtime-aware

[Cyril Hrubis]

Hi!
> diff --git a/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c b/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c
> index 88aec7952..595c76226 100644
> --- a/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c
> +++ b/testcases/kernel/io/ltp-aiodio/aiodio_sparse.c
> @@ -188,7 +188,6 @@ static void cleanup(void)
>  static void run(void)
>  {
>  	char *filename = "file.bin";
> -	int status;
>  	int i, pid;
>  
>  	*run_child = 1;
> @@ -222,12 +221,10 @@ static void run(void)
>  		}
>  	}
>  
> -	if (SAFE_WAITPID(-1, &status, WNOHANG))
> -		tst_res(TFAIL, "Non zero bytes read");
> -	else
> -		tst_res(TPASS, "All bytes read were zeroed");
> -
>  	*run_child = 0;
> +
> +	if (!tst_validate_children(numchildren))
> +		tst_res(TPASS, "All bytes read were zeroed");

This actually breaks the test, have a look at the io_read() in common.h.
The code is written so that the child exits with zero if we find
non-zero bytes so the only way how to actually report a failure is to
check if any of the children did exit before we set the run_child to
zero.

Looking at the code closer the break actually breaks only the inner
for () loop, so it looks like the code was broken and is stil broken. I
guess that we should do return instead of break as well.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[LTP] [PATCH 3/3] dio_sparse: Fix child exit code

[Martin Doucha]

dio_sparse currently ignores all child failures because children never
exit with non-zero code. Fix child exit status.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 testcases/kernel/io/ltp-aiodio/common.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/io/ltp-aiodio/common.h b/testcases/kernel/io/ltp-aiodio/common.h
index 68465dc54..6265831af 100644
--- a/testcases/kernel/io/ltp-aiodio/common.h
+++ b/testcases/kernel/io/ltp-aiodio/common.h
@@ -78,9 +78,9 @@ static inline void io_read(const char *filename, int filesize, volatile int *run
 			if (r > 0) {
 				bufoff = check_zero(buff, r);
 				if (bufoff) {
-					tst_res(TINFO, "non-zero read at offset %zu",
+					tst_brk(TBROK,
+						"non-zero read at offset %zu",
 						offset + (bufoff - buff));
-					break;
 				}
 				offset += r;
 			}
-- 
2.37.2


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

Re: [LTP] [PATCH 3/3] dio_sparse: Fix child exit code

[Cyril Hrubis]

Hi!
> dio_sparse currently ignores all child failures because children never
> exit with non-zero code. Fix child exit status.
> 
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  testcases/kernel/io/ltp-aiodio/common.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/testcases/kernel/io/ltp-aiodio/common.h b/testcases/kernel/io/ltp-aiodio/common.h
> index 68465dc54..6265831af 100644
> --- a/testcases/kernel/io/ltp-aiodio/common.h
> +++ b/testcases/kernel/io/ltp-aiodio/common.h
> @@ -78,9 +78,9 @@ static inline void io_read(const char *filename, int filesize, volatile int *run
>  			if (r > 0) {
>  				bufoff = check_zero(buff, r);
>  				if (bufoff) {
> -					tst_res(TINFO, "non-zero read at offset %zu",
> +					tst_brk(TBROK,
> +						"non-zero read at offset %zu",
>  						offset + (bufoff - buff));

Ah, this is the fix. I would go for tst_res(TFAIL, ""); and return 1;
otherwise this looks fine applied over the previous changes.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 3/3] dio_sparse: Fix child exit code

[Martin Doucha]

On 14. 09. 22 11:37, Cyril Hrubis wrote:
> Hi!
>> dio_sparse currently ignores all child failures because children never
>> exit with non-zero code. Fix child exit status.
>>
>> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
>> ---
>>   testcases/kernel/io/ltp-aiodio/common.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/testcases/kernel/io/ltp-aiodio/common.h b/testcases/kernel/io/ltp-aiodio/common.h
>> index 68465dc54..6265831af 100644
>> --- a/testcases/kernel/io/ltp-aiodio/common.h
>> +++ b/testcases/kernel/io/ltp-aiodio/common.h
>> @@ -78,9 +78,9 @@ static inline void io_read(const char *filename, int filesize, volatile int *run
>>   			if (r > 0) {
>>   				bufoff = check_zero(buff, r);
>>   				if (bufoff) {
>> -					tst_res(TINFO, "non-zero read at offset %zu",
>> +					tst_brk(TBROK,
>> +						"non-zero read at offset %zu",
>>   						offset + (bufoff - buff));
> 
> Ah, this is the fix. I would go for tst_res(TFAIL, ""); and return 1;
> otherwise this looks fine applied over the previous changes.

If I returned from io_read(), I'd have to rewrite the calls in 
dio_sparse.c and aiodio_sparse.c to exit(io_read()). Otherwise testrun() 
in LTP library would always force the exit code to 0. This is less work 
and you won't need to remember LTP library implementation details when 
you reuse io_read() in a new test.

Should I send a v2 for tst_validate_children() or will you delete the 
if(WCOREDUMP()) branch and merge it now?

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH 3/3] dio_sparse: Fix child exit code

[Cyril Hrubis]

Hi!
> > Ah, this is the fix. I would go for tst_res(TFAIL, ""); and return 1;
> > otherwise this looks fine applied over the previous changes.
> 
> If I returned from io_read(), I'd have to rewrite the calls in 
> dio_sparse.c and aiodio_sparse.c to exit(io_read()). Otherwise testrun() 
> in LTP library would always force the exit code to 0. This is less work 
> and you won't need to remember LTP library implementation details when 
> you reuse io_read() in a new test.

What about tst_res(TFAIL, ""); followed by exit(1). Really this is a
case where the test does fail we and we should report failure properly.
Or even just exit(1) as we do check the exit value after your changes.

> Should I send a v2 for tst_validate_children() or will you delete the 
> if(WCOREDUMP()) branch and merge it now?

Is there a good reason why you are trying to avoid tst_strstatus() that
simplifies the whole inner body of the loop to a single if?

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 3/3] dio_sparse: Fix child exit code

[Martin Doucha]

On 14. 09. 22 11:49, Cyril Hrubis wrote:
> Hi!
>>> Ah, this is the fix. I would go for tst_res(TFAIL, ""); and return 1;
>>> otherwise this looks fine applied over the previous changes.
>>
>> If I returned from io_read(), I'd have to rewrite the calls in
>> dio_sparse.c and aiodio_sparse.c to exit(io_read()). Otherwise testrun()
>> in LTP library would always force the exit code to 0. This is less work
>> and you won't need to remember LTP library implementation details when
>> you reuse io_read() in a new test.
> 
> What about tst_res(TFAIL, ""); followed by exit(1). Really this is a
> case where the test does fail we and we should report failure properly.
> Or even just exit(1) as we do check the exit value after your changes.
> 
>> Should I send a v2 for tst_validate_children() or will you delete the
>> if(WCOREDUMP()) branch and merge it now?
> 
> Is there a good reason why you are trying to avoid tst_strstatus() that
> simplifies the whole inner body of the loop to a single if?

OK, I'll send v2.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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