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

[Waiman Long]

On 7/20/26 11:01 AM, Michal Koutný wrote:
> On Fri, Jul 17, 2026 at 03:18:14PM -0400, Waiman Long <longman@xxxxxxxxxx> 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