Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1476593 > unrolled thread

[RFC v2 PATCH 0/7] Add support for monitoring guest TLB operations

Back to article view | Back to linux.kernel

Contents

  [RFC v2 PATCH 0/7] Add support for monitoring guest TLB operations Punit Agrawal <punit.agrawal@arm.com> - 2016-09-05 18:40 +0200
    [RFC v2 PATCH 3/7] KVM: arm/arm64: Register perf trace event notifier Punit Agrawal <punit.agrawal@arm.com> - 2016-09-05 18:40 +0200
      Re: [RFC v2 PATCH 3/7] KVM: arm/arm64: Register perf trace event  notifier Christoffer Dall <christoffer.dall@linaro.org> - 2016-09-06 08:40 +0200
        Re: [RFC v2 PATCH 3/7] KVM: arm/arm64: Register perf trace event notifier Punit Agrawal <punit.agrawal@arm.com> - 2016-09-06 18:20 +0200
    [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process Punit Agrawal <punit.agrawal@arm.com> - 2016-09-05 18:40 +0200
      Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process Christoffer Dall <christoffer.dall@linaro.org> - 2016-09-06 08:30 +0200
        Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process Punit Agrawal <punit.agrawal@arm.com> - 2016-09-06 12:00 +0200
          Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process Christoffer Dall <christoffer.dall@linaro.org> - 2016-09-06 12:30 +0200
            Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process Punit Agrawal <punit.agrawal@arm.com> - 2016-09-06 13:10 +0200
              Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process Christoffer Dall <christoffer.dall@linaro.org> - 2016-09-06 13:20 +0200
                Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process Punit Agrawal <punit.agrawal@arm.com> - 2016-09-06 17:30 +0200
                  Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process Christoffer Dall <christoffer.dall@linaro.org> - 2016-09-06 19:00 +0200
                    Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process Punit Agrawal <punit.agrawal@arm.com> - 2016-09-06 19:10 +0200

#1476593 — [RFC v2 PATCH 0/7] Add support for monitoring guest TLB operations

Hi,

Although there are no PMU events to monitor TLB operations, ARMv8
supports trapping guest TLB maintenance operations to the
hypervisor. This trapping mechanism can be used to monitor the use of
guest TLB instructions.

As taking a trap for every TLB operation can have significant
overhead, trapping should only be enabled -

* on user request
* for the VM of interest

This patchset adds support to listen to perf trace event state change
notifications. The notifications and associated context are then used
to enable trapping of guest TLB operations when requested by the
user. The trap handling generates trace events (kvm_tlb_invalidate)
which can already be counted using existing perf trace
functionality.

I would appreciate feedback on the approach to tie the control of TLB
monitoring with perf trace events (Patch 1) especially if there are
any suggestions on avoiding (or reducing) the overhead of "perf trace"
notifications.

I looked at using regfunc/unregfunc tracepoint hooks but they don't
include the event context. But the bigger problem was that the
callbacks are only called on the first instance of simultaneously
executing perf stat invocations.

The patchset is based on v4.8-rc5 and adds support for monitoring
guest TLB operations on 64bit hosts. If the approach taken in the
patches is acceptable, I'll add 32bit host support as well.

This is the second posting of this patchset. Earlier discussion can be
found at [0].

Changes:

RFC -> RFC v2
* Patch 4 - Rename left-over TLBI macro to __TLBI
* Patch 6 - Replace individual TLB operation emulation with
  invalidating all stage 1 TLB for the VM. TLB monitoring is expected
  to be a debug feature and performance is not critical.

With this patchset, 'perf' tool when attached to a VM process can be
used to monitor the TLB operations. E.g., to monitor a VM with process
id 4166 -

# perf stat -e "kvm:kvm_tlb_invalidate" -p 4166

Perform some operations in VM (running 'make -j 7' on the kernel
sources in this instance). Breaking out of perf shows -

Performance counter stats for process id '4166':

         7,471,974      kvm:kvm_tlb_invalidate

     374.235405282 seconds time elapsed

All feedback welcome.

Thanks,
Punit

[0] http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1210715.html

Mark Rutland (2):
  arm64: tlbflush.h: add __tlbi() macro
  arm64/kvm: hyp: tlb: use __tlbi() helper

Punit Agrawal (5):
  perf/trace: Add notification for perf trace events
  KVM: Track the pid of the VM process
  KVM: arm/arm64: Register perf trace event notifier
  arm64: KVM: Handle trappable TLB instructions
  arm64: KVM: Enable selective trapping of TLB instructions

 arch/arm/include/asm/kvm_host.h   |   3 +
 arch/arm/kvm/arm.c                |   2 +
 arch/arm64/include/asm/kvm_asm.h  |   1 +
 arch/arm64/include/asm/kvm_host.h |   8 ++
 arch/arm64/include/asm/tlbflush.h |  31 ++++++--
 arch/arm64/kvm/Kconfig            |   4 +
 arch/arm64/kvm/Makefile           |   1 +
 arch/arm64/kvm/hyp/tlb.c          |  81 ++++++++++++++++++--
 arch/arm64/kvm/perf_trace.c       | 154 ++++++++++++++++++++++++++++++++++++++
 arch/arm64/kvm/sys_regs.c         |  81 ++++++++++++++++++++
 arch/arm64/kvm/trace.h            |  16 ++++
 include/linux/kvm_host.h          |   1 +
 include/linux/trace_events.h      |   3 +
 kernel/trace/trace_event_perf.c   |  24 ++++++
 virt/kvm/kvm_main.c               |   2 +
 15 files changed, 398 insertions(+), 14 deletions(-)
 create mode 100644 arch/arm64/kvm/perf_trace.c

-- 
2.8.1

[toc] | [next] | [standalone]

#1476594 — [RFC v2 PATCH 3/7] KVM: arm/arm64: Register perf trace event notifier

Register a notifier to track state changes of perf trace events.

The notifier will enable taking appropriate action for trace events
targeting VM.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_host.h   |   3 +
 arch/arm/kvm/arm.c                |   2 +
 arch/arm64/include/asm/kvm_host.h |   8 +++
 arch/arm64/kvm/Kconfig            |   4 ++
 arch/arm64/kvm/Makefile           |   1 +
 arch/arm64/kvm/perf_trace.c       | 122 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 140 insertions(+)
 create mode 100644 arch/arm64/kvm/perf_trace.c

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index de338d9..609998e 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -280,6 +280,9 @@ static inline int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
+static inline int kvm_perf_trace_init(void) { return 0; }
+static inline int kvm_perf_trace_teardown(void) { return 0; }
+
 void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
 
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 75f130e..e1b99c4 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -1220,6 +1220,7 @@ static int init_subsystems(void)
 		goto out;
 
 	kvm_perf_init();
+	kvm_perf_trace_init();
 	kvm_coproc_table_init();
 
 out:
@@ -1411,6 +1412,7 @@ out_err:
 void kvm_arch_exit(void)
 {
 	kvm_perf_teardown();
+	kvm_perf_trace_teardown();
 }
 
 static int arm_init(void)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 3eda975..f6ff8e5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -345,6 +345,14 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
+#if !defined(CONFIG_KVM_PERF_TRACE)
+static inline int kvm_perf_trace_init(void) { return 0; }
+static inline int kvm_perf_trace_teardown(void) { return 0; }
+#else
+int kvm_perf_trace_init(void);
+int kvm_perf_trace_teardown(void);
+#endif
+
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
 
 static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 9c9edc9..56e9537 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -19,6 +19,9 @@ if VIRTUALIZATION
 config KVM_ARM_VGIC_V3
 	bool
 
+config KVM_PERF_TRACE
+        bool
+
 config KVM
 	bool "Kernel-based Virtual Machine (KVM) support"
 	depends on OF
@@ -39,6 +42,7 @@ config KVM
 	select HAVE_KVM_MSI
 	select HAVE_KVM_IRQCHIP
 	select HAVE_KVM_IRQ_ROUTING
+	select KVM_PERF_TRACE if EVENT_TRACING && PERF_EVENTS
 	---help---
 	  Support hosting virtualized guest machines.
 	  We don't support KVM with 16K page tables yet, due to the multiple
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 695eb3c..7d175e4 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -19,6 +19,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o
 kvm-$(CONFIG_KVM_ARM_HOST) += emulate.o inject_fault.o regmap.o
 kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
 kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o
+kvm-$(CONFIG_KVM_PERF_TRACE) += perf_trace.o
 
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-init.o
diff --git a/arch/arm64/kvm/perf_trace.c b/arch/arm64/kvm/perf_trace.c
new file mode 100644
index 0000000..8bacd18
--- /dev/null
+++ b/arch/arm64/kvm/perf_trace.c
@@ -0,0 +1,122 @@
+/*
+ * Copyright (C) 2016 ARM Ltd.
+ * Author: Punit Agrawal <punit.agrawal@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/kvm_host.h>
+#include <linux/trace_events.h>
+
+typedef int (*perf_trace_callback_fn)(struct kvm *kvm, bool enable);
+
+struct kvm_trace_hook {
+	char *key;
+	perf_trace_callback_fn setup_fn;
+};
+
+static struct kvm_trace_hook trace_hook[] = {
+	{ },
+};
+
+static perf_trace_callback_fn find_trace_callback(const char *trace_key)
+{
+	int i;
+
+	for (i = 0; trace_hook[i].key; i++)
+		if (!strcmp(trace_key, trace_hook[i].key))
+			return trace_hook[i].setup_fn;
+
+	return NULL;
+}
+
+static int kvm_perf_trace_notifier(struct notifier_block *nb,
+				   unsigned long event, void *data)
+{
+	struct perf_event *p_event = data;
+	struct trace_event_call *tp_event = p_event->tp_event;
+	perf_trace_callback_fn setup_trace_fn;
+	struct kvm *kvm = NULL;
+	struct pid *pid;
+	bool found = false;
+
+	/*
+	 * Is this a trace point?
+	 */
+	if (!(tp_event->flags & TRACE_EVENT_FL_TRACEPOINT))
+		goto out;
+
+	/*
+	 * We'll get here for events we care to monitor for KVM. As we
+	 * only care about events attached to a VM, check that there
+	 * is a task associated with the perf event.
+	 */
+	if (p_event->attach_state != PERF_ATTACH_TASK)
+		goto out;
+
+	/*
+	 * This notifier gets called when perf trace event instance is
+	 * added or removed. Until we can restrict this to events of
+	 * interest in core, minimise the overhead below.
+	 *
+	 * Do we care about it? i.e., is there a callback for this
+	 * trace point?
+	 */
+	setup_trace_fn = find_trace_callback(tp_event->tp->name);
+	if (!setup_trace_fn)
+		goto out;
+
+	pid = get_task_pid(p_event->hw.target, PIDTYPE_PID);
+
+	/*
+	 * Does it match any of the VMs?
+	 */
+	spin_lock(&kvm_lock);
+	list_for_each_entry(kvm, &vm_list, vm_list) {
+		if (kvm->pid == pid) {
+			found = true;
+			break;
+		}
+	}
+	spin_unlock(&kvm_lock);
+
+	put_pid(pid);
+	if (!found)
+		goto out;
+
+	switch (event) {
+	case TRACE_REG_PERF_OPEN:
+		setup_trace_fn(kvm, true);
+		break;
+
+	case TRACE_REG_PERF_CLOSE:
+		setup_trace_fn(kvm, false);
+		break;
+	}
+
+out:
+	return 0;
+}
+
+static struct notifier_block kvm_perf_trace_notifier_block = {
+	.notifier_call = kvm_perf_trace_notifier,
+};
+
+int kvm_perf_trace_init(void)
+{
+	return perf_trace_notifier_register(&kvm_perf_trace_notifier_block);
+}
+
+int kvm_perf_trace_teardown(void)
+{
+	return perf_trace_notifier_unregister(&kvm_perf_trace_notifier_block);
+}
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1477102 — Re: [RFC v2 PATCH 3/7] KVM: arm/arm64: Register perf trace event notifier

On Mon, Sep 05, 2016 at 05:31:33PM +0100, Punit Agrawal wrote:
> Register a notifier to track state changes of perf trace events.
> 
> The notifier will enable taking appropriate action for trace events
> targeting VM.
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>

Overall this looks reasonable, but I'm wondering if most if this logic
should really go in virt/kvm/perf_trace.c and call into arch-specific
hooks, similar to the way it works for preempt notifiers.

On the other hand, if arm/arm64 are the only two architectures that are
going to use this, creating stubs for the other architectures could be a
bit tedious.

Thanks,
-Christoffer

> ---
>  arch/arm/include/asm/kvm_host.h   |   3 +
>  arch/arm/kvm/arm.c                |   2 +
>  arch/arm64/include/asm/kvm_host.h |   8 +++
>  arch/arm64/kvm/Kconfig            |   4 ++
>  arch/arm64/kvm/Makefile           |   1 +
>  arch/arm64/kvm/perf_trace.c       | 122 ++++++++++++++++++++++++++++++++++++++
>  6 files changed, 140 insertions(+)
>  create mode 100644 arch/arm64/kvm/perf_trace.c
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index de338d9..609998e 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -280,6 +280,9 @@ static inline int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
>  int kvm_perf_init(void);
>  int kvm_perf_teardown(void);
>  
> +static inline int kvm_perf_trace_init(void) { return 0; }
> +static inline int kvm_perf_trace_teardown(void) { return 0; }
> +
>  void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>  
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 75f130e..e1b99c4 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -1220,6 +1220,7 @@ static int init_subsystems(void)
>  		goto out;
>  
>  	kvm_perf_init();
> +	kvm_perf_trace_init();
>  	kvm_coproc_table_init();
>  
>  out:
> @@ -1411,6 +1412,7 @@ out_err:
>  void kvm_arch_exit(void)
>  {
>  	kvm_perf_teardown();
> +	kvm_perf_trace_teardown();
>  }
>  
>  static int arm_init(void)
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 3eda975..f6ff8e5 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -345,6 +345,14 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  int kvm_perf_init(void);
>  int kvm_perf_teardown(void);
>  
> +#if !defined(CONFIG_KVM_PERF_TRACE)
> +static inline int kvm_perf_trace_init(void) { return 0; }
> +static inline int kvm_perf_trace_teardown(void) { return 0; }
> +#else
> +int kvm_perf_trace_init(void);
> +int kvm_perf_trace_teardown(void);
> +#endif
> +
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>  
>  static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 9c9edc9..56e9537 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -19,6 +19,9 @@ if VIRTUALIZATION
>  config KVM_ARM_VGIC_V3
>  	bool
>  
> +config KVM_PERF_TRACE
> +        bool
> +
>  config KVM
>  	bool "Kernel-based Virtual Machine (KVM) support"
>  	depends on OF
> @@ -39,6 +42,7 @@ config KVM
>  	select HAVE_KVM_MSI
>  	select HAVE_KVM_IRQCHIP
>  	select HAVE_KVM_IRQ_ROUTING
> +	select KVM_PERF_TRACE if EVENT_TRACING && PERF_EVENTS
>  	---help---
>  	  Support hosting virtualized guest machines.
>  	  We don't support KVM with 16K page tables yet, due to the multiple
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 695eb3c..7d175e4 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -19,6 +19,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += emulate.o inject_fault.o regmap.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o
> +kvm-$(CONFIG_KVM_PERF_TRACE) += perf_trace.o
>  
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic.o
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-init.o
> diff --git a/arch/arm64/kvm/perf_trace.c b/arch/arm64/kvm/perf_trace.c
> new file mode 100644
> index 0000000..8bacd18
> --- /dev/null
> +++ b/arch/arm64/kvm/perf_trace.c
> @@ -0,0 +1,122 @@
> +/*
> + * Copyright (C) 2016 ARM Ltd.
> + * Author: Punit Agrawal <punit.agrawal@arm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/kvm_host.h>
> +#include <linux/trace_events.h>
> +
> +typedef int (*perf_trace_callback_fn)(struct kvm *kvm, bool enable);
> +
> +struct kvm_trace_hook {
> +	char *key;
> +	perf_trace_callback_fn setup_fn;
> +};
> +
> +static struct kvm_trace_hook trace_hook[] = {
> +	{ },
> +};
> +
> +static perf_trace_callback_fn find_trace_callback(const char *trace_key)
> +{
> +	int i;
> +
> +	for (i = 0; trace_hook[i].key; i++)
> +		if (!strcmp(trace_key, trace_hook[i].key))
> +			return trace_hook[i].setup_fn;
> +
> +	return NULL;
> +}
> +
> +static int kvm_perf_trace_notifier(struct notifier_block *nb,
> +				   unsigned long event, void *data)
> +{
> +	struct perf_event *p_event = data;
> +	struct trace_event_call *tp_event = p_event->tp_event;
> +	perf_trace_callback_fn setup_trace_fn;
> +	struct kvm *kvm = NULL;
> +	struct pid *pid;
> +	bool found = false;
> +
> +	/*
> +	 * Is this a trace point?
> +	 */
> +	if (!(tp_event->flags & TRACE_EVENT_FL_TRACEPOINT))
> +		goto out;
> +
> +	/*
> +	 * We'll get here for events we care to monitor for KVM. As we
> +	 * only care about events attached to a VM, check that there
> +	 * is a task associated with the perf event.
> +	 */
> +	if (p_event->attach_state != PERF_ATTACH_TASK)
> +		goto out;
> +
> +	/*
> +	 * This notifier gets called when perf trace event instance is
> +	 * added or removed. Until we can restrict this to events of
> +	 * interest in core, minimise the overhead below.
> +	 *
> +	 * Do we care about it? i.e., is there a callback for this
> +	 * trace point?
> +	 */
> +	setup_trace_fn = find_trace_callback(tp_event->tp->name);
> +	if (!setup_trace_fn)
> +		goto out;
> +
> +	pid = get_task_pid(p_event->hw.target, PIDTYPE_PID);
> +
> +	/*
> +	 * Does it match any of the VMs?
> +	 */
> +	spin_lock(&kvm_lock);
> +	list_for_each_entry(kvm, &vm_list, vm_list) {
> +		if (kvm->pid == pid) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	spin_unlock(&kvm_lock);
> +
> +	put_pid(pid);
> +	if (!found)
> +		goto out;
> +
> +	switch (event) {
> +	case TRACE_REG_PERF_OPEN:
> +		setup_trace_fn(kvm, true);
> +		break;
> +
> +	case TRACE_REG_PERF_CLOSE:
> +		setup_trace_fn(kvm, false);
> +		break;
> +	}
> +
> +out:
> +	return 0;
> +}
> +
> +static struct notifier_block kvm_perf_trace_notifier_block = {
> +	.notifier_call = kvm_perf_trace_notifier,
> +};
> +
> +int kvm_perf_trace_init(void)
> +{
> +	return perf_trace_notifier_register(&kvm_perf_trace_notifier_block);
> +}
> +
> +int kvm_perf_trace_teardown(void)
> +{
> +	return perf_trace_notifier_unregister(&kvm_perf_trace_notifier_block);
> +}
> -- 
> 2.8.1
> 

[toc] | [prev] | [next] | [standalone]

#1477584 — Re: [RFC v2 PATCH 3/7] KVM: arm/arm64: Register perf trace event notifier

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Mon, Sep 05, 2016 at 05:31:33PM +0100, Punit Agrawal wrote:
>> Register a notifier to track state changes of perf trace events.
>> 
>> The notifier will enable taking appropriate action for trace events
>> targeting VM.
>> 
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>
> Overall this looks reasonable, but I'm wondering if most if this logic
> should really go in virt/kvm/perf_trace.c and call into arch-specific
> hooks, similar to the way it works for preempt notifiers.
>
> On the other hand, if arm/arm64 are the only two architectures that are
> going to use this, creating stubs for the other architectures could be a
> bit tedious.

If the series looks to be headed in the right direction, I'll be adding
support for 32bit ARM host as well.

Most of the code here can be shared between arm and arm64. At that
point, it might be worth moving the logic here to a commong location
with the architecture code responsible for registering it's
kvm_trace_hooks as part of the initialisation (or something along those
lines).

I'll wait a bit for maintainer feedback on Patch 1 before re-spinning the
series. Patch 1 ultimately decides what the user facing interface for
this functionality looks like.

Thanks for your feedback on the series.

Punit

>
> Thanks,
> -Christoffer
>
>> ---
>>  arch/arm/include/asm/kvm_host.h   |   3 +
>>  arch/arm/kvm/arm.c                |   2 +
>>  arch/arm64/include/asm/kvm_host.h |   8 +++
>>  arch/arm64/kvm/Kconfig            |   4 ++
>>  arch/arm64/kvm/Makefile           |   1 +
>>  arch/arm64/kvm/perf_trace.c       | 122 ++++++++++++++++++++++++++++++++++++++
>>  6 files changed, 140 insertions(+)
>>  create mode 100644 arch/arm64/kvm/perf_trace.c
>> 
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index de338d9..609998e 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -280,6 +280,9 @@ static inline int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
>>  int kvm_perf_init(void);
>>  int kvm_perf_teardown(void);
>>  
>> +static inline int kvm_perf_trace_init(void) { return 0; }
>> +static inline int kvm_perf_trace_teardown(void) { return 0; }
>> +
>>  void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>>  
>>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 75f130e..e1b99c4 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -1220,6 +1220,7 @@ static int init_subsystems(void)
>>  		goto out;
>>  
>>  	kvm_perf_init();
>> +	kvm_perf_trace_init();
>>  	kvm_coproc_table_init();
>>  
>>  out:
>> @@ -1411,6 +1412,7 @@ out_err:
>>  void kvm_arch_exit(void)
>>  {
>>  	kvm_perf_teardown();
>> +	kvm_perf_trace_teardown();
>>  }
>>  
>>  static int arm_init(void)
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 3eda975..f6ff8e5 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -345,6 +345,14 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>  int kvm_perf_init(void);
>>  int kvm_perf_teardown(void);
>>  
>> +#if !defined(CONFIG_KVM_PERF_TRACE)
>> +static inline int kvm_perf_trace_init(void) { return 0; }
>> +static inline int kvm_perf_trace_teardown(void) { return 0; }
>> +#else
>> +int kvm_perf_trace_init(void);
>> +int kvm_perf_trace_teardown(void);
>> +#endif
>> +
>>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>>  
>>  static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>> index 9c9edc9..56e9537 100644
>> --- a/arch/arm64/kvm/Kconfig
>> +++ b/arch/arm64/kvm/Kconfig
>> @@ -19,6 +19,9 @@ if VIRTUALIZATION
>>  config KVM_ARM_VGIC_V3
>>  	bool
>>  
>> +config KVM_PERF_TRACE
>> +        bool
>> +
>>  config KVM
>>  	bool "Kernel-based Virtual Machine (KVM) support"
>>  	depends on OF
>> @@ -39,6 +42,7 @@ config KVM
>>  	select HAVE_KVM_MSI
>>  	select HAVE_KVM_IRQCHIP
>>  	select HAVE_KVM_IRQ_ROUTING
>> +	select KVM_PERF_TRACE if EVENT_TRACING && PERF_EVENTS
>>  	---help---
>>  	  Support hosting virtualized guest machines.
>>  	  We don't support KVM with 16K page tables yet, due to the multiple
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index 695eb3c..7d175e4 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -19,6 +19,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += emulate.o inject_fault.o regmap.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o
>> +kvm-$(CONFIG_KVM_PERF_TRACE) += perf_trace.o
>>  
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic.o
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-init.o
>> diff --git a/arch/arm64/kvm/perf_trace.c b/arch/arm64/kvm/perf_trace.c
>> new file mode 100644
>> index 0000000..8bacd18
>> --- /dev/null
>> +++ b/arch/arm64/kvm/perf_trace.c
>> @@ -0,0 +1,122 @@
>> +/*
>> + * Copyright (C) 2016 ARM Ltd.
>> + * Author: Punit Agrawal <punit.agrawal@arm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#include <linux/kvm_host.h>
>> +#include <linux/trace_events.h>
>> +
>> +typedef int (*perf_trace_callback_fn)(struct kvm *kvm, bool enable);
>> +
>> +struct kvm_trace_hook {
>> +	char *key;
>> +	perf_trace_callback_fn setup_fn;
>> +};
>> +
>> +static struct kvm_trace_hook trace_hook[] = {
>> +	{ },
>> +};
>> +
>> +static perf_trace_callback_fn find_trace_callback(const char *trace_key)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; trace_hook[i].key; i++)
>> +		if (!strcmp(trace_key, trace_hook[i].key))
>> +			return trace_hook[i].setup_fn;
>> +
>> +	return NULL;
>> +}
>> +
>> +static int kvm_perf_trace_notifier(struct notifier_block *nb,
>> +				   unsigned long event, void *data)
>> +{
>> +	struct perf_event *p_event = data;
>> +	struct trace_event_call *tp_event = p_event->tp_event;
>> +	perf_trace_callback_fn setup_trace_fn;
>> +	struct kvm *kvm = NULL;
>> +	struct pid *pid;
>> +	bool found = false;
>> +
>> +	/*
>> +	 * Is this a trace point?
>> +	 */
>> +	if (!(tp_event->flags & TRACE_EVENT_FL_TRACEPOINT))
>> +		goto out;
>> +
>> +	/*
>> +	 * We'll get here for events we care to monitor for KVM. As we
>> +	 * only care about events attached to a VM, check that there
>> +	 * is a task associated with the perf event.
>> +	 */
>> +	if (p_event->attach_state != PERF_ATTACH_TASK)
>> +		goto out;
>> +
>> +	/*
>> +	 * This notifier gets called when perf trace event instance is
>> +	 * added or removed. Until we can restrict this to events of
>> +	 * interest in core, minimise the overhead below.
>> +	 *
>> +	 * Do we care about it? i.e., is there a callback for this
>> +	 * trace point?
>> +	 */
>> +	setup_trace_fn = find_trace_callback(tp_event->tp->name);
>> +	if (!setup_trace_fn)
>> +		goto out;
>> +
>> +	pid = get_task_pid(p_event->hw.target, PIDTYPE_PID);
>> +
>> +	/*
>> +	 * Does it match any of the VMs?
>> +	 */
>> +	spin_lock(&kvm_lock);
>> +	list_for_each_entry(kvm, &vm_list, vm_list) {
>> +		if (kvm->pid == pid) {
>> +			found = true;
>> +			break;
>> +		}
>> +	}
>> +	spin_unlock(&kvm_lock);
>> +
>> +	put_pid(pid);
>> +	if (!found)
>> +		goto out;
>> +
>> +	switch (event) {
>> +	case TRACE_REG_PERF_OPEN:
>> +		setup_trace_fn(kvm, true);
>> +		break;
>> +
>> +	case TRACE_REG_PERF_CLOSE:
>> +		setup_trace_fn(kvm, false);
>> +		break;
>> +	}
>> +
>> +out:
>> +	return 0;
>> +}
>> +
>> +static struct notifier_block kvm_perf_trace_notifier_block = {
>> +	.notifier_call = kvm_perf_trace_notifier,
>> +};
>> +
>> +int kvm_perf_trace_init(void)
>> +{
>> +	return perf_trace_notifier_register(&kvm_perf_trace_notifier_block);
>> +}
>> +
>> +int kvm_perf_trace_teardown(void)
>> +{
>> +	return perf_trace_notifier_unregister(&kvm_perf_trace_notifier_block);
>> +}
>> -- 
>> 2.8.1
>> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[toc] | [prev] | [next] | [standalone]

#1476595 — [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process

Userspace tools such as perf can be used to profile individual
processes.

Track the PID of the virtual machine process to match profiling requests
targeted at it. This can be used to take appropriate action to enable
the requested profiling operations for the VMs of interest.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/kvm_host.h | 1 +
 virt/kvm/kvm_main.c      | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 9c28b4d..7c42c94 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -374,6 +374,7 @@ struct kvm_memslots {
 struct kvm {
 	spinlock_t mmu_lock;
 	struct mutex slots_lock;
+	struct pid *pid;
 	struct mm_struct *mm; /* userspace tied to this vm */
 	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
 	struct srcu_struct srcu;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1950782..ab2535a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -613,6 +613,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	spin_lock_init(&kvm->mmu_lock);
 	atomic_inc(&current->mm->mm_count);
 	kvm->mm = current->mm;
+	kvm->pid = get_task_pid(current, PIDTYPE_PID);
 	kvm_eventfd_init(kvm);
 	mutex_init(&kvm->lock);
 	mutex_init(&kvm->irq_lock);
@@ -712,6 +713,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 	int i;
 	struct mm_struct *mm = kvm->mm;
 
+	put_pid(kvm->pid);
 	kvm_destroy_vm_debugfs(kvm);
 	kvm_arch_sync_events(kvm);
 	spin_lock(&kvm_lock);
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1477096 — Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process

On Mon, Sep 05, 2016 at 05:31:32PM +0100, Punit Agrawal wrote:
> Userspace tools such as perf can be used to profile individual
> processes.
> 
> Track the PID of the virtual machine process to match profiling requests
> targeted at it. This can be used to take appropriate action to enable
> the requested profiling operations for the VMs of interest.
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  include/linux/kvm_host.h | 1 +
>  virt/kvm/kvm_main.c      | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 9c28b4d..7c42c94 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -374,6 +374,7 @@ struct kvm_memslots {
>  struct kvm {
>  	spinlock_t mmu_lock;
>  	struct mutex slots_lock;
> +	struct pid *pid;
>  	struct mm_struct *mm; /* userspace tied to this vm */
>  	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
>  	struct srcu_struct srcu;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 1950782..ab2535a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -613,6 +613,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
>  	spin_lock_init(&kvm->mmu_lock);
>  	atomic_inc(&current->mm->mm_count);
>  	kvm->mm = current->mm;
> +	kvm->pid = get_task_pid(current, PIDTYPE_PID);

How dooes this deal with threading?  Is the idea that the user by
specifying the main thread's pid will enable trapping for all vcpu
threads belonging to that VM?

>  	kvm_eventfd_init(kvm);
>  	mutex_init(&kvm->lock);
>  	mutex_init(&kvm->irq_lock);
> @@ -712,6 +713,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
>  	int i;
>  	struct mm_struct *mm = kvm->mm;
>  
> +	put_pid(kvm->pid);
>  	kvm_destroy_vm_debugfs(kvm);
>  	kvm_arch_sync_events(kvm);
>  	spin_lock(&kvm_lock);
> -- 
> 2.8.1
> 

[toc] | [prev] | [next] | [standalone]

#1477233 — Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process

Hi Christoffer,

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Mon, Sep 05, 2016 at 05:31:32PM +0100, Punit Agrawal wrote:
>> Userspace tools such as perf can be used to profile individual
>> processes.
>> 
>> Track the PID of the virtual machine process to match profiling requests
>> targeted at it. This can be used to take appropriate action to enable
>> the requested profiling operations for the VMs of interest.
>> 
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  include/linux/kvm_host.h | 1 +
>>  virt/kvm/kvm_main.c      | 2 ++
>>  2 files changed, 3 insertions(+)
>> 
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 9c28b4d..7c42c94 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -374,6 +374,7 @@ struct kvm_memslots {
>>  struct kvm {
>>  	spinlock_t mmu_lock;
>>  	struct mutex slots_lock;
>> +	struct pid *pid;
>>  	struct mm_struct *mm; /* userspace tied to this vm */
>>  	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
>>  	struct srcu_struct srcu;
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 1950782..ab2535a 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -613,6 +613,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
>>  	spin_lock_init(&kvm->mmu_lock);
>>  	atomic_inc(&current->mm->mm_count);
>>  	kvm->mm = current->mm;
>> +	kvm->pid = get_task_pid(current, PIDTYPE_PID);
>
> How dooes this deal with threading?  Is the idea that the user by
> specifying the main thread's pid will enable trapping for all vcpu
> threads belonging to that VM?

Yes that's correct - specifying the main thread PID will enable trapping
for the VM (all vcpus).

I am happy to move to a more suitable identifier if available.

Thanks,
Punit

>
>>  	kvm_eventfd_init(kvm);
>>  	mutex_init(&kvm->lock);
>>  	mutex_init(&kvm->irq_lock);
>> @@ -712,6 +713,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
>>  	int i;
>>  	struct mm_struct *mm = kvm->mm;
>>  
>> +	put_pid(kvm->pid);
>>  	kvm_destroy_vm_debugfs(kvm);
>>  	kvm_arch_sync_events(kvm);
>>  	spin_lock(&kvm_lock);
>> -- 
>> 2.8.1
>> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[toc] | [prev] | [next] | [standalone]

#1477249 — Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process

On Tue, Sep 06, 2016 at 10:51:27AM +0100, Punit Agrawal wrote:
> Hi Christoffer,
> 
> Christoffer Dall <christoffer.dall@linaro.org> writes:
> 
> > On Mon, Sep 05, 2016 at 05:31:32PM +0100, Punit Agrawal wrote:
> >> Userspace tools such as perf can be used to profile individual
> >> processes.
> >> 
> >> Track the PID of the virtual machine process to match profiling requests
> >> targeted at it. This can be used to take appropriate action to enable
> >> the requested profiling operations for the VMs of interest.
> >> 
> >> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  include/linux/kvm_host.h | 1 +
> >>  virt/kvm/kvm_main.c      | 2 ++
> >>  2 files changed, 3 insertions(+)
> >> 
> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> >> index 9c28b4d..7c42c94 100644
> >> --- a/include/linux/kvm_host.h
> >> +++ b/include/linux/kvm_host.h
> >> @@ -374,6 +374,7 @@ struct kvm_memslots {
> >>  struct kvm {
> >>  	spinlock_t mmu_lock;
> >>  	struct mutex slots_lock;
> >> +	struct pid *pid;
> >>  	struct mm_struct *mm; /* userspace tied to this vm */
> >>  	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
> >>  	struct srcu_struct srcu;
> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >> index 1950782..ab2535a 100644
> >> --- a/virt/kvm/kvm_main.c
> >> +++ b/virt/kvm/kvm_main.c
> >> @@ -613,6 +613,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
> >>  	spin_lock_init(&kvm->mmu_lock);
> >>  	atomic_inc(&current->mm->mm_count);
> >>  	kvm->mm = current->mm;
> >> +	kvm->pid = get_task_pid(current, PIDTYPE_PID);
> >
> > How dooes this deal with threading?  Is the idea that the user by
> > specifying the main thread's pid will enable trapping for all vcpu
> > threads belonging to that VM?
> 
> Yes that's correct - specifying the main thread PID will enable trapping
> for the VM (all vcpus).
> 
> I am happy to move to a more suitable identifier if available.
> 

What is the 'main thread' ?

Does something mandate that the VM is created by the thread group
leader?  If not, is it not a bit strange from a user perspective, that
you have to find the specific subthread pid that created the vm to
enable this tracing for all vcpu threads and that the tgid doesn't work
in this case?

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1477279 — Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Tue, Sep 06, 2016 at 10:51:27AM +0100, Punit Agrawal wrote:
>> Hi Christoffer,
>> 
>> Christoffer Dall <christoffer.dall@linaro.org> writes:
>> 
>> > On Mon, Sep 05, 2016 at 05:31:32PM +0100, Punit Agrawal wrote:
>> >> Userspace tools such as perf can be used to profile individual
>> >> processes.
>> >> 
>> >> Track the PID of the virtual machine process to match profiling requests
>> >> targeted at it. This can be used to take appropriate action to enable
>> >> the requested profiling operations for the VMs of interest.
>> >> 
>> >> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> >> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> >> ---
>> >>  include/linux/kvm_host.h | 1 +
>> >>  virt/kvm/kvm_main.c      | 2 ++
>> >>  2 files changed, 3 insertions(+)
>> >> 
>> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> >> index 9c28b4d..7c42c94 100644
>> >> --- a/include/linux/kvm_host.h
>> >> +++ b/include/linux/kvm_host.h
>> >> @@ -374,6 +374,7 @@ struct kvm_memslots {
>> >>  struct kvm {
>> >>  	spinlock_t mmu_lock;
>> >>  	struct mutex slots_lock;
>> >> +	struct pid *pid;
>> >>  	struct mm_struct *mm; /* userspace tied to this vm */
>> >>  	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
>> >>  	struct srcu_struct srcu;
>> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> >> index 1950782..ab2535a 100644
>> >> --- a/virt/kvm/kvm_main.c
>> >> +++ b/virt/kvm/kvm_main.c
>> >> @@ -613,6 +613,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
>> >>  	spin_lock_init(&kvm->mmu_lock);
>> >>  	atomic_inc(&current->mm->mm_count);
>> >>  	kvm->mm = current->mm;
>> >> +	kvm->pid = get_task_pid(current, PIDTYPE_PID);
>> >
>> > How dooes this deal with threading?  Is the idea that the user by
>> > specifying the main thread's pid will enable trapping for all vcpu
>> > threads belonging to that VM?
>> 
>> Yes that's correct - specifying the main thread PID will enable trapping
>> for the VM (all vcpus).
>> 
>> I am happy to move to a more suitable identifier if available.
>> 
>
> What is the 'main thread' ?
>
> Does something mandate that the VM is created by the thread group
> leader?  If not, is it not a bit strange from a user perspective, that
> you have to find the specific subthread pid that created the vm to
> enable this tracing for all vcpu threads and that the tgid doesn't work
> in this case?

Let me correct my terminology usage - the value recorded above (and used
to identify the VM) should be the tgid. It is confusing because 'ps'
reports it as pid.

I picked the value as existing KVM code already uses the PID of the
creating task (see kvm_create_vm_debugfs) to export VM statistics in
debugfs.

If I've got this wrong, then kvm_create_vm_debugfs also likely needs an
update.

What do you think?

>
> Thanks,
> -Christoffer
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[toc] | [prev] | [next] | [standalone]

#1477282 — Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process

On Tue, Sep 06, 2016 at 12:07:59PM +0100, Punit Agrawal wrote:
> Christoffer Dall <christoffer.dall@linaro.org> writes:
> 
> > On Tue, Sep 06, 2016 at 10:51:27AM +0100, Punit Agrawal wrote:
> >> Hi Christoffer,
> >> 
> >> Christoffer Dall <christoffer.dall@linaro.org> writes:
> >> 
> >> > On Mon, Sep 05, 2016 at 05:31:32PM +0100, Punit Agrawal wrote:
> >> >> Userspace tools such as perf can be used to profile individual
> >> >> processes.
> >> >> 
> >> >> Track the PID of the virtual machine process to match profiling requests
> >> >> targeted at it. This can be used to take appropriate action to enable
> >> >> the requested profiling operations for the VMs of interest.
> >> >> 
> >> >> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> >> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> >> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> >> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> >> ---
> >> >>  include/linux/kvm_host.h | 1 +
> >> >>  virt/kvm/kvm_main.c      | 2 ++
> >> >>  2 files changed, 3 insertions(+)
> >> >> 
> >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> >> >> index 9c28b4d..7c42c94 100644
> >> >> --- a/include/linux/kvm_host.h
> >> >> +++ b/include/linux/kvm_host.h
> >> >> @@ -374,6 +374,7 @@ struct kvm_memslots {
> >> >>  struct kvm {
> >> >>  	spinlock_t mmu_lock;
> >> >>  	struct mutex slots_lock;
> >> >> +	struct pid *pid;
> >> >>  	struct mm_struct *mm; /* userspace tied to this vm */
> >> >>  	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
> >> >>  	struct srcu_struct srcu;
> >> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >> >> index 1950782..ab2535a 100644
> >> >> --- a/virt/kvm/kvm_main.c
> >> >> +++ b/virt/kvm/kvm_main.c
> >> >> @@ -613,6 +613,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
> >> >>  	spin_lock_init(&kvm->mmu_lock);
> >> >>  	atomic_inc(&current->mm->mm_count);
> >> >>  	kvm->mm = current->mm;
> >> >> +	kvm->pid = get_task_pid(current, PIDTYPE_PID);
> >> >
> >> > How dooes this deal with threading?  Is the idea that the user by
> >> > specifying the main thread's pid will enable trapping for all vcpu
> >> > threads belonging to that VM?
> >> 
> >> Yes that's correct - specifying the main thread PID will enable trapping
> >> for the VM (all vcpus).
> >> 
> >> I am happy to move to a more suitable identifier if available.
> >> 
> >
> > What is the 'main thread' ?
> >
> > Does something mandate that the VM is created by the thread group
> > leader?  If not, is it not a bit strange from a user perspective, that
> > you have to find the specific subthread pid that created the vm to
> > enable this tracing for all vcpu threads and that the tgid doesn't work
> > in this case?
> 
> Let me correct my terminology usage - the value recorded above (and used
> to identify the VM) should be the tgid. It is confusing because 'ps'
> reports it as pid.
> 
> I picked the value as existing KVM code already uses the PID of the
> creating task (see kvm_create_vm_debugfs) to export VM statistics in
> debugfs.
> 
> If I've got this wrong, then kvm_create_vm_debugfs also likely needs an
> update.
> 
> What do you think?
> 
When you do get_task_pid(current, PIDTYPE_PID) it actually gets the
kernel view of a PID which is the thead-id from userspace's point of
view, right?

I don't see why this has to be the same as the debugfs code, as there it
makes potentially more sense to thread-specific, but for your case, are
you not targeting the behavior that a user can do "ps aux | grep qemu"
or whatever, and then set tracing for the reported PID (which is
actually a tgid)?

If this is indeed the case, then I don't think the current code supports
this if QEMU was ever changed to create the VM with a different thread
than the tgl.

That being said, I'm typically wrong when I talk about userspace.

-Christoffer

[toc] | [prev] | [next] | [standalone]

#1477540 — Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Tue, Sep 06, 2016 at 12:07:59PM +0100, Punit Agrawal wrote:
>> Christoffer Dall <christoffer.dall@linaro.org> writes:
>> 
>> > On Tue, Sep 06, 2016 at 10:51:27AM +0100, Punit Agrawal wrote:
>> >> Hi Christoffer,
>> >> 
>> >> Christoffer Dall <christoffer.dall@linaro.org> writes:
>> >> 
>> >> > On Mon, Sep 05, 2016 at 05:31:32PM +0100, Punit Agrawal wrote:
>> >> >> Userspace tools such as perf can be used to profile individual
>> >> >> processes.
>> >> >> 
>> >> >> Track the PID of the virtual machine process to match profiling requests
>> >> >> targeted at it. This can be used to take appropriate action to enable
>> >> >> the requested profiling operations for the VMs of interest.
>> >> >> 
>> >> >> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> >> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> >> >> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>> >> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> >> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> >> >> ---
>> >> >>  include/linux/kvm_host.h | 1 +
>> >> >>  virt/kvm/kvm_main.c      | 2 ++
>> >> >>  2 files changed, 3 insertions(+)
>> >> >> 
>> >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> >> >> index 9c28b4d..7c42c94 100644
>> >> >> --- a/include/linux/kvm_host.h
>> >> >> +++ b/include/linux/kvm_host.h
>> >> >> @@ -374,6 +374,7 @@ struct kvm_memslots {
>> >> >>  struct kvm {
>> >> >>  	spinlock_t mmu_lock;
>> >> >>  	struct mutex slots_lock;
>> >> >> +	struct pid *pid;
>> >> >>  	struct mm_struct *mm; /* userspace tied to this vm */
>> >> >>  	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
>> >> >>  	struct srcu_struct srcu;
>> >> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> >> >> index 1950782..ab2535a 100644
>> >> >> --- a/virt/kvm/kvm_main.c
>> >> >> +++ b/virt/kvm/kvm_main.c
>> >> >> @@ -613,6 +613,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
>> >> >>  	spin_lock_init(&kvm->mmu_lock);
>> >> >>  	atomic_inc(&current->mm->mm_count);
>> >> >>  	kvm->mm = current->mm;
>> >> >> +	kvm->pid = get_task_pid(current, PIDTYPE_PID);
>> >> >
>> >> > How dooes this deal with threading?  Is the idea that the user by
>> >> > specifying the main thread's pid will enable trapping for all vcpu
>> >> > threads belonging to that VM?
>> >> 
>> >> Yes that's correct - specifying the main thread PID will enable trapping
>> >> for the VM (all vcpus).
>> >> 
>> >> I am happy to move to a more suitable identifier if available.
>> >> 
>> >
>> > What is the 'main thread' ?
>> >
>> > Does something mandate that the VM is created by the thread group
>> > leader?  If not, is it not a bit strange from a user perspective, that
>> > you have to find the specific subthread pid that created the vm to
>> > enable this tracing for all vcpu threads and that the tgid doesn't work
>> > in this case?
>> 
>> Let me correct my terminology usage - the value recorded above (and used
>> to identify the VM) should be the tgid. It is confusing because 'ps'
>> reports it as pid.
>> 
>> I picked the value as existing KVM code already uses the PID of the
>> creating task (see kvm_create_vm_debugfs) to export VM statistics in
>> debugfs.
>> 
>> If I've got this wrong, then kvm_create_vm_debugfs also likely needs an
>> update.
>> 
>> What do you think?
>> 
> When you do get_task_pid(current, PIDTYPE_PID) it actually gets the
> kernel view of a PID which is the thead-id from userspace's point of
> view, right?

That makes sense. It seems to works here because the pid of the first
task is also the tgid of the group. And I reckon it's the same
assumption being made with debugfs code (more below).

I've changed the first argument of the call to get_task_pid to
current->group_leader.

>
> I don't see why this has to be the same as the debugfs code, as there it
> makes potentially more sense to thread-specific, but for your case, are
> you not targeting the behavior that a user can do "ps aux | grep qemu"
> or whatever, and then set tracing for the reported PID (which is
> actually a tgid)?

The debugfs stats are not thread (vcpu) specific but for the VM.

Both values, debugfs and here, are being used to represent the VM to the
user. A mismatch in these identifiers will be very confusing.

If you agree, I can separately send a patch to address this for VM
debugfs directory as well.

>
> If this is indeed the case, then I don't think the current code supports
> this if QEMU was ever changed to create the VM with a different thread
> than the tgl.
>
> That being said, I'm typically wrong when I talk about userspace.
>
> -Christoffer
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[toc] | [prev] | [next] | [standalone]

#1477645 — Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process

On Tue, Sep 06, 2016 at 04:22:17PM +0100, Punit Agrawal wrote:
> Christoffer Dall <christoffer.dall@linaro.org> writes:
> 
> > On Tue, Sep 06, 2016 at 12:07:59PM +0100, Punit Agrawal wrote:
> >> Christoffer Dall <christoffer.dall@linaro.org> writes:
> >> 
> >> > On Tue, Sep 06, 2016 at 10:51:27AM +0100, Punit Agrawal wrote:
> >> >> Hi Christoffer,
> >> >> 
> >> >> Christoffer Dall <christoffer.dall@linaro.org> writes:
> >> >> 
> >> >> > On Mon, Sep 05, 2016 at 05:31:32PM +0100, Punit Agrawal wrote:
> >> >> >> Userspace tools such as perf can be used to profile individual
> >> >> >> processes.
> >> >> >> 
> >> >> >> Track the PID of the virtual machine process to match profiling requests
> >> >> >> targeted at it. This can be used to take appropriate action to enable
> >> >> >> the requested profiling operations for the VMs of interest.
> >> >> >> 
> >> >> >> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> >> >> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> >> >> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> >> >> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> >> >> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> >> >> ---
> >> >> >>  include/linux/kvm_host.h | 1 +
> >> >> >>  virt/kvm/kvm_main.c      | 2 ++
> >> >> >>  2 files changed, 3 insertions(+)
> >> >> >> 
> >> >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> >> >> >> index 9c28b4d..7c42c94 100644
> >> >> >> --- a/include/linux/kvm_host.h
> >> >> >> +++ b/include/linux/kvm_host.h
> >> >> >> @@ -374,6 +374,7 @@ struct kvm_memslots {
> >> >> >>  struct kvm {
> >> >> >>  	spinlock_t mmu_lock;
> >> >> >>  	struct mutex slots_lock;
> >> >> >> +	struct pid *pid;
> >> >> >>  	struct mm_struct *mm; /* userspace tied to this vm */
> >> >> >>  	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
> >> >> >>  	struct srcu_struct srcu;
> >> >> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >> >> >> index 1950782..ab2535a 100644
> >> >> >> --- a/virt/kvm/kvm_main.c
> >> >> >> +++ b/virt/kvm/kvm_main.c
> >> >> >> @@ -613,6 +613,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
> >> >> >>  	spin_lock_init(&kvm->mmu_lock);
> >> >> >>  	atomic_inc(&current->mm->mm_count);
> >> >> >>  	kvm->mm = current->mm;
> >> >> >> +	kvm->pid = get_task_pid(current, PIDTYPE_PID);
> >> >> >
> >> >> > How dooes this deal with threading?  Is the idea that the user by
> >> >> > specifying the main thread's pid will enable trapping for all vcpu
> >> >> > threads belonging to that VM?
> >> >> 
> >> >> Yes that's correct - specifying the main thread PID will enable trapping
> >> >> for the VM (all vcpus).
> >> >> 
> >> >> I am happy to move to a more suitable identifier if available.
> >> >> 
> >> >
> >> > What is the 'main thread' ?
> >> >
> >> > Does something mandate that the VM is created by the thread group
> >> > leader?  If not, is it not a bit strange from a user perspective, that
> >> > you have to find the specific subthread pid that created the vm to
> >> > enable this tracing for all vcpu threads and that the tgid doesn't work
> >> > in this case?
> >> 
> >> Let me correct my terminology usage - the value recorded above (and used
> >> to identify the VM) should be the tgid. It is confusing because 'ps'
> >> reports it as pid.
> >> 
> >> I picked the value as existing KVM code already uses the PID of the
> >> creating task (see kvm_create_vm_debugfs) to export VM statistics in
> >> debugfs.
> >> 
> >> If I've got this wrong, then kvm_create_vm_debugfs also likely needs an
> >> update.
> >> 
> >> What do you think?
> >> 
> > When you do get_task_pid(current, PIDTYPE_PID) it actually gets the
> > kernel view of a PID which is the thead-id from userspace's point of
> > view, right?
> 
> That makes sense. It seems to works here because the pid of the first
> task is also the tgid of the group. And I reckon it's the same
> assumption being made with debugfs code (more below).

That is probably the implementation of all QEMU versions and kvmtool
versions out there.

> 
> I've changed the first argument of the call to get_task_pid to
> current->group_leader.
> 
> >
> > I don't see why this has to be the same as the debugfs code, as there it
> > makes potentially more sense to thread-specific, but for your case, are
> > you not targeting the behavior that a user can do "ps aux | grep qemu"
> > or whatever, and then set tracing for the reported PID (which is
> > actually a tgid)?
> 
> The debugfs stats are not thread (vcpu) specific but for the VM.
> 
> Both values, debugfs and here, are being used to represent the VM to the
> user. A mismatch in these identifiers will be very confusing.
> 
> If you agree, I can separately send a patch to address this for VM
> debugfs directory as well.
> 

I don't know how the debugfs stuff is used or was intended, so I really
can't speak for that.  It seems less weird to me with debugfs, because I
imagine it can be used by simply looking at what exists in the debugfs
directory and mapping that to a VM.

In your case, there's a clear expectation from the user that using the
tgid should cover this VM, and it will be weird if that's not the case.

-Christoffer

[toc] | [prev] | [next] | [standalone]

#1477667 — Re: [RFC v2 PATCH 2/7] KVM: Track the pid of the VM process

Christoffer Dall <christoffer.dall@linaro.org> writes:

> On Tue, Sep 06, 2016 at 04:22:17PM +0100, Punit Agrawal wrote:
>> Christoffer Dall <christoffer.dall@linaro.org> writes:
>> 
>> > On Tue, Sep 06, 2016 at 12:07:59PM +0100, Punit Agrawal wrote:
>> >> Christoffer Dall <christoffer.dall@linaro.org> writes:
>> >> 
>> >> > On Tue, Sep 06, 2016 at 10:51:27AM +0100, Punit Agrawal wrote:
>> >> >> Hi Christoffer,
>> >> >> 
>> >> >> Christoffer Dall <christoffer.dall@linaro.org> writes:
>> >> >> 
>> >> >> > On Mon, Sep 05, 2016 at 05:31:32PM +0100, Punit Agrawal wrote:
>> >> >> >> Userspace tools such as perf can be used to profile individual
>> >> >> >> processes.
>> >> >> >> 
>> >> >> >> Track the PID of the virtual machine process to match profiling requests
>> >> >> >> targeted at it. This can be used to take appropriate action to enable
>> >> >> >> the requested profiling operations for the VMs of interest.
>> >> >> >> 
>> >> >> >> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> >> >> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> >> >> >> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>> >> >> >> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> >> >> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> >> >> >> ---
>> >> >> >>  include/linux/kvm_host.h | 1 +
>> >> >> >>  virt/kvm/kvm_main.c      | 2 ++
>> >> >> >>  2 files changed, 3 insertions(+)
>> >> >> >> 
>> >> >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> >> >> >> index 9c28b4d..7c42c94 100644
>> >> >> >> --- a/include/linux/kvm_host.h
>> >> >> >> +++ b/include/linux/kvm_host.h
>> >> >> >> @@ -374,6 +374,7 @@ struct kvm_memslots {
>> >> >> >>  struct kvm {
>> >> >> >>  	spinlock_t mmu_lock;
>> >> >> >>  	struct mutex slots_lock;
>> >> >> >> +	struct pid *pid;
>> >> >> >>  	struct mm_struct *mm; /* userspace tied to this vm */
>> >> >> >>  	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
>> >> >> >>  	struct srcu_struct srcu;
>> >> >> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> >> >> >> index 1950782..ab2535a 100644
>> >> >> >> --- a/virt/kvm/kvm_main.c
>> >> >> >> +++ b/virt/kvm/kvm_main.c
>> >> >> >> @@ -613,6 +613,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
>> >> >> >>  	spin_lock_init(&kvm->mmu_lock);
>> >> >> >>  	atomic_inc(&current->mm->mm_count);
>> >> >> >>  	kvm->mm = current->mm;
>> >> >> >> +	kvm->pid = get_task_pid(current, PIDTYPE_PID);
>> >> >> >
>> >> >> > How dooes this deal with threading?  Is the idea that the user by
>> >> >> > specifying the main thread's pid will enable trapping for all vcpu
>> >> >> > threads belonging to that VM?
>> >> >> 
>> >> >> Yes that's correct - specifying the main thread PID will enable trapping
>> >> >> for the VM (all vcpus).
>> >> >> 
>> >> >> I am happy to move to a more suitable identifier if available.
>> >> >> 
>> >> >
>> >> > What is the 'main thread' ?
>> >> >
>> >> > Does something mandate that the VM is created by the thread group
>> >> > leader?  If not, is it not a bit strange from a user perspective, that
>> >> > you have to find the specific subthread pid that created the vm to
>> >> > enable this tracing for all vcpu threads and that the tgid doesn't work
>> >> > in this case?
>> >> 
>> >> Let me correct my terminology usage - the value recorded above (and used
>> >> to identify the VM) should be the tgid. It is confusing because 'ps'
>> >> reports it as pid.
>> >> 
>> >> I picked the value as existing KVM code already uses the PID of the
>> >> creating task (see kvm_create_vm_debugfs) to export VM statistics in
>> >> debugfs.
>> >> 
>> >> If I've got this wrong, then kvm_create_vm_debugfs also likely needs an
>> >> update.
>> >> 
>> >> What do you think?
>> >> 
>> > When you do get_task_pid(current, PIDTYPE_PID) it actually gets the
>> > kernel view of a PID which is the thead-id from userspace's point of
>> > view, right?
>> 
>> That makes sense. It seems to works here because the pid of the first
>> task is also the tgid of the group. And I reckon it's the same
>> assumption being made with debugfs code (more below).
>
> That is probably the implementation of all QEMU versions and kvmtool
> versions out there.
>
>> 
>> I've changed the first argument of the call to get_task_pid to
>> current->group_leader.
>> 
>> >
>> > I don't see why this has to be the same as the debugfs code, as there it
>> > makes potentially more sense to thread-specific, but for your case, are
>> > you not targeting the behavior that a user can do "ps aux | grep qemu"
>> > or whatever, and then set tracing for the reported PID (which is
>> > actually a tgid)?
>> 
>> The debugfs stats are not thread (vcpu) specific but for the VM.
>> 
>> Both values, debugfs and here, are being used to represent the VM to the
>> user. A mismatch in these identifiers will be very confusing.
>> 
>> If you agree, I can separately send a patch to address this for VM
>> debugfs directory as well.
>> 
>
> I don't know how the debugfs stuff is used or was intended, so I really
> can't speak for that.  It seems less weird to me with debugfs, because I
> imagine it can be used by simply looking at what exists in the debugfs
> directory and mapping that to a VM.

Ok, I'll let debugfs stuff be as is then.

>
> In your case, there's a clear expectation from the user that using the
> tgid should cover this VM, and it will be weird if that's not the
> case.

Right. Switching over to current->group_leader should avert problems if
userspace tools do things differently.

Thanks,
Punit

>
> -Christoffer
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web