Re: [PATCH 3/4] KVM: selftests: Test steal time across vCPU pid changes on x86

On Sat, Aug 15, 2026 10:55:19PM -0700, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The test races and hangs infinitely if the main thread is scheduled during the helper thread's 200µs wall-clock window.
> - [Medium] Unhandled `pthread_create()` and `pthread_setaffinity_np()` errors lead to an infinite test hang and undefined behavior.
> --
> 
> --- Patch [3]: [PATCH 3/4] KVM: selftests: Test steal time across vCPU pid changes on x86 ---
> commit dbc0e15e73158fcb584e1322da57a03214c88dd3
> Author: Dongli Zhang <[email protected]>
> 
> KVM: selftests: Test steal time across vCPU pid changes on x86
> 
> Add a selftest for the case where the same vCPU fd is run from a new host
> thread after steal time has already been enabled and updated.
> 
>> diff --git a/tools/testing/selftests/kvm/steal_time_change_pid.c b/tools/testing/selftests/kvm/steal_time_change_pid.c
>> new file mode 100644
>> index 0000000000000..3c39594db398e
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/steal_time_change_pid.c
> 
> [ ... ]
> 
>> +int main(void)
>> +{
>> +	struct kvm_vcpu *vcpu;
>> +	struct kvm_vm *vm;
>> +	pthread_attr_t attr;
>> +	pthread_t thread;
>> +	cpu_set_t cpuset;
>> +	long run_delay;
>> +	long run_delay_delta;
>> +
>> +	ksft_print_header();
>> +	ksft_set_plan(1);
>> +
>> +	CPU_ZERO(&cpuset);
>> +	CPU_SET(0, &cpuset);
>> +	pthread_attr_init(&attr);
>> +	pthread_attr_setaffinity_np(&attr, sizeof(cpuset), &cpuset);
>> +	pthread_setaffinity_np(pthread_self(), sizeof(cpuset), &cpuset);
> 
> [Severity: Medium]
> Is an error check needed for pthread_setaffinity_np()?
> 
> If CPU 0 is offline or excluded by the process taskset mask, this will fail
> silently. The threads would then run on different CPUs and avoid runqueue
> contention, preventing the main thread's wait time from increasing and
> potentially causing an infinite hang in the later sched_yield() loop.

This test intentionally follows the existing steal_time.c pattern, which uses
the same CPU0 affinity setup, unchecked pthread calls, and run_delay wait loop.

> 
> [ ... ]
> 
>> +	steal_time_enable(vcpu);
>> +	run_vcpu(vcpu);
>> +
>> +	run_delay = get_run_delay();
>> +	pthread_create(&thread, &attr, do_steal_time, NULL);
> 
> [Severity: Medium]
> What happens if pthread_create() fails here?
> 
> If thread creation fails, do_steal_time() won't execute to generate
> contention, and the subsequent loop waiting for run_delay to increase could
> spin infinitely.
> 
>> +
>> +	while (get_run_delay() - run_delay < MIN_RUN_DELAY_NS)
>> +		sched_yield();

This test intentionally follows the existing steal_time.c pattern, which uses
the same CPU0 affinity setup, unchecked pthread calls, and run_delay wait loop.

> 
> [Severity: Medium]
> Can this loop hang infinitely if the main thread is scheduled during the
> helper thread's wall-clock window?
> 
> The helper thread do_steal_time() bounds its execution to strictly
> MIN_RUN_DELAY_NS of wall-clock time:

This test intentionally follows the existing steal_time.c pattern, which uses
the same CPU0 affinity setup, unchecked pthread calls, and run_delay wait loop.

> 
> static void *do_steal_time(void *arg)
> {
> 	struct timespec ts, stop;
> 	clock_gettime(CLOCK_MONOTONIC, &ts);
> 	stop = timespec_add_ns(ts, MIN_RUN_DELAY_NS);
> 	while (timespec_to_ns(timespec_sub(ts, stop)) < 0)
> 		clock_gettime(CLOCK_MONOTONIC, &ts);
> 	return NULL;
> }
> 
> If a scheduler tick occurs during this window, the main thread may preempt
> the helper thread to evaluate this loop condition.
> 
> While the main thread is running, its runqueue wait time (run_delay) does
> not increase, but the helper thread's wall-clock timer still advances. As a
> result, the helper thread could exit having been on the runqueue/CPU for
> exactly the requested wall-clock time, but the main thread will have
> accumulated strictly less than MIN_RUN_DELAY_NS of wait time.
> 
> Once the helper thread exits, there are no competing threads on the pinned
> CPU, so the main thread's wait time stops increasing permanently, causing
> this loop to spin infinitely.
> 
> [ ... ]
> 
>> +	run_vcpu(vcpu);
>> +	sync_global_from_guest(vm, guest_stolen_time);
>> +	main_steal = guest_stolen_time;
>> +
>> +	TEST_ASSERT(main_steal >= MIN_RUN_DELAY_NS,
>> +		    "Expected steal time >= %ld, got %"PRIu64,
>> +		    MIN_RUN_DELAY_NS, main_steal);
>> +
>> +	pthread_create(&thread, NULL, vcpu_thread, vcpu);
> 
> [Severity: Medium]
> Should we check the return value of pthread_create() here?
> 
> If it fails, the thread variable remains uninitialized, which would trigger
> undefined behavior when passed to pthread_join() on the next line.
> 
>> +	pthread_join(thread, NULL);
> 

This test intentionally follows the existing steal_time.c pattern, which uses
the same CPU0 affinity setup, unchecked pthread calls, and run_delay wait loop.

Thank you very much!

Dongli Zhang