diff mbox

kvm: avoid unused variable warning for UP builds

Message ID 1499250930-34010-1-git-send-email-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paolo Bonzini July 5, 2017, 10:35 a.m. UTC 
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(-)

Comments

David Hildenbrand July 5, 2017, 12:22 p.m. UTC | #1 
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.
Paolo Bonzini July 5, 2017, 12:24 p.m. UTC | #2 
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.
> 
>
David Hildenbrand July 5, 2017, 12:26 p.m. UTC | #3 
On 05.07.2017 14:24, Paolo Bonzini wrote:
> 
> 
> 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. :)

Sure, for now take my

Reviewed-by: David Hildenbrand <david@redhat.com>

:)

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

Ah, I remember again why using booleans was once considered bad :)

Makes sense.

> 
> Paolo
kernel test robot July 6, 2017, 8:13 a.m. UTC | #4 
Hi Paolo,

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v4.12]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Paolo-Bonzini/kvm-avoid-unused-variable-warning-for-UP-builds/20170706-051225
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: x86_64-allyesdebian (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function 'kvm_make_all_cpus_request':
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:221:4: error: implicit declaration of function '__cpumask_set_cpu' [-Werror=implicit-function-declaration]
       __cpumask_set_cpu(cpu, cpus);
       ^~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/__cpumask_set_cpu +221 arch/x86/kvm/../../../virt/kvm/kvm_main.c

   215	
   216			if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu))
   217				continue;
   218	
   219			if (cpus != NULL && cpu != -1 && cpu != me &&
   220			    kvm_request_needs_ipi(vcpu, req))
 > 221				__cpumask_set_cpu(cpu, cpus);
   222		}
   223		called = kvm_kick_many_cpus(cpus, !!(req & KVM_REQUEST_WAIT));
   224		put_cpu();

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

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;
+}
+
 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));
 	put_cpu();
 	free_cpumask_var(cpus);
 	return called;