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

[Gleb Natapov <gleb@kernel.org>]

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.