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

[Paolo Bonzini <pbonzini@redhat.com>]

On 05/07/2017 14:22, David Hildenbrand wrote:
>> 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.

Separate patch of course. :)

>> +
>>  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.

I prefer having it.  There are corner cases (e.g. isolating bit 32 or
higher and the function accepting an unsigned int instead of a bool)
where it can save your butt, and it's idiomatic C.

Paolo

>>  	put_cpu();
>>  	free_cpumask_var(cpus);
>>  	return called;
>>
> 
> I like this from a cleanup point as well.
> 
>