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

[Petr Vorel <pvorel@suse.cz>]

Hi Piotr,

...
> > > +static void run(void)
> > > +{
> > > +	bool status = 1;
> > > +	char line[8192];
> > > +	uint64_t interrupt_init[nproc], interrupt_later[nproc];
> > > +
> > > +	read_interrupts(interrupt_init, nproc);
> > > +
> > > +	DIR *dir = SAFE_OPENDIR("/sys/class/thermal/");
> > > +	struct dirent *entry;
> > > +	int tz_counter = 0;
> > > +
> > > +	while ((entry = SAFE_READDIR(dir))) {
> > > +		if ((strncmp(entry->d_name, "thermal_zone",
> > > sizeof("thermal_zone"))) > 0)
> > > +			tz_counter++;
> > > +	}
> > > +	SAFE_CLOSEDIR(dir);
> > > +	tst_res(TDEBUG, "Found %d thermal zone(s)", tz_counter);
> > As I noted previously, at least this part will not change if you run
> > test more
> > times, does it? Why not to move it to the setup()?

> > Imagine running test 1000x iterations:
> > ./thermal_interrupt_events -i 1000

> > Why to waste time with reading it again?

> > The only exception might be reading interrupts. I would expect it's
> > ok to have
> > only the initial state (read in the setup() as well), but maybe (when
> > test run
> > with more iterations via -i x) it needs to have the updated state
> > (from the
> > previous iteration).
> That part is still in consultation with our architect.

Thank you! Of course, it's ok to keep it if it's needed.

...
> > > +	for (int i = 0; i < tz_counter; i++) {
> > > +		if (x86_pkg_temp_tz[i]) {

> > run() is quite long. Maybe move content of of this loop would help.
> > Something like this (use whatever function name) would help the
> > readability.

> > 	for (int i = 0; i < tz_counter; i++) {
> > 		if (x86_pkg_temp_tz[i])
> > 			test_zone(x86_pkg_temp_tz[i]);
> > 	}
> > Maybe even split the while part into it's own function.
> Changed. I wanted to avoid creating functions that were only used once.

Understand, but there is also code readability which matters.

...
> > > +
> > > +			while (sleep_time > 0) {
> > > +				tst_res(TDEBUG, "Running for %f
> > > seconds, then sleeping for %d seconds", run_time, sleep_time);
> > nit: %f should be %d, right?
> run_time is double, because difftime returns double. Switching to %d
> causes a warning. If you prefer, I might add casting to int and then
> %d.

The output looks better but it's very minor.

Kind regards,
Petr

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