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.
 2) Thread synchronization logic allows the main thread to read
    uninitialized stack memory, causing test flakiness.
 3) Test fails instead of skipping gracefully on uniprocessor systems
    or when CPU 1 is unavailable.

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.
 3) Return KSFT_SKIP on "cpuset.cpus" setting failure.

[1] https://sashiko.dev/#/patchset/20260712235510.373125-1-longman%40redhat.com

Fixes: 98149f542530 ("selftests/cgroup: Add test for cpuset affinity on controller disable")
Signed-off-by: Waiman Long <longman@redhat.com>
---
 tools/testing/selftests/cgroup/test_cpuset.c | 38 +++++++++++---------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/cgroup/test_cpuset.c b/tools/testing/selftests/cgroup/test_cpuset.c
index 8b4c4a9dd78b..8c2d4d4ef1fc 100644
--- a/tools/testing/selftests/cgroup/test_cpuset.c
+++ b/tools/testing/selftests/cgroup/test_cpuset.c
@@ -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);
 
@@ -260,8 +260,6 @@ static int cpu_set_equal(cpu_set_t *dst, unsigned long mask)
 
 enum test_phase {
 	AFFINITY_SETUP,
-	AFFINITY_THREAD_A_READY,
-	AFFINITY_THREADS_READY,
 	AFFINITY_CONTROLLER_DISABLED,
 	AFFINITY_COMPLETE,
 	AFFINITY_ERROR
@@ -271,7 +269,7 @@ struct thread_args {
 	const char *cgroup;
 	cpu_set_t *affinity_before;
 	cpu_set_t *affinity_after;
-	enum test_phase ready_phase;
+	int affinity_before_ready;
 };
 
 static pthread_mutex_t test_mutex = PTHREAD_MUTEX_INITIALIZER;
@@ -289,8 +287,7 @@ static void *affinity_thread_fn(void *arg)
 		goto fail;
 
 	pthread_mutex_lock(&test_mutex);
-	if (test_phase < args->ready_phase)
-		test_phase = args->ready_phase;
+	args->affinity_before_ready = 1;
 	pthread_cond_broadcast(&test_cond);
 
 	while (test_phase < AFFINITY_CONTROLLER_DISABLED)
@@ -361,18 +358,20 @@ static int test_cpuset_affinity_on_controller_disable(const char *root)
 		goto cleanup;
 
 	/* Now enable cpuset controller in parent */
-	if (cg_write(parent, "cgroup.subtree_control", "+cpuset")) {
-		ret = KSFT_SKIP;
-		goto cleanup;
-	}
+	if (cg_write(parent, "cgroup.subtree_control", "+cpuset"))
+		goto skip;
 
-	/* Set CPU affinity constraints */
+	/*
+	 * Set CPU affinity constraints
+	 * Skip the test if the setting of "cpuset.cpus" fails as the test
+	 * system may not have CPU 1.
+	 */
 	if (cg_write(parent, "cpuset.cpus", "0-1"))
-		goto cleanup;
+		goto skip;
 	if (cg_write(child_a, "cpuset.cpus", "0-1"))
-		goto cleanup;
+		goto skip;
 	if (cg_write(child_b, "cpuset.cpus", "1"))
-		goto cleanup;
+		goto skip;
 
 	/* Move group leader (main thread) to child A */
 	if (cg_enter_current(child_a))
@@ -385,7 +384,7 @@ static int test_cpuset_affinity_on_controller_disable(const char *root)
 		.cgroup = child_a,
 		.affinity_before = &affinity_a_before,
 		.affinity_after = &affinity_a_after,
-		.ready_phase = AFFINITY_THREAD_A_READY,
+		.affinity_before_ready = 0,
 	};
 	if (pthread_create(&thread_a, NULL, affinity_thread_fn, &args_a))
 		goto cleanup;
@@ -395,14 +394,15 @@ static int test_cpuset_affinity_on_controller_disable(const char *root)
 		.cgroup = child_b,
 		.affinity_before = &affinity_b_before,
 		.affinity_after = &affinity_b_after,
-		.ready_phase = AFFINITY_THREADS_READY,
+		.affinity_before_ready = 0,
 	};
 	if (pthread_create(&thread_b, NULL, affinity_thread_fn, &args_b))
 		goto cleanup_threads;
 	thread_b_created = 1;
 
 	pthread_mutex_lock(&test_mutex);
-	while (test_phase < AFFINITY_THREADS_READY)
+	while ((test_phase < AFFINITY_ERROR) &&
+	       (args_a.affinity_before_ready + args_b.affinity_before_ready < 2))
 		pthread_cond_wait(&test_cond, &test_mutex);
 
 	/* If a thread failed during setup, bail out */
@@ -449,6 +449,10 @@ static int test_cpuset_affinity_on_controller_disable(const char *root)
 	ret = KSFT_PASS;
 	goto cleanup;
 
+skip:
+	ret = KSFT_SKIP;
+	goto cleanup;
+
 cleanup_threads:
 	pthread_mutex_lock(&test_mutex);
 	test_phase = AFFINITY_COMPLETE;
-- 
2.55.0

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)

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

> @@ -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);



Michal

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

Hello,

Applied to cgroup/for-7.3 with a couple of typos in the description fixed.

Thanks.

-- 
tejun

On 7/17/26 6:32 PM, Tejun Heo wrote:
> Hello,
>
> Applied to cgroup/for-7.3 with a couple of typos in the description fixed.
>
Thanks for fixing the typos. I did make some careless spelling or 
grammatical mistakes from time to time:-)

Cheers,
Longman