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

[Cyril Hrubis <chrubis@suse.cz>]

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.

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

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

> > +	.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
> > +};

-- 
Cyril Hrubis
chrubis@suse.cz

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