Re: [PATCH] kvm: add halt_poll_ns module parameter

[Paolo Bonzini <pbonzini@redhat.com>]

On 09/02/2015 16:21, Radim Krčmář wrote:
> 2015-02-06 13:48+0100, Paolo Bonzini:
> [...]
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
> 
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> Noticed changes since RFC:
>  - polling is used in more situations
>  - new tracepoint
>  - module parameter in nanoseconds
>  - properly handled time
>  - no polling with overcommit

Yup, pretty much what came in from Marcelo and David.

>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> @@ -148,6 +148,7 @@ struct kvm_vm_stat {
>>  };
>>  
>>  struct kvm_vcpu_stat {
>> +	u32 halt_successful_poll;
>>  	u32 halt_wakeup;
>>  };
> 
> We don't expose it in arch/arm/kvm/guest.c,
>   struct kvm_stats_debugfs_item debugfs_entries[] = {
>   	{ NULL }
>   };

Yes.  Too late for 3.20.

>> +TRACE_EVENT(kvm_vcpu_wakeup,
>> +	    TP_PROTO(__u64 ns, bool waited),
> 
> (__u64 is preferred here?)

Preferred to what?

>> @@ -1813,29 +1816,60 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
>>  void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>>  {
>> +	ktime_t start, cur;
>>  	DEFINE_WAIT(wait);
>> +	bool waited = false;
>> +
>> +	start = cur = ktime_get();
>> +	if (halt_poll_ns) {
>> +		ktime_t stop = ktime_add_ns(ktime_get(), halt_poll_ns);
>> +		do {
>> +			/*
>> +			 * This sets KVM_REQ_UNHALT if an interrupt
>> +			 * arrives.
>> +			 */
>> +			if (kvm_vcpu_check_block(vcpu) < 0) {
>> +				++vcpu->stat.halt_successful_poll;
>> +				goto out;
>> +			}
>> +			cur = ktime_get();
>> +		} while (single_task_running() && ktime_before(cur, stop));
> 
> After reading a bunch of code, I'm still not sure ...
>  - need_resched() can't be true when single_task_running()?
>    (I think it could happen -- balancing comes into mind.)

Single_task_running is per-CPU; for a task to relinquish control to
another task, you first need to have multiple tasks running.  In other
words, I think it cannot.

>  - is it ok to ignore need_resched() when single_task_running()?
>    (Most likely not.)

Paolo