Re: [PATCH] kvm: avoid unused variable warning for UP builds

[David Hildenbrand <david@redhat.com>]

On 05.07.2017 12:35, Paolo Bonzini wrote:
> The uniprocessor version of smp_call_function_many does not evaluate
> all of its argument, and the compiler emits a warning about "wait"
> being unused.  This breaks the build on architectures for which
> "-Werror" is enabled by default.
> 
> Work around it by moving the invocation of smp_call_function_many to
> its own inline function.
> 
> Reported-by: Paul Mackerras <paulus@ozlabs.org>
> Cc: stable@vger.kernel.org
> Fixes: 7a97cec26b94c909f4cbad2dc3186af3e457a522
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  virt/kvm/kvm_main.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index f0fe9d02f6bb..09368501d9cf 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -187,12 +187,23 @@ static void ack_flush(void *_completed)
>  {
>  }
>  
> +static inline bool kvm_kick_many_cpus(const struct cpumask *cpus, bool wait)
> +{
> +	if (unlikely(!cpus))
> +		cpus = cpu_online_mask;
> +
> +	if (cpumask_empty(cpus))
> +		return false;
> +
> +	smp_call_function_many(cpus, ack_flush, NULL, wait);
> +	return true;
> +}

wonder if the !cpus case would be worth moving into smp_call_function_many.

smp_call_function_many() might also not kick any cpu, so we could make
it return if it actually kicked/called this on any cpu. Then you could
even get rid of the special handling of cpumask_empty(cpus) here and
simply return the result of smp_call_function_many.

> +
>  bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  {
>  	int i, cpu, me;
>  	cpumask_var_t cpus;
> -	bool called = true;
> -	bool wait = req & KVM_REQUEST_WAIT;
> +	bool called;
>  	struct kvm_vcpu *vcpu;
>  
>  	zalloc_cpumask_var(&cpus, GFP_ATOMIC);
> @@ -207,14 +218,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  
>  		if (cpus != NULL && cpu != -1 && cpu != me &&
>  		    kvm_request_needs_ipi(vcpu, req))
> -			cpumask_set_cpu(cpu, cpus);
> +			__cpumask_set_cpu(cpu, cpus);
>  	}
> -	if (unlikely(cpus == NULL))
> -		smp_call_function_many(cpu_online_mask, ack_flush, NULL, wait);
> -	else if (!cpumask_empty(cpus))
> -		smp_call_function_many(cpus, ack_flush, NULL, wait);
> -	else
> -		called = false;
> +	called = kvm_kick_many_cpus(cpus, !!(req & KVM_REQUEST_WAIT));

Is the !! really needed here? I think not.

>  	put_cpu();
>  	free_cpumask_var(cpus);
>  	return called;
> 

I like this from a cleanup point as well.


-- 

Thanks,

David