The next commit will introduce a member to the kvmppc_vcore struct which
references MAX_SMT_THREADS which is defined in kvm_book3s_asm.h, however
this file isn't included in kvm_host.h directly. Thus compiling for
certain platforms such as pmac32_defconfig and ppc64e_defconfig with KVM
fails due to MAX_SMT_THREADS not being defined.
Move the struct kvmppc_vcore definition to kvm_book3s.h which explicitly
includes kvm_book3s_asm.h.
---
Change Log:
V1 -> V2:
- Added patch to series
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
arch/powerpc/include/asm/kvm_book3s.h | 35 +++++++++++++++++++++++++++++++++++
arch/powerpc/include/asm/kvm_host.h | 35 -----------------------------------
2 files changed, 35 insertions(+), 35 deletions(-)
The struct kvmppc_vcore is a structure used to store various information
about a virtual core for a kvm guest. The runnable_threads element of the
struct provides a list of all of the currently runnable vcpus on the core
(those in the KVMPPC_VCPU_RUNNABLE state). The previous implementation of
this list was a linked_list. The next patch requires that the list be able
to be iterated over without holding the vcore lock.
Reimplement the runnable_threads list in the kvmppc_vcore struct as an
array. Implement function to iterate over valid entries in the array and
update access sites accordingly.
---
Change Log:
V1 -> V2:
- Nothing
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
arch/powerpc/include/asm/kvm_book3s.h | 2 +-
arch/powerpc/include/asm/kvm_host.h | 1 -
arch/powerpc/kvm/book3s_hv.c | 68 +++++++++++++++++++++--------------
3 files changed, 43 insertions(+), 28 deletions(-)
@@ -96,6 +97,26 @@ MODULE_PARM_DESC(h_ipi_redirect, "Redirect H_IPI wakeup to a free host core");staticvoidkvmppc_end_cede(structkvm_vcpu*vcpu);staticintkvmppc_hv_setup_htab_rma(structkvm_vcpu*vcpu);+staticinlinestructkvm_vcpu*next_runnable_thread(structkvmppc_vcore*vc,+int*ip)+{+inti=*ip;+structkvm_vcpu*vcpu;++while(++i<MAX_SMT_THREADS){+vcpu=READ_ONCE(vc->runnable_threads[i]);+if(vcpu){+*ip=i;+returnvcpu;+}+}+returnNULL;+}++/* Used to traverse the list of runnable threads for a given vcore */+#define for_each_runnable_thread(i, vcpu, vc) \+for(i=-1;(vcpu=next_runnable_thread(vc,&i));)+staticboolkvmppc_ipi_thread(intcpu){/* On POWER8 for IPIs to threads in the same core, use msgsnd */
@@ -2258,15 +2278,14 @@ static void collect_piggybacks(struct core_info *cip, int target_threads)staticvoidpost_guest_process(structkvmppc_vcore*vc,boolis_master){-intstill_running=0;+intstill_running=0,i;u64now;longret;-structkvm_vcpu*vcpu,*vnext;+structkvm_vcpu*vcpu;spin_lock(&vc->lock);now=get_tb();-list_for_each_entry_safe(vcpu,vnext,&vc->runnable_threads,-arch.run_list){+for_each_runnable_thread(i,vcpu,vc){/* cancel pending dec exception if dec is positive */if(now<vcpu->arch.dec_expires&&kvmppc_core_pending_dec(vcpu))
@@ -2306,8 +2325,8 @@ static void post_guest_process(struct kvmppc_vcore *vc, bool is_master)}if(vc->n_runnable>0&&vc->runner==NULL){/* make sure there's a candidate runner awake */-vcpu=list_first_entry(&vc->runnable_threads,-structkvm_vcpu,arch.run_list);+i=-1;+vcpu=next_runnable_thread(vc,&i);wake_up(&vcpu->arch.cpu_run);}}
@@ -2758,8 +2774,8 @@ static int kvmppc_run_vcpu(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)if(vc->n_runnable&&vc->vcore_state==VCORE_INACTIVE){/* Wake up some vcpu to run the core */-v=list_first_entry(&vc->runnable_threads,-structkvm_vcpu,arch.run_list);+i=-1;+v=next_runnable_thread(vc,&i);wake_up(&v->arch.cpu_run);}
This patch introduces new halt polling functionality into the kvm_hv kernel
module. When a vcore is idle it will poll for some period of time before
scheduling itself out.
When all of the runnable vcpus on a vcore have ceded (and thus the vcore is
idle) we schedule ourselves out to allow something else to run. In the
event that we need to wake up very quickly (for example an interrupt
arrives), we are required to wait until we get scheduled again.
Implement halt polling so that when a vcore is idle, and before scheduling
ourselves, we poll for vcpus in the runnable_threads list which have
pending exceptions or which leave the ceded state. If we poll successfully
then we can get back into the guest very quickly without ever scheduling
ourselves, otherwise we schedule ourselves out as before.
Testing of this patch with a TCP round robin test between two guests with
virtio network interfaces has found a decrease in round trip time from
~140us to ~115us. A performance gain is only seen when going out of and
back into the guest often and quickly, otherwise there is no net benefit
from the polling. The polling interval is adjusted such that when we are
often scheduled out for long periods of time it is reduced, and when we
often poll successfully it is increased. The rate at which the polling
interval increases or decreases, and the maximum polling interval, can
be set through module parameters.
Based on the implementation in the generic kvm module by Wanpeng Li and
Paolo Bonzini, and on direction from Paul Mackerras.
---
Change Log:
V1 -> V2:
- Nothing
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
arch/powerpc/include/asm/kvm_book3s.h | 1 +
arch/powerpc/include/asm/kvm_host.h | 1 +
arch/powerpc/kvm/book3s_hv.c | 115 +++++++++++++++++++++++++++++-----
arch/powerpc/kvm/trace_hv.h | 22 +++++++
4 files changed, 125 insertions(+), 14 deletions(-)
@@ -94,6 +94,23 @@ module_param_cb(h_ipi_redirect, &module_param_ops, &h_ipi_redirect,MODULE_PARM_DESC(h_ipi_redirect,"Redirect H_IPI wakeup to a free host core");#endif+/* Maximum halt poll interval defaults to KVM_HALT_POLL_NS_DEFAULT */+staticunsignedinthalt_poll_max_ns=KVM_HALT_POLL_NS_DEFAULT;+module_param(halt_poll_max_ns,uint,S_IRUGO|S_IWUSR);+MODULE_PARM_DESC(halt_poll_max_ns,"Maximum halt poll time in ns");++/* Factor by which the vcore halt poll interval is grown, default is to double+*/+staticunsignedinthalt_poll_ns_grow=2;+module_param(halt_poll_ns_grow,int,S_IRUGO);+MODULE_PARM_DESC(halt_poll_ns_grow,"Factor halt poll time is grown by");++/* Factor by which the vcore halt poll interval is shrunk, default is to reset+*/+staticunsignedinthalt_poll_ns_shrink;+module_param(halt_poll_ns_shrink,int,S_IRUGO);+MODULE_PARM_DESC(halt_poll_ns_shrink,"Factor halt poll time is shrunk by");+staticvoidkvmppc_end_cede(structkvm_vcpu*vcpu);staticintkvmppc_hv_setup_htab_rma(structkvm_vcpu*vcpu);
@@ -2620,32 +2637,82 @@ static void kvmppc_wait_for_exec(struct kvmppc_vcore *vc,finish_wait(&vcpu->arch.cpu_run,&wait);}+staticvoidgrow_halt_poll_ns(structkvmppc_vcore*vc)+{+/* 10us base */+if(vc->halt_poll_ns==0&&halt_poll_ns_grow)+vc->halt_poll_ns=10000;+else+vc->halt_poll_ns*=halt_poll_ns_grow;++if(vc->halt_poll_ns>halt_poll_max_ns)+vc->halt_poll_ns=halt_poll_max_ns;+}++staticvoidshrink_halt_poll_ns(structkvmppc_vcore*vc)+{+if(halt_poll_ns_shrink==0)+vc->halt_poll_ns=0;+else+vc->halt_poll_ns/=halt_poll_ns_shrink;+}++/* Check to see if any of the runnable vcpus on the vcore have pending+*exceptionsorarenolongerceded+*/+staticintkvmppc_vcore_check_block(structkvmppc_vcore*vc)+{+structkvm_vcpu*vcpu;+inti;++for_each_runnable_thread(i,vcpu,vc){+if(vcpu->arch.pending_exceptions||!vcpu->arch.ceded)+return1;+}++return0;+}+/**Allthevcpusinthisvcoreareidle,sowaitforadecrementer*orexternalinterrupttooneofthevcpus.vc->lockisheld.*/staticvoidkvmppc_vcore_blocked(structkvmppc_vcore*vc){-structkvm_vcpu*vcpu;-intdo_sleep=1,i;+intdo_sleep=1;+ktime_tcur,start;+u64block_ns;DECLARE_SWAITQUEUE(wait);-prepare_to_swait(&vc->wq,&wait,TASK_INTERRUPTIBLE);+/* Poll for pending exceptions and ceded state */+cur=start=ktime_get();+if(vc->halt_poll_ns){+ktime_tstop=ktime_add_ns(start,vc->halt_poll_ns);-/*-*Checkonelasttimeforpendingexceptionsandcededstateafter-*weputourselvesonthewaitqueue-*/-for_each_runnable_thread(i,vcpu,vc){-if(vcpu->arch.pending_exceptions||!vcpu->arch.ceded){-do_sleep=0;-break;-}+vc->vcore_state=VCORE_POLLING;+spin_unlock(&vc->lock);++do{+if(kvmppc_vcore_check_block(vc)){+do_sleep=0;+break;+}+cur=ktime_get();+}while(ktime_before(cur,stop));++spin_lock(&vc->lock);+vc->vcore_state=VCORE_INACTIVE;++if(!do_sleep)+gotoout;}-if(!do_sleep){+prepare_to_swait(&vc->wq,&wait,TASK_INTERRUPTIBLE);++if(kvmppc_vcore_check_block(vc)){finish_swait(&vc->wq,&wait);-return;+do_sleep=0;+gotoout;}vc->vcore_state=VCORE_SLEEPING;
@@ -2656,6 +2723,26 @@ static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)spin_lock(&vc->lock);vc->vcore_state=VCORE_INACTIVE;trace_kvmppc_vcore_blocked(vc,1);++cur=ktime_get();++out:+block_ns=ktime_to_ns(cur)-ktime_to_ns(start);++if(halt_poll_max_ns){+if(block_ns<=vc->halt_poll_ns)+;+/* We slept and blocked for longer than the max halt time */+elseif(vc->halt_poll_ns&&block_ns>halt_poll_max_ns)+shrink_halt_poll_ns(vc);+/* We slept and our poll time is too small */+elseif(vc->halt_poll_ns<halt_poll_max_ns&&+block_ns<halt_poll_max_ns)+grow_halt_poll_ns(vc);+}else+vc->halt_poll_ns=0;++trace_kvmppc_vcore_wakeup(do_sleep,block_ns);}staticintkvmppc_run_vcpu(structkvm_run*kvm_run,structkvm_vcpu*vcpu)
From: David Matlack <dmatlack@google.com> Date: 2016-07-11 16:57:31
On Mon, Jul 11, 2016 at 12:08 AM, Suraj Jitindar Singh
[off-list ref] wrote:
This patch introduces new halt polling functionality into the kvm_hv kernel
module. When a vcore is idle it will poll for some period of time before
scheduling itself out.
Is there any way to reuse the existing halt-polling code? Having two
copies risks them diverging over time.
quoted hunk
When all of the runnable vcpus on a vcore have ceded (and thus the vcore is
idle) we schedule ourselves out to allow something else to run. In the
event that we need to wake up very quickly (for example an interrupt
arrives), we are required to wait until we get scheduled again.
Implement halt polling so that when a vcore is idle, and before scheduling
ourselves, we poll for vcpus in the runnable_threads list which have
pending exceptions or which leave the ceded state. If we poll successfully
then we can get back into the guest very quickly without ever scheduling
ourselves, otherwise we schedule ourselves out as before.
Testing of this patch with a TCP round robin test between two guests with
virtio network interfaces has found a decrease in round trip time from
~140us to ~115us. A performance gain is only seen when going out of and
back into the guest often and quickly, otherwise there is no net benefit
from the polling. The polling interval is adjusted such that when we are
often scheduled out for long periods of time it is reduced, and when we
often poll successfully it is increased. The rate at which the polling
interval increases or decreases, and the maximum polling interval, can
be set through module parameters.
Based on the implementation in the generic kvm module by Wanpeng Li and
Paolo Bonzini, and on direction from Paul Mackerras.
---
Change Log:
V1 -> V2:
- Nothing
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
arch/powerpc/include/asm/kvm_book3s.h | 1 +
arch/powerpc/include/asm/kvm_host.h | 1 +
arch/powerpc/kvm/book3s_hv.c | 115 +++++++++++++++++++++++++++++-----
arch/powerpc/kvm/trace_hv.h | 22 +++++++
4 files changed, 125 insertions(+), 14 deletions(-)
@@ -94,6 +94,23 @@ module_param_cb(h_ipi_redirect, &module_param_ops, &h_ipi_redirect,MODULE_PARM_DESC(h_ipi_redirect,"Redirect H_IPI wakeup to a free host core");#endif+/* Maximum halt poll interval defaults to KVM_HALT_POLL_NS_DEFAULT */+staticunsignedinthalt_poll_max_ns=KVM_HALT_POLL_NS_DEFAULT;+module_param(halt_poll_max_ns,uint,S_IRUGO|S_IWUSR);+MODULE_PARM_DESC(halt_poll_max_ns,"Maximum halt poll time in ns");++/* Factor by which the vcore halt poll interval is grown, default is to double+*/+staticunsignedinthalt_poll_ns_grow=2;+module_param(halt_poll_ns_grow,int,S_IRUGO);+MODULE_PARM_DESC(halt_poll_ns_grow,"Factor halt poll time is grown by");++/* Factor by which the vcore halt poll interval is shrunk, default is to reset+*/+staticunsignedinthalt_poll_ns_shrink;+module_param(halt_poll_ns_shrink,int,S_IRUGO);+MODULE_PARM_DESC(halt_poll_ns_shrink,"Factor halt poll time is shrunk by");+staticvoidkvmppc_end_cede(structkvm_vcpu*vcpu);staticintkvmppc_hv_setup_htab_rma(structkvm_vcpu*vcpu);
@@ -2620,32 +2637,82 @@ static void kvmppc_wait_for_exec(struct kvmppc_vcore *vc,finish_wait(&vcpu->arch.cpu_run,&wait);}+staticvoidgrow_halt_poll_ns(structkvmppc_vcore*vc)+{+/* 10us base */+if(vc->halt_poll_ns==0&&halt_poll_ns_grow)+vc->halt_poll_ns=10000;+else+vc->halt_poll_ns*=halt_poll_ns_grow;++if(vc->halt_poll_ns>halt_poll_max_ns)+vc->halt_poll_ns=halt_poll_max_ns;+}++staticvoidshrink_halt_poll_ns(structkvmppc_vcore*vc)+{+if(halt_poll_ns_shrink==0)+vc->halt_poll_ns=0;+else+vc->halt_poll_ns/=halt_poll_ns_shrink;+}++/* Check to see if any of the runnable vcpus on the vcore have pending+*exceptionsorarenolongerceded+*/+staticintkvmppc_vcore_check_block(structkvmppc_vcore*vc)+{+structkvm_vcpu*vcpu;+inti;++for_each_runnable_thread(i,vcpu,vc){+if(vcpu->arch.pending_exceptions||!vcpu->arch.ceded)+return1;+}++return0;+}+/**Allthevcpusinthisvcoreareidle,sowaitforadecrementer*orexternalinterrupttooneofthevcpus.vc->lockisheld.*/staticvoidkvmppc_vcore_blocked(structkvmppc_vcore*vc){-structkvm_vcpu*vcpu;-intdo_sleep=1,i;+intdo_sleep=1;+ktime_tcur,start;+u64block_ns;DECLARE_SWAITQUEUE(wait);-prepare_to_swait(&vc->wq,&wait,TASK_INTERRUPTIBLE);+/* Poll for pending exceptions and ceded state */+cur=start=ktime_get();+if(vc->halt_poll_ns){+ktime_tstop=ktime_add_ns(start,vc->halt_poll_ns);-/*-*Checkonelasttimeforpendingexceptionsandcededstateafter-*weputourselvesonthewaitqueue-*/-for_each_runnable_thread(i,vcpu,vc){-if(vcpu->arch.pending_exceptions||!vcpu->arch.ceded){-do_sleep=0;-break;-}+vc->vcore_state=VCORE_POLLING;+spin_unlock(&vc->lock);++do{+if(kvmppc_vcore_check_block(vc)){+do_sleep=0;+break;+}+cur=ktime_get();+}while(ktime_before(cur,stop));++spin_lock(&vc->lock);+vc->vcore_state=VCORE_INACTIVE;++if(!do_sleep)+gotoout;}-if(!do_sleep){+prepare_to_swait(&vc->wq,&wait,TASK_INTERRUPTIBLE);++if(kvmppc_vcore_check_block(vc)){finish_swait(&vc->wq,&wait);-return;+do_sleep=0;+gotoout;}vc->vcore_state=VCORE_SLEEPING;
@@ -2656,6 +2723,26 @@ static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)spin_lock(&vc->lock);vc->vcore_state=VCORE_INACTIVE;trace_kvmppc_vcore_blocked(vc,1);++cur=ktime_get();++out:+block_ns=ktime_to_ns(cur)-ktime_to_ns(start);++if(halt_poll_max_ns){+if(block_ns<=vc->halt_poll_ns)+;+/* We slept and blocked for longer than the max halt time */+elseif(vc->halt_poll_ns&&block_ns>halt_poll_max_ns)+shrink_halt_poll_ns(vc);+/* We slept and our poll time is too small */+elseif(vc->halt_poll_ns<halt_poll_max_ns&&+block_ns<halt_poll_max_ns)+grow_halt_poll_ns(vc);+}else+vc->halt_poll_ns=0;++trace_kvmppc_vcore_wakeup(do_sleep,block_ns);}staticintkvmppc_run_vcpu(structkvm_run*kvm_run,structkvm_vcpu*vcpu)
@@ -432,6 +432,28 @@ TRACE_EVENT(kvmppc_vcore_blocked,__entry->runner_vcpu,__entry->n_runnable,__entry->tgid));+TRACE_EVENT(kvmppc_vcore_wakeup,+TP_PROTO(intdo_sleep,__u64ns),++TP_ARGS(do_sleep,ns),++TP_STRUCT__entry(+__field(__u64,ns)+__field(int,waited)+__field(pid_t,tgid)+),++TP_fast_assign(+__entry->ns=ns;+__entry->waited=do_sleep;+__entry->tgid=current->tgid;+),++TP_printk("%s time %lld ns, tgid=%d",+__entry->waited?"wait":"poll",+__entry->ns,__entry->tgid)+);+TRACE_EVENT(kvmppc_run_vcpu_enter,TP_PROTO(structkvm_vcpu*vcpu),--
2.5.5
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2016-07-11 17:07:52
On 11/07/2016 18:57, David Matlack wrote:
On Mon, Jul 11, 2016 at 12:08 AM, Suraj Jitindar Singh
[off-list ref] wrote:
quoted
This patch introduces new halt polling functionality into the kvm_hv kernel
module. When a vcore is idle it will poll for some period of time before
scheduling itself out.
Is there any way to reuse the existing halt-polling code? Having two
copies risks them diverging over time.
s/risks/guarantees/ :(
Unfortunately, handling of the hardware threads in KVM PPC is a mess,
and I don't think it's possible to remove the duplication.
Paolo
From: David Matlack <dmatlack@google.com> Date: 2016-07-11 17:26:52
On Mon, Jul 11, 2016 at 10:07 AM, Paolo Bonzini [off-list ref] wrote:
On 11/07/2016 18:57, David Matlack wrote:
quoted
On Mon, Jul 11, 2016 at 12:08 AM, Suraj Jitindar Singh
[off-list ref] wrote:
quoted
This patch introduces new halt polling functionality into the kvm_hv kernel
module. When a vcore is idle it will poll for some period of time before
scheduling itself out.
Is there any way to reuse the existing halt-polling code? Having two
copies risks them diverging over time.
s/risks/guarantees/ :(
Unfortunately, handling of the hardware threads in KVM PPC is a mess,
and I don't think it's possible to remove the duplication.
On Mon, Jul 11, 2016 at 10:07 AM, Paolo Bonzini [off-list ref] wrote:
quoted
On 11/07/2016 18:57, David Matlack wrote:
quoted
On Mon, Jul 11, 2016 at 12:08 AM, Suraj Jitindar Singh
[off-list ref] wrote:
quoted
This patch introduces new halt polling functionality into the kvm_hv kernel
module. When a vcore is idle it will poll for some period of time before
scheduling itself out.
Is there any way to reuse the existing halt-polling code? Having two
copies risks them diverging over time.
s/risks/guarantees/ :(
Unfortunately, handling of the hardware threads in KVM PPC is a mess,
and I don't think it's possible to remove the duplication.
Ah, ok. That's a shame.
It's definitely not ideal having this code duplicated, although we have
the issue that on PPC we only poll once all of the vcpus on a vcore have
ceded and need to retain a reference to that vcore.
Additionally we only actually do this in HV code, on the KVM PR
version we call the generic halt-polling code which doesn't know
about vcores.
I don't see an easy way to use the existing function.
vcpus have statistics associated with them which can be viewed within the
debugfs. Currently it is assumed within the vcpu_stat_get() and
vcpu_stat_get_per_vm() functions that all of these statistics are
represented as 32-bit numbers. The next patch adds some 64-bit statistics,
so add provisioning for the display of 64-bit vcpu statistics.
---
Change Log:
V1 -> V2:
- Nothing
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
arch/powerpc/kvm/book3s.c | 1 +
include/linux/kvm_host.h | 1 +
virt/kvm/kvm_main.c | 60 +++++++++++++++++++++++++++++++++++++++++++----
3 files changed, 58 insertions(+), 4 deletions(-)
From: David Matlack <dmatlack@google.com> Date: 2016-07-11 16:52:03
On Mon, Jul 11, 2016 at 12:08 AM, Suraj Jitindar Singh
[off-list ref] wrote:
vcpus have statistics associated with them which can be viewed within the
debugfs. Currently it is assumed within the vcpu_stat_get() and
vcpu_stat_get_per_vm() functions that all of these statistics are
represented as 32-bit numbers. The next patch adds some 64-bit statistics,
so add provisioning for the display of 64-bit vcpu statistics.
Thanks, we need 64-bit stats in other places as well. Can we use this
opportunity to wholesale upgrade all KVM stats from u32 to u64? Most
of this patch is duplicated code with "u32" swapped with "u64".
@@ -3627,9 +3658,30 @@ static int vcpu_stat_get(void *_offset, u64 *val)DEFINE_SIMPLE_ATTRIBUTE(vcpu_stat_fops,vcpu_stat_get,NULL,"%llu\n");+staticintvcpu_stat_u64_get(void*_offset,u64*val)+{+unsignedoffset=(long)_offset;+structkvm*kvm;+structkvm_stat_datastat_tmp={.offset=offset};+u64tmp_val;++*val=0;+spin_lock(&kvm_lock);+list_for_each_entry(kvm,&vm_list,vm_list){+stat_tmp.kvm=kvm;+vcpu_stat_u64_get_per_vm((void*)&stat_tmp,&tmp_val);+*val+=tmp_val;+}+spin_unlock(&kvm_lock);+return0;+}++DEFINE_SIMPLE_ATTRIBUTE(vcpu_stat_u64_fops,vcpu_stat_u64_get,NULL,"%llu\n");+staticconststructfile_operations*stat_fops[]={-[KVM_STAT_VCPU]=&vcpu_stat_fops,-[KVM_STAT_VM]=&vm_stat_fops,+[KVM_STAT_VCPU]=&vcpu_stat_fops,+[KVM_STAT_VCPU_U64]=&vcpu_stat_u64_fops,+[KVM_STAT_VM]=&vm_stat_fops,};staticintkvm_init_debug(void)--
2.5.5
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2016-07-11 17:05:44
On 11/07/2016 18:51, David Matlack wrote:
quoted
quoted
vcpus have statistics associated with them which can be viewed within the
debugfs. Currently it is assumed within the vcpu_stat_get() and
vcpu_stat_get_per_vm() functions that all of these statistics are
represented as 32-bit numbers. The next patch adds some 64-bit statistics,
so add provisioning for the display of 64-bit vcpu statistics.
Thanks, we need 64-bit stats in other places as well. Can we use this
opportunity to wholesale upgrade all KVM stats from u32 to u64? Most
of this patch is duplicated code with "u32" swapped with "u64".
I'm not sure of what 32-bit architectures would do, but perhaps we could
upgrade them to unsigned long at least.
Paolo
From: David Matlack <dmatlack@google.com> Date: 2016-07-11 17:30:52
On Mon, Jul 11, 2016 at 10:05 AM, Paolo Bonzini [off-list ref] wrote:
On 11/07/2016 18:51, David Matlack wrote:
quoted
quoted
quoted
vcpus have statistics associated with them which can be viewed within the
debugfs. Currently it is assumed within the vcpu_stat_get() and
vcpu_stat_get_per_vm() functions that all of these statistics are
represented as 32-bit numbers. The next patch adds some 64-bit statistics,
so add provisioning for the display of 64-bit vcpu statistics.
Thanks, we need 64-bit stats in other places as well. Can we use this
opportunity to wholesale upgrade all KVM stats from u32 to u64? Most
of this patch is duplicated code with "u32" swapped with "u64".
I'm not sure of what 32-bit architectures would do, but perhaps we could
upgrade them to unsigned long at least.
I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2016-07-11 19:31:15
On 11/07/2016 19:30, David Matlack wrote:
On Mon, Jul 11, 2016 at 10:05 AM, Paolo Bonzini [off-list ref] wrote:
quoted
On 11/07/2016 18:51, David Matlack wrote:
quoted
quoted
quoted
vcpus have statistics associated with them which can be viewed within the
debugfs. Currently it is assumed within the vcpu_stat_get() and
vcpu_stat_get_per_vm() functions that all of these statistics are
represented as 32-bit numbers. The next patch adds some 64-bit statistics,
so add provisioning for the display of 64-bit vcpu statistics.
Thanks, we need 64-bit stats in other places as well. Can we use this
opportunity to wholesale upgrade all KVM stats from u32 to u64? Most
of this patch is duplicated code with "u32" swapped with "u64".
I'm not sure of what 32-bit architectures would do, but perhaps we could
upgrade them to unsigned long at least.
I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.
Yes, but not all 32-bit architectures can do atomic read-modify-write
(e.g. add) operations on 64-bit values.
Paolo
From: David Matlack <dmatlack@google.com> Date: 2016-07-11 19:45:25
On Mon, Jul 11, 2016 at 12:31 PM, Paolo Bonzini [off-list ref] wrote:
On 11/07/2016 19:30, David Matlack wrote:
quoted
On Mon, Jul 11, 2016 at 10:05 AM, Paolo Bonzini [off-list ref] wrote:
quoted
On 11/07/2016 18:51, David Matlack wrote:
quoted
quoted
quoted
vcpus have statistics associated with them which can be viewed within the
debugfs. Currently it is assumed within the vcpu_stat_get() and
vcpu_stat_get_per_vm() functions that all of these statistics are
represented as 32-bit numbers. The next patch adds some 64-bit statistics,
so add provisioning for the display of 64-bit vcpu statistics.
Thanks, we need 64-bit stats in other places as well. Can we use this
opportunity to wholesale upgrade all KVM stats from u32 to u64? Most
of this patch is duplicated code with "u32" swapped with "u64".
I'm not sure of what 32-bit architectures would do, but perhaps we could
upgrade them to unsigned long at least.
I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.
Yes, but not all 32-bit architectures can do atomic read-modify-write
(e.g. add) operations on 64-bit values.
I think that's ok, none of the stats currently use atomic operations.
On Mon, Jul 11, 2016 at 12:31 PM, Paolo Bonzini [off-list ref] wrote:
quoted
On 11/07/2016 19:30, David Matlack wrote:
quoted
On Mon, Jul 11, 2016 at 10:05 AM, Paolo Bonzini [off-list ref] wrote:
quoted
On 11/07/2016 18:51, David Matlack wrote:
quoted
quoted
quoted
vcpus have statistics associated with them which can be viewed within the
debugfs. Currently it is assumed within the vcpu_stat_get() and
vcpu_stat_get_per_vm() functions that all of these statistics are
represented as 32-bit numbers. The next patch adds some 64-bit statistics,
so add provisioning for the display of 64-bit vcpu statistics.
Thanks, we need 64-bit stats in other places as well. Can we use this
opportunity to wholesale upgrade all KVM stats from u32 to u64? Most
of this patch is duplicated code with "u32" swapped with "u64".
I'm not sure of what 32-bit architectures would do, but perhaps we could
upgrade them to unsigned long at least.
I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.
Yes, but not all 32-bit architectures can do atomic read-modify-write
(e.g. add) operations on 64-bit values.
I think that's ok, none of the stats currently use atomic operations.
Yeah so this patch pretty much duplicates the 32-bit code.
So what you're saying is just replace all of the 32-bit statistics with
longs, that way we get 32-bit on 32-bit machines and 64-bit on 64-bit
machines? Then we just accept that on 32-bit machines we will get overflow
on some stats.
Or do you think u64s would be better and we accept that on 32-bit machines
we might get update conflicts from non-atomic concurrent accesses? Which
honestly I don't see being a huge issue in this use case.
From: Christian Borntraeger <hidden> Date: 2016-07-13 18:01:08
On 07/11/2016 09:31 PM, Paolo Bonzini wrote:
On 11/07/2016 19:30, David Matlack wrote:
quoted
On Mon, Jul 11, 2016 at 10:05 AM, Paolo Bonzini [off-list ref] wrote:
quoted
On 11/07/2016 18:51, David Matlack wrote:
quoted
quoted
quoted
vcpus have statistics associated with them which can be viewed within the
debugfs. Currently it is assumed within the vcpu_stat_get() and
vcpu_stat_get_per_vm() functions that all of these statistics are
represented as 32-bit numbers. The next patch adds some 64-bit statistics,
so add provisioning for the display of 64-bit vcpu statistics.
Thanks, we need 64-bit stats in other places as well. Can we use this
opportunity to wholesale upgrade all KVM stats from u32 to u64? Most
of this patch is duplicated code with "u32" swapped with "u64".
I'm not sure of what 32-bit architectures would do, but perhaps we could
upgrade them to unsigned long at least.
I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.
Yes, but not all 32-bit architectures can do atomic read-modify-write
(e.g. add) operations on 64-bit values.
So what about only doing it for the VCPU events? Those should be only
modified by one CPU. We would have some odd values on 32bit overflow, but
this will be certainly better than just start with 0
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2016-07-14 09:43:01
On 13/07/2016 20:00, Christian Borntraeger wrote:
quoted
quoted
quoted
quoted
I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.
Yes, but not all 32-bit architectures can do atomic read-modify-write
(e.g. add) operations on 64-bit values.
So what about only doing it for the VCPU events? Those should be only
modified by one CPU. We would have some odd values on 32bit overflow, but
this will be certainly better than just start with 0
I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.
Yes, but not all 32-bit architectures can do atomic read-modify-write
(e.g. add) operations on 64-bit values.
So what about only doing it for the VCPU events? Those should be only
modified by one CPU. We would have some odd values on 32bit overflow, but
this will be certainly better than just start with 0
If that's good enough for PPC, that's fine.
Paolo
I'm don't feel great about having vcpu_stats as u64 and vm_stats still as u32
it's just a bit inconsistent.
That being said, it's only the vcpu_stats which I require to be u64 at this
stage so it's possible to just upgrade those.
From: Christian Borntraeger <hidden> Date: 2016-07-18 07:18:07
On 07/15/2016 09:52 AM, Suraj Jitindar Singh wrote:
On 14/07/16 19:42, Paolo Bonzini wrote:
quoted
On 13/07/2016 20:00, Christian Borntraeger wrote:
quoted
quoted
quoted
quoted
quoted
I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.
Yes, but not all 32-bit architectures can do atomic read-modify-write
(e.g. add) operations on 64-bit values.
So what about only doing it for the VCPU events? Those should be only
modified by one CPU. We would have some odd values on 32bit overflow, but
this will be certainly better than just start with 0
If that's good enough for PPC, that's fine.
Paolo
I'm don't feel great about having vcpu_stats as u64 and vm_stats still as u32
it's just a bit inconsistent.
That being said, it's only the vcpu_stats which I require to be u64 at this
stage so it's possible to just upgrade those.
Yes, its not nice, but we probably want to avoid the overhead of atomics.
What about using u64 for vcpu_stats and unsigned long for vm_stats. This will be
correct for anyone and on 64bit systems we get 64 bits for everything?
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2016-07-18 08:25:00
On 18/07/2016 09:17, Christian Borntraeger wrote:
On 07/15/2016 09:52 AM, Suraj Jitindar Singh wrote:
quoted
On 14/07/16 19:42, Paolo Bonzini wrote:
quoted
On 13/07/2016 20:00, Christian Borntraeger wrote:
quoted
quoted
quoted
quoted
quoted
I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.
Yes, but not all 32-bit architectures can do atomic read-modify-write
(e.g. add) operations on 64-bit values.
So what about only doing it for the VCPU events? Those should be only
modified by one CPU. We would have some odd values on 32bit overflow, but
this will be certainly better than just start with 0
If that's good enough for PPC, that's fine.
Paolo
I'm don't feel great about having vcpu_stats as u64 and vm_stats still as u32
it's just a bit inconsistent.
That being said, it's only the vcpu_stats which I require to be u64 at this
stage so it's possible to just upgrade those.
Yes, its not nice, but we probably want to avoid the overhead of atomics.
What about using u64 for vcpu_stats and unsigned long for vm_stats. This will be
correct for anyone and on 64bit systems we get 64 bits for everything?
On 07/15/2016 09:52 AM, Suraj Jitindar Singh wrote:
quoted
On 14/07/16 19:42, Paolo Bonzini wrote:
quoted
On 13/07/2016 20:00, Christian Borntraeger wrote:
quoted
quoted
quoted
quoted
quoted
I thought u64 still existed on 32-bit architectures. unsigned long
would be fine but with the caveat that certain stats would overflow on
32-bit architectures.
Yes, but not all 32-bit architectures can do atomic read-modify-write
(e.g. add) operations on 64-bit values.
So what about only doing it for the VCPU events? Those should be only
modified by one CPU. We would have some odd values on 32bit overflow, but
this will be certainly better than just start with 0
If that's good enough for PPC, that's fine.
Paolo
I'm don't feel great about having vcpu_stats as u64 and vm_stats still as u32
it's just a bit inconsistent.
That being said, it's only the vcpu_stats which I require to be u64 at this
stage so it's possible to just upgrade those.
Yes, its not nice, but we probably want to avoid the overhead of atomics.
What about using u64 for vcpu_stats and unsigned long for vm_stats. This will be
correct for anyone and on 64bit systems we get 64 bits for everything?
vcpu stats are used to collect information about a vcpu which can be viewed
in the debugfs. For example halt_attempted_poll and halt_successful_poll
are used to keep track of the number of times the vcpu attempts to and
successfully polls. These stats are currently not used on powerpc.
Implement incrementation of the halt_attempted_poll and
halt_successful_poll vcpu stats for powerpc. Since these stats are summed
over all the vcpus for all running guests it doesn't matter which vcpu
they are attributed to, thus we choose the current runner vcpu of the
vcore.
Also add new vcpu stats: halt_poll_time and halt_wait_time to be used to
accumulate the total time spend polling and waiting respectively, and
halt_successful_wait to accumulate the number of times the vcpu waits.
Given that halt_poll_time and halt_wait_time are expressed in nanoseconds
it is necessary to represent these as 64-bit quantities, otherwise they
would overflow after only about 4 seconds.
Given that the total time spend either polling or waiting will be known and
the number of times that each was done, it will be possible to determine
the average poll and wait times. This will give the ability to tune the kvm
module parameters based on the calculated average wait and poll times.
---
Change Log:
V1 -> V2:
- Nothing
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
arch/powerpc/include/asm/kvm_host.h | 3 +++
arch/powerpc/kvm/book3s.c | 3 +++
arch/powerpc/kvm/book3s_hv.c | 14 +++++++++++++-
3 files changed, 19 insertions(+), 1 deletion(-)
@@ -2712,6 +2715,9 @@ static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)if(kvmppc_vcore_check_block(vc)){finish_swait(&vc->wq,&wait);do_sleep=0;+/* If we polled, count this as a successful poll */+if(vc->halt_poll_ns)+++vc->runner->stat.halt_successful_poll;gotoout;}
From: David Matlack <dmatlack@google.com> Date: 2016-07-11 16:49:32
On Mon, Jul 11, 2016 at 12:08 AM, Suraj Jitindar Singh
[off-list ref] wrote:
quoted hunk
vcpu stats are used to collect information about a vcpu which can be viewed
in the debugfs. For example halt_attempted_poll and halt_successful_poll
are used to keep track of the number of times the vcpu attempts to and
successfully polls. These stats are currently not used on powerpc.
Implement incrementation of the halt_attempted_poll and
halt_successful_poll vcpu stats for powerpc. Since these stats are summed
over all the vcpus for all running guests it doesn't matter which vcpu
they are attributed to, thus we choose the current runner vcpu of the
vcore.
Also add new vcpu stats: halt_poll_time and halt_wait_time to be used to
accumulate the total time spend polling and waiting respectively, and
halt_successful_wait to accumulate the number of times the vcpu waits.
Given that halt_poll_time and halt_wait_time are expressed in nanoseconds
it is necessary to represent these as 64-bit quantities, otherwise they
would overflow after only about 4 seconds.
Given that the total time spend either polling or waiting will be known and
the number of times that each was done, it will be possible to determine
the average poll and wait times. This will give the ability to tune the kvm
module parameters based on the calculated average wait and poll times.
---
Change Log:
V1 -> V2:
- Nothing
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
arch/powerpc/include/asm/kvm_host.h | 3 +++
arch/powerpc/kvm/book3s.c | 3 +++
arch/powerpc/kvm/book3s_hv.c | 14 +++++++++++++-
3 files changed, 19 insertions(+), 1 deletion(-)
@@ -2712,6 +2715,9 @@ static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)if(kvmppc_vcore_check_block(vc)){finish_swait(&vc->wq,&wait);do_sleep=0;+/* If we polled, count this as a successful poll */+if(vc->halt_poll_ns)+++vc->runner->stat.halt_successful_poll;gotoout;}
It's possible to poll and wait in one halt, conflating this stat with
polling time. Is it useful to split out a third stat,
halt_poll_fail_ns which counts how long we polled which ended up
sleeping? Then halt_wait_time only counts the time the VCPU spent on
the wait queue. The sum of all 3 is still the total time spent halted.
+ else if (vc->halt_poll_ns)
+ vc->runner->stat.halt_poll_time += block_ns;
+
if (halt_poll_max_ns) {
if (block_ns <= vc->halt_poll_ns)
;
--
2.5.5
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 11, 2016 at 12:08 AM, Suraj Jitindar Singh
[off-list ref] wrote:
quoted
vcpu stats are used to collect information about a vcpu which can be viewed
in the debugfs. For example halt_attempted_poll and halt_successful_poll
are used to keep track of the number of times the vcpu attempts to and
successfully polls. These stats are currently not used on powerpc.
Implement incrementation of the halt_attempted_poll and
halt_successful_poll vcpu stats for powerpc. Since these stats are summed
over all the vcpus for all running guests it doesn't matter which vcpu
they are attributed to, thus we choose the current runner vcpu of the
vcore.
Also add new vcpu stats: halt_poll_time and halt_wait_time to be used to
accumulate the total time spend polling and waiting respectively, and
halt_successful_wait to accumulate the number of times the vcpu waits.
Given that halt_poll_time and halt_wait_time are expressed in nanoseconds
it is necessary to represent these as 64-bit quantities, otherwise they
would overflow after only about 4 seconds.
Given that the total time spend either polling or waiting will be known and
the number of times that each was done, it will be possible to determine
the average poll and wait times. This will give the ability to tune the kvm
module parameters based on the calculated average wait and poll times.
---
Change Log:
V1 -> V2:
- Nothing
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
arch/powerpc/include/asm/kvm_host.h | 3 +++
arch/powerpc/kvm/book3s.c | 3 +++
arch/powerpc/kvm/book3s_hv.c | 14 +++++++++++++-
3 files changed, 19 insertions(+), 1 deletion(-)
@@ -2712,6 +2715,9 @@ static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)if(kvmppc_vcore_check_block(vc)){finish_swait(&vc->wq,&wait);do_sleep=0;+/* If we polled, count this as a successful poll */+if(vc->halt_poll_ns)+++vc->runner->stat.halt_successful_poll;gotoout;}
It's possible to poll and wait in one halt, conflating this stat with
polling time. Is it useful to split out a third stat,
halt_poll_fail_ns which counts how long we polled which ended up
sleeping? Then halt_wait_time only counts the time the VCPU spent on
the wait queue. The sum of all 3 is still the total time spent halted.
I see what you're saying. I would say that in the event that you do wait
then the most useful number is going to be the total block time (the sum
of the wait and poll time) as this is the minimum value you would have to
set the halt_poll_max_ns module parameter in order to ensure you poll
for long enough (in most circumstances) to avoid waiting, which is the main
use case I envision for this statistic. That being said this is definitely
a source of ambiguity and splitting this into two statistics would make the
distinction clearer without any loss of data, you could simply sum the two
stats to get the same number.
Either way I don't think it really makes much of a difference, but in the
interest of clarity I think I'll split the statistic.
quoted
+ else if (vc->halt_poll_ns)
+ vc->runner->stat.halt_poll_time += block_ns;
+
if (halt_poll_max_ns) {
if (block_ns <= vc->halt_poll_ns)
;
--
2.5.5
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 11, 2016 at 12:08 AM, Suraj Jitindar Singh
[off-list ref] wrote:
quoted
vcpu stats are used to collect information about a vcpu which can be viewed
in the debugfs. For example halt_attempted_poll and halt_successful_poll
are used to keep track of the number of times the vcpu attempts to and
successfully polls. These stats are currently not used on powerpc.
Implement incrementation of the halt_attempted_poll and
halt_successful_poll vcpu stats for powerpc. Since these stats are summed
over all the vcpus for all running guests it doesn't matter which vcpu
they are attributed to, thus we choose the current runner vcpu of the
vcore.
Also add new vcpu stats: halt_poll_time and halt_wait_time to be used to
accumulate the total time spend polling and waiting respectively, and
halt_successful_wait to accumulate the number of times the vcpu waits.
Given that halt_poll_time and halt_wait_time are expressed in nanoseconds
it is necessary to represent these as 64-bit quantities, otherwise they
would overflow after only about 4 seconds.
Given that the total time spend either polling or waiting will be known and
the number of times that each was done, it will be possible to determine
the average poll and wait times. This will give the ability to tune the kvm
module parameters based on the calculated average wait and poll times.
---
Change Log:
V1 -> V2:
- Nothing
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
arch/powerpc/include/asm/kvm_host.h | 3 +++
arch/powerpc/kvm/book3s.c | 3 +++
arch/powerpc/kvm/book3s_hv.c | 14 +++++++++++++-
3 files changed, 19 insertions(+), 1 deletion(-)
@@ -2712,6 +2715,9 @@ static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)if(kvmppc_vcore_check_block(vc)){finish_swait(&vc->wq,&wait);do_sleep=0;+/* If we polled, count this as a successful poll */+if(vc->halt_poll_ns)+++vc->runner->stat.halt_successful_poll;gotoout;}
It's possible to poll and wait in one halt, conflating this stat with
polling time. Is it useful to split out a third stat,
halt_poll_fail_ns which counts how long we polled which ended up
sleeping? Then halt_wait_time only counts the time the VCPU spent on
the wait queue. The sum of all 3 is still the total time spent halted.
I see what you're saying. I would say that in the event that you do wait
then the most useful number is going to be the total block time (the sum
of the wait and poll time) as this is the minimum value you would have to
set the halt_poll_max_ns module parameter in order to ensure you poll
for long enough (in most circumstances) to avoid waiting, which is the main
use case I envision for this statistic. That being said this is definitely
a source of ambiguity and splitting this into two statistics would make the
distinction clearer without any loss of data, you could simply sum the two
stats to get the same number.
Either way I don't think it really makes much of a difference, but in the
interest of clarity I think I'll split the statistic.
On further though, I really think that splitting this statistic is an
unnecessary source of ambiguity. In reality the interesting piece of
information is going to be the average time that you blocked on
either an unsuccessful poll or a successful poll.
So instead of splitting the statistic I'm going to rename them as:
halt_poll_time -> halt_block_time_successful_poll
halt_wait_time -> halt_block_time_waited
quoted
quoted
+ else if (vc->halt_poll_ns)
+ vc->runner->stat.halt_poll_time += block_ns;
+
if (halt_poll_max_ns) {
if (block_ns <= vc->halt_poll_ns)
;
--
2.5.5
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Matlack <dmatlack@google.com> Date: 2016-07-13 17:21:07
On Tue, Jul 12, 2016 at 11:07 PM, Suraj Jitindar Singh
[off-list ref] wrote:
On 12/07/16 16:17, Suraj Jitindar Singh wrote:
quoted
On 12/07/16 02:49, David Matlack wrote:
[snip]
quoted
quoted
It's possible to poll and wait in one halt, conflating this stat with
polling time. Is it useful to split out a third stat,
halt_poll_fail_ns which counts how long we polled which ended up
sleeping? Then halt_wait_time only counts the time the VCPU spent on
the wait queue. The sum of all 3 is still the total time spent halted.
I see what you're saying. I would say that in the event that you do wait
then the most useful number is going to be the total block time (the sum
of the wait and poll time) as this is the minimum value you would have to
set the halt_poll_max_ns module parameter in order to ensure you poll
for long enough (in most circumstances) to avoid waiting, which is the main
use case I envision for this statistic. That being said this is definitely
a source of ambiguity and splitting this into two statistics would make the
distinction clearer without any loss of data, you could simply sum the two
stats to get the same number.
Either way I don't think it really makes much of a difference, but in the
interest of clarity I think I'll split the statistic.
On further though, I really think that splitting this statistic is an
unnecessary source of ambiguity. In reality the interesting piece of
information is going to be the average time that you blocked on
either an unsuccessful poll or a successful poll.
So instead of splitting the statistic I'm going to rename them as:
halt_poll_time -> halt_block_time_successful_poll
halt_wait_time -> halt_block_time_waited
The downside of having only these 2 stats is there is no way to see
the total time spent halt-polling. Halt-polling shows up as host
kernel CPU usage on the VCPU thread, despite it really being idle
cycles that could be reclaimed. It's useful to have the total amount
of time spent halt-polling (halt_poll_fail + halt_poll_success) to
feed into provisioning/monitoring systems that look at CPU usage.
FWIW, I have a very similar patch internally. It adds 2 stats,
halt_poll_success_ns and halt_poll_fail_ns, to the halt-polling code
in virt/kvm/kvm_main.c. So if you agree splitting the stats makes
sense, it would be helpful to us if we can adopt the same naming
convention.
On Tue, Jul 12, 2016 at 11:07 PM, Suraj Jitindar Singh
[off-list ref] wrote:
quoted
On 12/07/16 16:17, Suraj Jitindar Singh wrote:
quoted
On 12/07/16 02:49, David Matlack wrote:
[snip]
quoted
quoted
quoted
It's possible to poll and wait in one halt, conflating this stat with
polling time. Is it useful to split out a third stat,
halt_poll_fail_ns which counts how long we polled which ended up
sleeping? Then halt_wait_time only counts the time the VCPU spent on
the wait queue. The sum of all 3 is still the total time spent halted.
I see what you're saying. I would say that in the event that you do wait
then the most useful number is going to be the total block time (the sum
of the wait and poll time) as this is the minimum value you would have to
set the halt_poll_max_ns module parameter in order to ensure you poll
for long enough (in most circumstances) to avoid waiting, which is the main
use case I envision for this statistic. That being said this is definitely
a source of ambiguity and splitting this into two statistics would make the
distinction clearer without any loss of data, you could simply sum the two
stats to get the same number.
Either way I don't think it really makes much of a difference, but in the
interest of clarity I think I'll split the statistic.
On further though, I really think that splitting this statistic is an
unnecessary source of ambiguity. In reality the interesting piece of
information is going to be the average time that you blocked on
either an unsuccessful poll or a successful poll.
So instead of splitting the statistic I'm going to rename them as:
halt_poll_time -> halt_block_time_successful_poll
halt_wait_time -> halt_block_time_waited
The downside of having only these 2 stats is there is no way to see
the total time spent halt-polling. Halt-polling shows up as host
kernel CPU usage on the VCPU thread, despite it really being idle
cycles that could be reclaimed. It's useful to have the total amount
of time spent halt-polling (halt_poll_fail + halt_poll_success) to
feed into provisioning/monitoring systems that look at CPU usage.
FWIW, I have a very similar patch internally. It adds 2 stats,
halt_poll_success_ns and halt_poll_fail_ns, to the halt-polling code
in virt/kvm/kvm_main.c. So if you agree splitting the stats makes
sense, it would be helpful to us if we can adopt the same naming
convention.
Ok, I didn't realise that was a use case.
Makes sense, I'll split it and adopt those names.
Thanks