Re: [PATCH 3/9] KVM: selftests: Affine child tasks to other pCPUs in hardware disable test

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Thread initialization race causing severe CPU contention on the parent's single CPU, defeating the patch's purpose.
--

--- Patch [3]: [PATCH 3/9] KVM: selftests: Affine child tasks to other pCPUs in hardware disable test ---
commit b00682d387a4f6e1d76a822e983d388f7469a322
Author: Sean Christopherson <[email protected]>

KVM: selftests: Affine child tasks to other pCPUs in hardware disable test

This commit modifies the hardware disable test to affine child tasks to
different pCPUs than the parent task. This is intended to prevent the
child tasks from starving the parent task on preemptible kernels.

> diff --git a/tools/testing/selftests/kvm/hardware_disable_test.c b/tools/testing/selftests/kvm/hardware_disable_test.c
> index 3147f5c97e943..1bcbd13e99947 100644
> --- a/tools/testing/selftests/kvm/hardware_disable_test.c
> +++ b/tools/testing/selftests/kvm/hardware_disable_test.c
[ ... ]
> @@ -84,16 +85,11 @@ static void run_test(u32 run)
[ ... ]
>  		check_create_thread(&threads[i], NULL, run_vcpu, vcpu);
> -		check_set_affinity(threads[i], &cpu_set);
> +		check_set_affinity(threads[i], &child_cpu_set);
>  
>  		for (j = 0; j < SLEEPING_THREAD_NUM; ++j) {
>  			check_create_thread(&throw_away, NULL, sleeping_thread,
>  					    (void *)NULL);
> -			check_set_affinity(throw_away, &cpu_set);
> +			check_set_affinity(throw_away, &child_cpu_set);
>  		}
>  	}
[ ... ]
> @@ -147,12 +143,30 @@ void wait_for_child_setup(pid_t pid)
[ ... ]
> +static void setup_child_cpu_set(void)
> +{
> +	int cpu;
> +
> +	kvm_sched_getaffinity(0, sizeof(child_cpu_set), &child_cpu_set);
> +
> +	if (CPU_COUNT(&child_cpu_set) < 2)
> +		return;
> +
> +	cpu = pin_task_to_random_cpu(pthread_self(), &child_cpu_set);
> +	CPU_CLR(cpu, &child_cpu_set);
> +
> +	while (CPU_COUNT(&child_cpu_set) > VCPU_NUM)
> +		CPU_CLR(kvm_pick_random_cpu(&child_cpu_set), &child_cpu_set);
> +}

[Severity: Medium]
Since setup_child_cpu_set() pins the parent thread to a single random CPU
prior to spawning children, won't the newly created threads in run_test()
inherit this single-CPU affinity upon creation?

Because check_create_thread() spawns the threads before their affinity is
explicitly updated to child_cpu_set, they might immediately begin executing
on the exact same CPU as the parent.

Could this result in the child threads starving the parent on that single CPU
before the parent gets a chance to execute check_set_affinity()? This seems
like it might heavily delay the migration and trigger the same CPU contention
that this patch aims to fix.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3