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

[Dongli Zhang <dongli.zhang@oracle.com>]

The implementation of this selftest program borrows from
tools/testing/selftests/kvm/steal_time.c.

The same concerns also apply to tools/testing/selftests/kvm/steal_time.c.

Based on my understanding, KVM selftests do not require strict error-handling code.

Thank you very much!

Dongli Zhang

On Fri, Sep 4, 2026 11:11:03AM -0700, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The delay accumulation loop can hang indefinitely because the background thread uses a fixed wall-clock timeout instead of synchronizing with the main thread.
> - [Medium] Unchecked return values from pthread APIs lead to undefined behavior and potential test hangs.
> --
> 
> commit 4093ded4f0187a7d185578eeea38cf167b09f1d2
> Author: Dongli Zhang <dongli.zhang@oracle.com>
> 
> KVM: selftests: Test steal time across vCPU pid changes on x86
> 
> This commit adds a selftest to verify that KVM correctly resets steal-time
> accounting when a vCPU is run from a new host thread. It creates a background
> thread to accumulate run delay and checks if the steal time remains monotonic
> across PID changes.
> 
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/steal_time_change_pid.c
>> @@ -0,0 +1,162 @@
> [ ... ]
>> +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);
> 
> [Severity: Medium]
> Does this background thread rely on a fixed wall-clock timeout instead of
> synchronizing with the main thread's delay accumulation?
> 
> If scheduler interleaving allows the main thread to run during this window,
> its accumulated run delay will be strictly less than MIN_RUN_DELAY_NS when
> this thread exits. Once this thread exits, CPU 0 becomes uncontended and
> the main thread's delay accumulation loop will hang indefinitely.
> 
>> +
>> +	return NULL;
>> +}
> [ ... ]
>> +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]
> If this pthread_setaffinity_np call fails (for example, if CPU 0 is not in
> the allowed task mask), could the threads run on different CPUs? This would
> prevent the main thread from ever accumulating runqueue wait time, causing it
> to hang forever in the sched_yield loop.
> 
> [ ... ]
>> +	run_delay = get_run_delay();
>> +	pthread_create(&thread, &attr, do_steal_time, NULL);
> 
> [Severity: Medium]
> If pthread_create fails due to resource limits, will the thread variable
> remain uninitialized? 
> 
>> +
>> +	while (get_run_delay() - run_delay < MIN_RUN_DELAY_NS)
>> +		sched_yield();
> 
> [Severity: Medium]
> As mentioned above, if do_steal_time exits before MIN_RUN_DELAY_NS is
> accumulated by the main thread, does this spinloop hang permanently because
> the background thread is no longer consuming CPU time?
> 
>> +
>> +	pthread_join(thread, NULL);
> 
> [Severity: Medium]
> If the thread variable is uninitialized due to a prior pthread_create
> failure, does passing it unconditionally to pthread_join invoke undefined
> behavior?
>