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

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.

[ ... ]

> +	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();

[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:

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

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