Re: [PATCH] kvm: add halt_poll_ns module parameter

["Radim Krčmář" <rkrcmar@redhat.com>]

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

> 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 }
  };

> diff --git a/include/trace/events/kvm.h b/include/trace/events/kvm.h
> @@ -37,6 +37,25 @@ TRACE_EVENT(kvm_userspace_exit,
>  		  __entry->errno < 0 ? -__entry->errno : __entry->reason)
>  );
>  
> +TRACE_EVENT(kvm_vcpu_wakeup,
> +	    TP_PROTO(__u64 ns, bool waited),

(__u64 is preferred here?)

> @@ -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.)
 - is it ok to ignore need_resched() when single_task_running()?
   (Most likely not.)

Thanks.


---
  Annoying detail:

  single_task_running() makes it look that we are only trying to avoid
  CPU power saving with accompanied latency.  We aren't doing it
  perfectly though:  scheduler can run to short task and the CPU will go
  to sleep.  (Before the event KVM is waiting for arrives.)

  Adding a general API (cpu_don't_sleep_for_next_x_ns) and modifying the
  cpu_idle_loop() would cover this improbable situation and may be
  applied to other use-cases too ... but still is extra work with
  uncertain acceptability :)