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

[sashiko-bot@kernel.org]

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904175550.430266-1-dongli.zhang@oracle.com?part=3