2016-05-19 19:23 GMT+08:00 Christian Borntraeger <[email protected]>:
> On 05/19/2016 11:26 AM, Wanpeng Li wrote:
>
> I think in general a good idea to poll if a timer will expire soon.
>
> Some patch comments:
>
> Same for all non-x86 archs:
>> +static inline unsigned int kvm_arch_timer_remaining(struct kvm_vcpu *vcpu)
>> {}
>
> A function returning int, without a return statement?
> That gives at least a compiler warning.
How about return 0 for all non-x86 archs?
>
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -78,6 +78,9 @@ module_param(halt_poll_ns_grow, uint, S_IRUGO | S_IWUSR);
>> static unsigned int halt_poll_ns_shrink;
>> module_param(halt_poll_ns_shrink, uint, S_IRUGO | S_IWUSR);
>>
>> +/* lower-end of message passing workload latency TCP_RR's poll time < 10us
>> */
>> +static unsigned int halt_poll_ns_base = 10000;
>> +
>> /*
>> * Ordering of locks:
>> *
>> @@ -1966,7 +1969,7 @@ static void grow_halt_poll_ns(struct kvm_vcpu *vcpu)
>> grow = READ_ONCE(halt_poll_ns_grow);
>> /* 10us base */
>> if (val == 0 && grow)
>> - val = 10000;
>> + val = halt_poll_ns_base;
>> else
>> val *= grow;
>>
>> @@ -2015,11 +2018,15 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>> DECLARE_SWAITQUEUE(wait);
>> bool waited = false;
>> u64 block_ns;
>> + unsigned int delta, remaining;
>>
>> + remaining = kvm_arch_timer_remaining(vcpu);
>
> and now it causes undefined behaviour, no?
Ditto.
>
>
>> start = cur = ktime_get();
>> - if (vcpu->halt_poll_ns) {
>> - ktime_t stop = ktime_add_ns(ktime_get(), vcpu->halt_poll_ns);
>> + if (vcpu->halt_poll_ns || (remaining < halt_poll_ns_base)) {
>> + ktime_t stop;
>>
>> + delta = vcpu->halt_poll_ns ? vcpu->halt_poll_ns : remaining;
>> + stop = ktime_add_ns(ktime_get(), delta);
>> ++vcpu->stat.halt_attempted_poll;
>> do {
>> /*
>>
>
> So you avoid to shrink/grow for these cases? Probably makes sense
I think my patch also shrink/grow for these cases.
Regards,
Wanpeng Li
