Re: [LTP] [PATCH v14] thermal: add new test group

[Petr Vorel <pvorel@suse.cz>]

Hi Piotr,

Minor notes:

> +static void setup(void)
> +{
> +	char line[8192];
> +
> +	nproc = tst_ncpus();
> +	tst_res(TDEBUG, "Number of logical cores: %d", nproc);
> +	interrupt_init = calloc(nproc, sizeof(uint64_t));
> +	interrupt_later = calloc(nproc, sizeof(uint64_t));
You correctly had SAFE_CALLOC() in v13, here you accident start using calloc()
again.

> +
> +	DIR *dir = SAFE_OPENDIR("/sys/class/thermal/");
> +	struct dirent *entry;
> +
> +	while ((entry = SAFE_READDIR(dir))) {
> +		if ((!strncmp(entry->d_name, "thermal_zone", sizeof("thermal_zone") - 1)))
> +			tz_counter++;
> +	}
> +	SAFE_CLOSEDIR(dir);
> +	tst_res(TDEBUG, "Found %d thermal zone(s)", tz_counter);
> +
> +	x86_pkg_temp_tz = calloc(tz_counter, sizeof(bool));
And here as well.

...
> +	memset(interrupts, 0, nproc * sizeof(*interrupts));
> +	FILE *fp = SAFE_FOPEN("/proc/interrupts", "r");
> +
> +	while (fgets(line, sizeof(line), fp)) {
> +		if (strstr(line, "Thermal event interrupts")) {
> +			interrupts_found = true;
> +			char *ptr = strchr(line, ':');
> +
> +			for (int i = 0; i < nproc; i++) {
> +				char *endptr;
> +
> +				while (*ptr && !isdigit(*ptr))
> +					ptr++;
> +
> +				errno = 0;
> +
> +				interrupts[i] = strtoull(ptr, &endptr, 10);
> +
> +				if (ptr == endptr)
> +					tst_brk(TBROK, "interrupt not found");
Printing CPU would help to debug on error, right?
					tst_brk(TBROK, "CPU %d: interrupt not found", nproc);

> +
> +				if (errno == ERANGE)
> +					tst_brk(TCONF, "interrupt out of range");
I would expect this is quite serious error (test bug), therefore TBROK should be
used, right?  TCONF is really for skipping the test due SUT not suitable for
running the test. But if the only error, it can be changed before merge.

					tst_brk(TBROK, "CPU %d: interrupt out of range", nproc);
...
> +static struct tst_test test = {
> +	.cleanup = cleanup,
> +	.forks_child = 1,
> +	.needs_drivers = (const char *const []) {
> +		"x86_pkg_temp_thermal",
> +		NULL
> +	},
> +	.min_runtime = 180,
You had .min_runtime = 5 sec in the v13, now again back 3 min.
As I wrote earlier, using tst_set_runtime() would be much better than expect 3
min run (on my laptop the test needs few sec, but it will waste time in case of
the test get stuck for whatever reason or might not enough for really big machine).

+#define	TEST_RUNTIME	3
 #define	RUNTIME		30
 #define	SLEEP		10
 #define	TEMP_INCREMENT	10
...
@@ -73,9 +74,10 @@ static void setup(void)
 	char line[8192];
 
 	nproc = tst_ncpus();
+	tst_set_runtime(nproc * TEST_RUNTIME);

This will lead to run in my machine:
tst_test.c:1887: TINFO: Overall timeout per run is 0h 00m 30s
tst_test.c:1908: TINFO: Updating runtime to 0h 00m 12s
tst_test.c:1887: TINFO: Overall timeout per run is 0h 00m 42s

I'd be ok to apply the following changes before merge. But I'd really prefer
Cyril to give ack to this before merge.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> +	.needs_root = 1,
> +	.setup = setup,
> +	.supported_archs = (const char *const []) {
> +		"x86",
> +		"x86_64",
> +		NULL
> +	},
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "9635c586a559ba0e45b2bfbff79c937ddbaf1a62"},
> +		{}
> +	},
> +	.test_all = run
> +};

+++ testcases/kernel/thermal/thermal_interrupt_events.c
@@ -19,6 +19,7 @@
 #include "tst_test.h"
 #include "tst_timer_test.h"
 
+#define	TEST_RUNTIME	3
 #define	RUNTIME		30
 #define	SLEEP		10
 #define	TEMP_INCREMENT	10
@@ -52,10 +53,10 @@ static void read_interrupts(uint64_t *interrupts, const int nproc)
 				interrupts[i] = strtoull(ptr, &endptr, 10);
 
 				if (ptr == endptr)
-					tst_brk(TBROK, "interrupt not found");
+					tst_brk(TBROK, "CPU %d: interrupt not found", nproc);
 
 				if (errno == ERANGE)
-					tst_brk(TCONF, "interrupt out of range");
+					tst_brk(TBROK, "CPU %d: interrupt out of range", nproc);
 
 				ptr = endptr;
 				tst_res(TDEBUG, "interrupts[%d]: %ld", i, interrupts[i]);
@@ -73,9 +74,10 @@ static void setup(void)
 	char line[8192];
 
 	nproc = tst_ncpus();
+	tst_set_runtime(nproc * TEST_RUNTIME);
 	tst_res(TDEBUG, "Number of logical cores: %d", nproc);
-	interrupt_init = calloc(nproc, sizeof(uint64_t));
-	interrupt_later = calloc(nproc, sizeof(uint64_t));
+	interrupt_init = SAFE_CALLOC(nproc, sizeof(uint64_t));
+	interrupt_later = SAFE_CALLOC(nproc, sizeof(uint64_t));
 
 	DIR *dir = SAFE_OPENDIR("/sys/class/thermal/");
 	struct dirent *entry;
@@ -87,7 +89,7 @@ static void setup(void)
 	SAFE_CLOSEDIR(dir);
 	tst_res(TDEBUG, "Found %d thermal zone(s)", tz_counter);
 
-	x86_pkg_temp_tz = calloc(tz_counter, sizeof(bool));
+	x86_pkg_temp_tz = SAFE_CALLOC(tz_counter, sizeof(bool));
 
 	for (int i = 0; i < tz_counter; i++) {
 		char path[PATH_MAX];
@@ -214,7 +216,6 @@ static struct tst_test test = {
 		"x86_pkg_temp_thermal",
 		NULL
 	},
-	.min_runtime = 180,
 	.needs_root = 1,
 	.setup = setup,
 	.supported_archs = (const char *const []) {

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