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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7f7ee5d9-22b6-383f-e7c8-d39ced64314a@redhat.com>
Date:   Wed, 5 Jul 2017 14:24:16 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     paulus@...abs.org, stable@...r.kernel.org
Subject: Re: [PATCH] kvm: avoid unused variable warning for UP builds



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