Re: [RFC PATCH v3 2/2] cgroup: selftests: Add tests for freezer time

[Tiffany Yang <ynaffit@google.com>]

Michal Koutný <mkoutny@suse.com> writes:
...


> 	if (curr < 0) {
> 		ret = KSFT_SKIP;
> 		goto cleanup;
> 	}
> 	if (curr > 0) {
> 		debug("Expect time (%ld) to be 0\n", curr);
> 		goto cleanup;
> 	}

> I might like the version with less indentation and explicit guards. It's
> only minor stylistic issue.


Noted! Will be fixed in v4.

>> +
>> +	/*
>> +	 * 2) Freeze the cgroup. Check that its freeze time is
>> +	 *    larger than 0.
>> +	 */
>> +	if (cg_freeze_nowait(cgroup, true))
>> +		goto cleanup;
>> +	prev = curr;
>> +	curr = cg_check_freezetime(cgroup);
>> +	if (curr <= prev) {

> Here and...
>> +		debug("Expect time (%ld) > 0\n", curr);
>> +		goto cleanup;
>> +	}
>> +
>> +	/*
>> +	 * 3) Sleep for 100 us. Check that the freeze time is at
>> +	 *    least 100 us larger than it was at 2).
>> +	 */
>> +	usleep(100);
>> +	prev = curr;
>> +	curr = cg_check_freezetime(cgroup);
>> +	if ((curr - prev) < 100) {

> ...here
> I'm slightly worried it may cause test flakiness on systems with too
> coarse clock granularity.

> Is the first check anyhow meaningful? (I think it's only as strong as
> checking return value of the preceding write(2) to cgroup.freeze.)


Hmm I had originally put the check at 2) in to make sure that the value
increases as expected for an empty cgroup (the simplest case), but I
think the check at 3) (and most other checks in these test cases)
establish the same thing.

The other purpose it serves is to act as kind of a buffer for the time
it takes to freeze the cgroup (t_1 -> t_2) to ensure that the cgroup
would be frozen for the entirety of the sleep. I.e., preventing the case
where we fail the check because the time measured at t_3 ends up being
(100 - the time it took to freeze).

That said, the time between writing to an empty cgroup's cgroup.freeze
and it beginning to freeze is basically negligible relative to the time
scales in this test, so I'm happy to take it out!

(If what I've written above is worded too confusingly, ignore it.
TL;DR: we don't need this check! I'm taking it out!)

> Would it compromise your use case if the latter check was at least
> 1000 μs (based on other usleeps in cgroup selftests)? (Ditto for other
> 100 μs checks.)

Not at all! I'll make this change for v4.

Thanks,
-- 
Tiffany Y. Yang