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