Re: [PATCH] KVM: PPC: BOOK3S: PR: Fix PURR and SPURR emulation
Paul Mackerras pau...@samba.org writes: On Tue, Jun 03, 2014 at 05:46:11PM +0530, Aneesh Kumar K.V wrote: We use time base for PURR and SPURR emulation with PR KVM since we are emulating a single threaded core. When using time base we need to make sure that we don't accumulate time spent in the host in PURR and SPURR value. Mostly looks good except for this... @@ -170,6 +175,11 @@ void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu, out: preempt_enable(); +/* + * Update purr and spurr using time base + */ +vcpu-arch.purr += get_tb() - vcpu-arch.entry_tb; +vcpu-arch.spurr += get_tb() - vcpu-arch.entry_tb; You need to do those updates before the out: label. Otherwise if this function gets called with !svcpu-in_use (which can happen if CONFIG_PREEMPT is enabled) we would do these updates a second time for one guest exit. The thing is that kvmppc_copy_from_svcpu() can get called from kvmppc_core_vcpu_put_pr() if the vcpu task gets preempted on the way out from the guest before we get to the regular call of kvmppc_copy_from_svcpu(). It would then get called again when the task gets to run, but this time it does nothing because svcpu-in_use is false. Looking at the code, since we enable MSR.EE early now, we might possibly end up calling this function late in the guest exit path. That implies, we may account that time (time spent after a preempt immediately following a guest exit) in purr/spurr. I guess that amount of inaccuracy is ok, because that is the best we could do here ? -aneesh -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: PPC: BOOK3S: PR: Fix PURR and SPURR emulation
Aneesh Kumar K.V aneesh.ku...@linux.vnet.ibm.com writes: Paul Mackerras pau...@samba.org writes: On Tue, Jun 03, 2014 at 05:46:11PM +0530, Aneesh Kumar K.V wrote: We use time base for PURR and SPURR emulation with PR KVM since we are emulating a single threaded core. When using time base we need to make sure that we don't accumulate time spent in the host in PURR and SPURR value. Mostly looks good except for this... @@ -170,6 +175,11 @@ void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu, out: preempt_enable(); + /* +* Update purr and spurr using time base +*/ + vcpu-arch.purr += get_tb() - vcpu-arch.entry_tb; + vcpu-arch.spurr += get_tb() - vcpu-arch.entry_tb; You need to do those updates before the out: label. Otherwise if this function gets called with !svcpu-in_use (which can happen if CONFIG_PREEMPT is enabled) we would do these updates a second time for one guest exit. The thing is that kvmppc_copy_from_svcpu() can get called from kvmppc_core_vcpu_put_pr() if the vcpu task gets preempted on the way out from the guest before we get to the regular call of kvmppc_copy_from_svcpu(). It would then get called again when the task gets to run, but this time it does nothing because svcpu-in_use is false. Looking at the code, since we enable MSR.EE early now, we might possibly end up calling this function late in the guest exit path. That implies, we may account that time (time spent after a preempt immediately following a guest exit) in purr/spurr. I guess that amount of inaccuracy is ok, because that is the best we could do here ? May be not, we do call kvmppc_copy_to_svcpu from preempt notifier callbacks. That should ensure that we get the right value. I have sent patch -V2 taking care of the above review comment. -aneesh -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: PPC: BOOK3S: PR: Fix PURR and SPURR emulation
On Tue, Jun 03, 2014 at 05:46:11PM +0530, Aneesh Kumar K.V wrote: We use time base for PURR and SPURR emulation with PR KVM since we are emulating a single threaded core. When using time base we need to make sure that we don't accumulate time spent in the host in PURR and SPURR value. Mostly looks good except for this... @@ -170,6 +175,11 @@ void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu, out: preempt_enable(); + /* + * Update purr and spurr using time base + */ + vcpu-arch.purr += get_tb() - vcpu-arch.entry_tb; + vcpu-arch.spurr += get_tb() - vcpu-arch.entry_tb; You need to do those updates before the out: label. Otherwise if this function gets called with !svcpu-in_use (which can happen if CONFIG_PREEMPT is enabled) we would do these updates a second time for one guest exit. The thing is that kvmppc_copy_from_svcpu() can get called from kvmppc_core_vcpu_put_pr() if the vcpu task gets preempted on the way out from the guest before we get to the regular call of kvmppc_copy_from_svcpu(). It would then get called again when the task gets to run, but this time it does nothing because svcpu-in_use is false. Paul. -- To unsubscribe from this list: send the line unsubscribe kvm-ppc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
