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

[Petr Vorel <pvorel@suse.cz>]

Hi all,

TL;DR: no more concerns from my side.

> Hi!
> > > +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?

> That's just to make sure that kernel generated sane result. In tests we
> shouldn't blindly trust anything.

Makes sense.

> > 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).

> It should be TBROK indeed.

> > 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?

> We would need to apply the kconfig patch I've send and then add hook for
> the CONFIG_X86_THERMAL_VECTOR. I wouldn't wait for that with this patch,
> we can fix it once our infrastructure is ready.

Ah, I haven't noticed x86_pkg_temp_thermal driver can be disabled in the runtime
(otherwise there is always /sys/class/thermal/ when compiled with
CONFIG_X86_THERMAL_VECTOR). But sure, let's keep it this way, I also don't want
to block this effort.

> > 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.

> That's why I requested it to be added.

I'm sorry, I overlooked you asked for it.

Kind regards,
Petr

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