Re: [PATCH-cgroup/for-7.3] selftests/cgroup: Fix minor defects in test_cpuset

[Waiman Long <longman@redhat.com>]

On 7/20/26 11:01 AM, Michal Koutný wrote:
> On Fri, Jul 17, 2026 at 03:18:14PM -0400, Waiman Long <longman@redhat.com> wrote:
>> With commit 98149f542530 ("selftests/cgroup: Add test for cpuset affinity
>> on controller disable"), sashiko [1] had report 3 different issues with
>> the new test_cpuset_affinity_on_controller_disable() test.
>>
>>   1) `cpu_set_equal` iterates over mask bytes instead of bits, ignoring
>>      CPUs >= 8.
> Inline comment
>
>>   2) Thread synchronization logic allows the main thread to read
>>      uninitialized stack memory, causing test flakiness.
> Hm, I cannot see it (alhtough I don't see it through), what was the
> stack memory?
> (test_phase is static, then re-initalized)
I believe it means the followings:
         cpu_set_t affinity_a_before, affinity_a_after;
         cpu_set_t affinity_b_before, affinity_b_after;

These variables are supposed to be set by child_a and child_b, but it is 
possible that child_b runs first, set ready_phase to 
AFFINITY_THREADS_READY before child_a run and set affinity_a_before 
which can be any value depending on its previous state of the stack. So 
the subsequent cpu_set_equal(&affinity_a_before, 0x3) call can pass or 
fail. That is what I believe the problem is.

>
>>   3) Test fails instead of skipping gracefully on uniprocessor systems
>>      or when CPU 1 is unavailable.
> Interesting catch.
>
>> Fix the reported issues by:
>>   1) Iterates over the bit size of the mask.
>>   2) Test the new ready flag for each thread to end the wait
>>      on the condoitional variable and eliminate the now unneeded
>>      AFFINITY_THREAD_A_READY and AFFINITY_THREADS_READY test phases.
> But the symmetric synchronization with counter is easier to reason
> about.
>
>>   3) Return KSFT_SKIP on "cpuset.cpus" setting failure.
> It'd be better to have same style with test_cpuset_prs.sh, i.e. a guard
> at the beginning requesting a minimal number of CPUs. Next time...
Yes, that can be another alternative. It is just that the current fix is 
easier.
>
>> @@ -251,7 +251,7 @@ static int cpu_set_equal(cpu_set_t *dst, unsigned long mask)
>>   	CPU_ZERO(&expected);
>>   	assert(sizeof(mask) < CPU_SETSIZE);
>>   
>> -	for (int cpu = 0; cpu < sizeof(mask); ++cpu)
>> +	for (int cpu = 0; cpu < sizeof(mask) * 8; ++cpu)
>>   		if ((1UL << cpu) & mask)
>>   			CPU_SET(cpu, &expected);
> Oh, that was my braino in how masks are stored.
> Thanks for correcting me!
>
> It should also extend the assert accordingly:
>    	assert(sizeof(mask) * 8 < CPU_SETSIZE);
>
I don't think we need an assertion like that as CPU_SETSIZE will always 
be a multiple of a long bit size . Perhaps we could have something like

     int max = min(sizeof(mask) * 8, CPU_SETSIZE);
     for (...; cpu < max; ...)

Maybe next time when we need to update test_cpuset.c.

Cheers,
Longman