Re: [PATCH v2 03/12] KVM: selftests: Pre-set threads affinity in hardware disable test when possible

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The return value of `pthread_attr_setaffinity_np()` is ignored, leading to silent failures and divergent error-handling between libcs.
- [Low] The test incorrectly uses the internal glibc macro `__USE_GNU` to detect GNU extension availability.
--

--- Patch [3]: [PATCH v2 03/12] KVM: selftests: Pre-set threads affinity in hardware disable test when possible ---
commit e87ff4fe7aad8c6f99e1f03d8e09fe9b5ceed69d
Author: Sean Christopherson <[email protected]>

KVM: selftests: Pre-set threads affinity in hardware disable test when possible

This commit attempts to reduce the perceived latency of pthread_create() on
large NUMA systems by pre-setting the CPU affinity for grandchildren threads
using pthread_attr_setaffinity_np() when building against a compatible libc.

> diff --git a/tools/testing/selftests/kvm/hardware_disable_test.c b/tools/testing/selftests/kvm/hardware_disable_test.c
> index f088b4af3ccc7..d8288147b0a69 100644
> --- a/tools/testing/selftests/kvm/hardware_disable_test.c
> +++ b/tools/testing/selftests/kvm/hardware_disable_test.c
> @@ -36,7 +36,9 @@ static void *run_vcpu(void *arg)
>  	struct kvm_vcpu *vcpu = arg;
>  	struct kvm_run *run = vcpu->run;
>  
> +#ifndef __USE_GNU

[Severity: Low]
Does checking __USE_GNU correctly detect the availability of this API across
different libc implementations?

__USE_GNU is an internal glibc macro. Other implementations like musl provide
pthread_attr_setaffinity_np() and support _GNU_SOURCE, but do not define
__USE_GNU, which would unnecessarily disable this optimization for them.

Should this code check for _GNU_SOURCE or rely on a standard feature test
macro instead?

> +	kvm_sched_setaffinity(0, sizeof(cpu_set_t), &threads_cpu_set);
> +#endif
>  
>  	vcpu_run(vcpu);
>  

[ ... ]

> @@ -80,22 +84,28 @@ static inline void check_join(pthread_t thread, void **retval)
>  static void run_test(u32 run)
>  {
>  	struct kvm_vcpu *vcpu;
> +	pthread_attr_t attr;
>  	struct kvm_vm *vm;
>  	pthread_t threads[VCPU_NUM];
>  	pthread_t throw_away;
>  	void *b;
>  	u32 i, j;
>  
> +	pthread_attr_init(&attr);
> +#ifdef __USE_GNU
> +	pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &threads_cpu_set);

[Severity: Medium]
Is it intentional to ignore the return value of pthread_attr_setaffinity_np()
here?

If this fails (for example, due to cgroup CPU mask restrictions), it silently
leaves the attribute unmodified and the optimization is missed. Meanwhile, the
fallback path in run_vcpu() using kvm_sched_setaffinity() explicitly asserts
and fails the test on error.

Should the return value be checked to maintain consistent test behavior and
error handling between the two paths?

> +#endif
> +
>  	vm = vm_create(VCPU_NUM);

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