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

[Petr Vorel <pvorel@suse.cz>]

Hi Piotr,

> Currently consists of only one test for the CPU package thermal sensor
> interface for Intel platforms.
> It works by checking the initial count of thermal interrupts. Then it
> decreases the threshold for sending a thermal interrupt to just above
> the current temperature and runs a workload on the CPU. Finally, it
> restores the original thermal threshold and checks whether the number
> of thermal interrupts increased.

> Signed-off-by: Piotr Kubaj <piotr.kubaj@intel.com>
> ---
> Patch version 11
> Switched 1 to true and added reference to kernel fix.

Generally LGTM, maybe Cyril will have some comments.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

...
> +static void read_interrupts(uint64_t *interrupts, const int nproc)
> +{
> +	bool interrupts_found = false;
> +	char line[8192];
> +
> +	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");
> +
> +				if (errno == ERANGE)
> +					tst_brk(TCONF, "interrupt out of range");

I wonder if this is expected to happen. Can be value really over LLONG_MAX?

Because arch/x86/include/asm/hardirq.h has

unsigned int irq_thermal_count

=> if it's over the range, I'd quit with tst_brk(TBROK).

Also we don't have safe_strtoull() in lib/safe_macros.c
(it would be trivial to add) otherwise I would just use it.

(Otherwise IMHO Cyril's request from v8 was done
https://lore.kernel.org/ltp/aZ72j9KvkhsDF7Yf@yuki.lan/)

> +
> +				ptr = endptr;
> +				tst_res(TDEBUG, "interrupts[%d]: %ld", i, interrupts[i]);
> +			}
> +			break;
> +		}
> +	}
> +	SAFE_FCLOSE(fp);
> +	if (!interrupts_found)
> +		tst_brk(TCONF, "No Thermal event interrupts line in /proc/interrupts");
> +}

...
> +static void cleanup(void)
> +{
> +	if (x86_pkg_temp_tz_found)
> +		SAFE_FILE_PRINTF(trip_path, "%d", trip);
> +
> +	free(x86_pkg_temp_tz);
> +	free(interrupt_init);
> +	free(interrupt_later);
> +}
> +
> +static void run(void)
> +{
> +	for (int i = 0; i < tz_counter; i++) {
> +		if (x86_pkg_temp_tz[i])
> +			test_zone(i);
> +	}
> +	read_interrupts(interrupt_later, nproc);
> +
> +	for (int i = 0; i < nproc; i++) {
> +		if (interrupt_later[i] < interrupt_init[i])
> +			tst_res(TFAIL, "CPU %d interrupt counter: %ld (previous: %ld)",
> +				i, interrupt_later[i], interrupt_init[i]);
> +	}
> +
> +	if (temp <= temp_high)
> +		tst_res(TFAIL, "Zone temperature is not rising as expected");
> +	else
> +		tst_res(TPASS, "x86 package thermal interrupt triggered");
> +}
> +
> +static struct tst_test test = {
> +	.cleanup = cleanup,
> +	.forks_child = 1,
> +	.needs_drivers = (const char *const []) {
> +		"x86_pkg_temp_thermal",
> +		NULL
> +	},
nit: Alternatively instead of needs drivers we could check via .needs_kconfigs
for CONFIG_X86_THERMAL_VECTOR (functionality wrapped in kernel, defined in
drivers/thermal/intel/Kconfig). Cyril WDYT?

IMHO we slowly convert from modules.{dep,builtin} based search in
lib/tst_kernel.c to kconfig related checks (functionality which come to LTP
later).

> +	.min_runtime = 180,

Test is mostly super quick. I suppose we can lower down this to e.g. 5
because it depends on number of the zones.
But could we define it to 5 here and set in the setup correct value via
tst_set_runtime().

> +	.needs_root = 1,
> +	.setup = setup,
> +	.supported_archs = (const char *const []) {
> +		"x86",
> +		"x86_64",
> +		NULL
> +	},
This is somehow redundant to .needs_drivers/.needs_kconfigs. OTOH it nicely
defines which arch is targeted.

> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "9635c586a559ba0e45b2bfbff79c937ddbaf1a62"},
FYI we normally use much shorter hash (12 chars), but IMHO this is good
approach. IMHO we should start to use sha1 (40 chars).

Kind regards,
Petr

> +		{}
> +	},
> +	.test_all = run
> +};

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