[PATCH] KVM/X86: vpmu migration, make perf_event associated with vcpu thread

[PATCH] KVM/X86: vpmu migration, make perf_event associated with vcpu thread

[Wang Hui]

After applying Paolo's patch, vpmu's data was migrated correctly.
https://patchwork.kernel.org/patch/2850813/

But when I wrote a test module to make IA32_PMC1 to count the event of unhalted
cpu-cycles, after migration the value of IA32_PMC1 never grows up again. I found
that after migration perf_event was created exactly, but when it was created,
"current" is qemu's main thread which won't enter no-root mode, so the count of
perf_event will never increase.

I have tried pid in the struct of kvm_vcpu to get the vcpu thread's task_struct,
but after migration when create perf_event, pid is pointed to qemu's main thread
but not vcpu thread because of the pid switching in vcpu_load. I don't understand
this very well, I think vcpu is created in qemu_kvm_cpu_thread_fn, which is the
vcpu thread, use the pid of current is enough, why switch is needed?

Maybe I was totally wrong, so I kept these code unchanged, add a extra "tid" to
keep the vcpu thread's pid, and use this "tid" to get the task_struct of vcpu
thread when create perf_event.

Thanks
Wang Hui

Signed-off-by: Wang Hui <john.wanghui@huawei.com>
---
 arch/x86/kvm/pmu.c       | 8 +++++++-
 arch/x86/kvm/x86.c       | 6 ++++++
 include/linux/kvm_host.h | 1 +
 virt/kvm/kvm_main.c      | 1 +
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 5c4f631..676227e 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -173,6 +173,8 @@ static void reprogram_counter(struct kvm_pmc *pmc, u32 type,
 		.exclude_kernel = exclude_kernel,
 		.config = config,
 	};
+	struct task_struct *task = NULL;
+
 	if (in_tx)
 		attr.config |= HSW_IN_TX;
 	if (in_tx_cp)
@@ -180,7 +182,11 @@ static void reprogram_counter(struct kvm_pmc *pmc, u32 type,

 	attr.sample_period = (-pmc->counter) & pmc_bitmask(pmc);

-	event = perf_event_create_kernel_counter(&attr, -1, current,
+	if (pmc->vcpu)
+		task = pid_task(pmc->vcpu->tid, PIDTYPE_PID);
+	if (!task)
+		task = current;
+	event = perf_event_create_kernel_counter(&attr, -1, task,
 						 intr ? kvm_perf_overflow_intr :
 						 kvm_perf_overflow, pmc);
 	if (IS_ERR(event)) {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 21ef1ba..f1f0e8e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6704,11 +6704,17 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
 int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 {
 	int r;
+	struct pid *cpu_thread_pid;

 	vcpu->arch.mtrr_state.have_fixed = 1;
 	r = vcpu_load(vcpu);
 	if (r)
 		return r;
+
+	cpu_thread_pid = get_task_pid(current, PIDTYPE_PID);
+	rcu_assign_pointer(vcpu->tid, cpu_thread_pid);
+	synchronize_rcu();
+
 	kvm_vcpu_reset(vcpu);
 	kvm_mmu_setup(vcpu);
 	vcpu_put(vcpu);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 9523d2a..ad7af9d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -235,6 +235,7 @@ struct kvm_vcpu {
 	int guest_fpu_loaded, guest_xcr0_loaded;
 	wait_queue_head_t wq;
 	struct pid *pid;
+	struct pid *tid;
 	int sigset_active;
 	sigset_t sigset;
 	struct kvm_vcpu_stat stat;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a0aa84b..80bcce5 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -242,6 +242,7 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init);

 void kvm_vcpu_uninit(struct kvm_vcpu *vcpu)
 {
+	put_pid(vcpu->tid);
 	put_pid(vcpu->pid);
 	kvm_arch_vcpu_uninit(vcpu);
 	free_page((unsigned long)vcpu->run);

Re: [PATCH] KVM/X86: vpmu migration, make perf_event associated with vcpu thread

[Gleb Natapov]

On Sat, Nov 30, 2013 at 01:59:24AM +0800, Wang Hui wrote:
> After applying Paolo's patch, vpmu's data was migrated correctly.
> https://patchwork.kernel.org/patch/2850813/
> 
> But when I wrote a test module to make IA32_PMC1 to count the event of unhalted
> cpu-cycles, after migration the value of IA32_PMC1 never grows up again. I found
> that after migration perf_event was created exactly, but when it was created,
> "current" is qemu's main thread which won't enter no-root mode, so the count of
> perf_event will never increase.
> 
> I have tried pid in the struct of kvm_vcpu to get the vcpu thread's task_struct,
> but after migration when create perf_event, pid is pointed to qemu's main thread
> but not vcpu thread because of the pid switching in vcpu_load. I don't understand
> this very well, I think vcpu is created in qemu_kvm_cpu_thread_fn, which is the
> vcpu thread, use the pid of current is enough, why switch is needed?
> 

Because the fact that thread that creates vcpu is the one that will
"run" the vcpu is QEMU implementation detail. Other userspace may create
all vcpus in one thread and then handle each one to a dedicated thread. Your
code will not work for such userspace. You need to use pid and reprogram all mmu
counters when it changes.

> Maybe I was totally wrong, so I kept these code unchanged, add a extra "tid" to
> keep the vcpu thread's pid, and use this "tid" to get the task_struct of vcpu
> thread when create perf_event.
> 
> Thanks
> Wang Hui
> 
> Signed-off-by: Wang Hui <john.wanghui@huawei.com>
> ---
>  arch/x86/kvm/pmu.c       | 8 +++++++-
>  arch/x86/kvm/x86.c       | 6 ++++++
>  include/linux/kvm_host.h | 1 +
>  virt/kvm/kvm_main.c      | 1 +
>  4 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 5c4f631..676227e 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -173,6 +173,8 @@ static void reprogram_counter(struct kvm_pmc *pmc, u32 type,
>  		.exclude_kernel = exclude_kernel,
>  		.config = config,
>  	};
> +	struct task_struct *task = NULL;
> +
>  	if (in_tx)
>  		attr.config |= HSW_IN_TX;
>  	if (in_tx_cp)
> @@ -180,7 +182,11 @@ static void reprogram_counter(struct kvm_pmc *pmc, u32 type,
> 
>  	attr.sample_period = (-pmc->counter) & pmc_bitmask(pmc);
> 
> -	event = perf_event_create_kernel_counter(&attr, -1, current,
> +	if (pmc->vcpu)
> +		task = pid_task(pmc->vcpu->tid, PIDTYPE_PID);
> +	if (!task)
> +		task = current;
> +	event = perf_event_create_kernel_counter(&attr, -1, task,
>  						 intr ? kvm_perf_overflow_intr :
>  						 kvm_perf_overflow, pmc);
>  	if (IS_ERR(event)) {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 21ef1ba..f1f0e8e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6704,11 +6704,17 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
>  int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>  {
>  	int r;
> +	struct pid *cpu_thread_pid;
> 
>  	vcpu->arch.mtrr_state.have_fixed = 1;
>  	r = vcpu_load(vcpu);
>  	if (r)
>  		return r;
> +
> +	cpu_thread_pid = get_task_pid(current, PIDTYPE_PID);
> +	rcu_assign_pointer(vcpu->tid, cpu_thread_pid);
> +	synchronize_rcu();
> +
>  	kvm_vcpu_reset(vcpu);
>  	kvm_mmu_setup(vcpu);
>  	vcpu_put(vcpu);
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 9523d2a..ad7af9d 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -235,6 +235,7 @@ struct kvm_vcpu {
>  	int guest_fpu_loaded, guest_xcr0_loaded;
>  	wait_queue_head_t wq;
>  	struct pid *pid;
> +	struct pid *tid;
>  	int sigset_active;
>  	sigset_t sigset;
>  	struct kvm_vcpu_stat stat;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a0aa84b..80bcce5 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -242,6 +242,7 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init);
> 
>  void kvm_vcpu_uninit(struct kvm_vcpu *vcpu)
>  {
> +	put_pid(vcpu->tid);
>  	put_pid(vcpu->pid);
>  	kvm_arch_vcpu_uninit(vcpu);
>  	free_page((unsigned long)vcpu->run);
> --
> 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

--
			Gleb.

Re: [PATCH] KVM/X86: vpmu migration, make perf_event associated with vcpu thread

[Wang Hui]

On 2013/11/30 18:54, Gleb Natapov wrote:
> On Sat, Nov 30, 2013 at 01:59:24AM +0800, Wang Hui wrote:
>> After applying Paolo's patch, vpmu's data was migrated correctly.
>> https://patchwork.kernel.org/patch/2850813/
>>
>> But when I wrote a test module to make IA32_PMC1 to count the event of unhalted
>> cpu-cycles, after migration the value of IA32_PMC1 never grows up again. I found
>> that after migration perf_event was created exactly, but when it was created,
>> "current" is qemu's main thread which won't enter no-root mode, so the count of
>> perf_event will never increase.
>>
>> I have tried pid in the struct of kvm_vcpu to get the vcpu thread's task_struct,
>> but after migration when create perf_event, pid is pointed to qemu's main thread
>> but not vcpu thread because of the pid switching in vcpu_load. I don't understand
>> this very well, I think vcpu is created in qemu_kvm_cpu_thread_fn, which is the
>> vcpu thread, use the pid of current is enough, why switch is needed?
>>
> 
> Because the fact that thread that creates vcpu is the one that will
> "run" the vcpu is QEMU implementation detail. Other userspace may create
> all vcpus in one thread and then handle each one to a dedicated thread. Your
> code will not work for such userspace. You need to use pid and reprogram all mmu
> counters when it changes.
> 
Thank you Gleb, my thought was too simple, I will try pid.

Re: [PATCH] KVM/X86: vpmu migration, make perf_event associated with vcpu thread

[Paolo Bonzini]

Il 29/11/2013 18:59, Wang Hui ha scritto:
> But when I wrote a test module to make IA32_PMC1 to count the event of unhalted
> cpu-cycles, after migration the value of IA32_PMC1 never grows up again. I found
> that after migration perf_event was created exactly, but when it was created,
> "current" is qemu's main thread which won't enter no-root mode, so the count of
> perf_event will never increase.

I think the fix is to apply the MSRs in QEMU's VCPU thread, through
run_on_cpu.  Interesting that I could not reproduce this when testing my
patches.

Paolo

Re: [PATCH] KVM/X86: vpmu migration, make perf_event associated with vcpu thread

[Wang Hui]

On 2013/11/30 19:08, Paolo Bonzini wrote:
> Il 29/11/2013 18:59, Wang Hui ha scritto:
>> But when I wrote a test module to make IA32_PMC1 to count the event of unhalted
>> cpu-cycles, after migration the value of IA32_PMC1 never grows up again. I found
>> that after migration perf_event was created exactly, but when it was created,
>> "current" is qemu's main thread which won't enter no-root mode, so the count of
>> perf_event will never increase.
> 
> I think the fix is to apply the MSRs in QEMU's VCPU thread, through
> run_on_cpu.  Interesting that I could not reproduce this when testing my
> patches.
> 
> Paolo
> 
Thank you for your advice, Paolo. I will try run_on_cpu.

And my qemu version was 1.5.1, maybe it is too old. I will try the latest version.

Thanks
Wang Hui