[PATCH] KVM: Create debugfs statistics for each VM

[PATCH] KVM: Create debugfs statistics for each VM

[Janosch Frank]

The following patch adds debugfs statistics on a per-vm basis, so
consumers, that do not want to use tracefs vm statistics or have
multiple VMs per pid, can fetch them.

This design was chosen, as qemu does create two VMs on arm for a very
short time, although the kvm api documentation states that it is
unsupported.

The first version went through s390-kvm but was reverted before being
merged into mainline.

Janosch Frank (1):
  KVM: Create debugfs statistics for each VM

 include/linux/kvm_host.h |   7 ++
 virt/kvm/kvm_main.c      | 187 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 184 insertions(+), 10 deletions(-)

-- 
2.3.0

[PATCH] KVM: Create debugfs statistics for each VM

[Janosch Frank]

KVM statistics for VMs (no. of exits, halts and other special
instructions) are currently only available in a summarized manner for
all VMs. They are exported to userland through files in the kvm
debugfs directory and used for performance monitoring, as well as VM
problem detection with helper tools like kvm_stat. If a VM has
problems and therefore creates a large number of exits, one can not
easily find out which one it is, as there is no VM specific data.

This patch adds a kvm debugfs subdirectory for each VM, which is named
after its pid and file descriptor. They contain the same kind of files
that are already in the kvm debugfs directory, but the data that is
exported through them is now VM specific.

CC: Dan Carpenter <dan.carpenter@oracle.com> [includes fixes by Dan Carpenter]
Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com>
---
 include/linux/kvm_host.h |   7 ++
 virt/kvm/kvm_main.c      | 187 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 184 insertions(+), 10 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f707f74..3f237e1 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -406,6 +406,8 @@ struct kvm {
 #endif
 	long tlbs_dirty;
 	struct list_head devices;
+	struct dentry *debugfs_dentry;
+	struct kvm_stat_data **debugfs_stat_data;
 };
 
 #define kvm_err(fmt, ...) \
@@ -982,6 +984,11 @@ enum kvm_stat_kind {
 	KVM_STAT_VCPU,
 };
 
+struct kvm_stat_data {
+	int offset;
+	struct kvm *kvm;
+};
+
 struct kvm_stats_debugfs_item {
 	const char *name;
 	int offset;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 314c777..d530e60 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -63,6 +63,9 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/kvm.h>
 
+/* Worst case buffer size needed for holding an integer. */
+#define ITOA_MAX_LEN 12
+
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
 
@@ -100,6 +103,9 @@ static __read_mostly struct preempt_ops kvm_preempt_ops;
 struct dentry *kvm_debugfs_dir;
 EXPORT_SYMBOL_GPL(kvm_debugfs_dir);
 
+static int kvm_debugfs_num_entries;
+static const struct file_operations *stat_fops_per_vm[];
+
 static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl,
 			   unsigned long arg);
 #ifdef CONFIG_KVM_COMPAT
@@ -529,6 +535,58 @@ static void kvm_free_memslots(struct kvm *kvm, struct kvm_memslots *slots)
 	kvfree(slots);
 }
 
+static void kvm_destroy_vm_debugfs(struct kvm *kvm)
+{
+	int i;
+
+	if (!kvm->debugfs_dentry)
+		return;
+
+	debugfs_remove_recursive(kvm->debugfs_dentry);
+
+	for (i = 0; i < kvm_debugfs_num_entries; i++)
+		kfree(kvm->debugfs_stat_data[i]);
+	kfree(kvm->debugfs_stat_data);
+}
+
+static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
+{
+	char dir_name[ITOA_MAX_LEN * 2];
+	struct kvm_stat_data *stat_data;
+	struct kvm_stats_debugfs_item *p;
+
+	if (!debugfs_initialized())
+		return 0;
+
+	snprintf(dir_name, sizeof(dir_name), "%d-%d", task_pid_nr(current), fd);
+	kvm->debugfs_dentry = debugfs_create_dir(dir_name,
+						 kvm_debugfs_dir);
+	if (!kvm->debugfs_dentry)
+		return -ENOMEM;
+
+	kvm->debugfs_stat_data = kcalloc(kvm_debugfs_num_entries,
+					 sizeof(*kvm->debugfs_stat_data),
+					 GFP_KERNEL);
+	if (!kvm->debugfs_stat_data)
+		return -ENOMEM;
+
+	for (p = debugfs_entries; p->name; p++) {
+		stat_data = kzalloc(sizeof(*stat_data), GFP_KERNEL);
+		if (!stat_data)
+			return -ENOMEM;
+
+		stat_data->kvm = kvm;
+		stat_data->offset = p->offset;
+		kvm->debugfs_stat_data[p - debugfs_entries] = stat_data;
+		if (!debugfs_create_file(p->name, 0444,
+					 kvm->debugfs_dentry,
+					 stat_data,
+					 stat_fops_per_vm[p->kind]))
+			return -ENOMEM;
+	}
+	return 0;
+}
+
 static struct kvm *kvm_create_vm(unsigned long type)
 {
 	int r, i;
@@ -636,6 +694,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 	int i;
 	struct mm_struct *mm = kvm->mm;
 
+	kvm_destroy_vm_debugfs(kvm);
 	kvm_arch_sync_events(kvm);
 	spin_lock(&kvm_lock);
 	list_del(&kvm->vm_list);
@@ -2962,8 +3021,15 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	}
 #endif
 	r = anon_inode_getfd("kvm-vm", &kvm_vm_fops, kvm, O_RDWR | O_CLOEXEC);
-	if (r < 0)
+	if (r < 0) {
 		kvm_put_kvm(kvm);
+		return r;
+	}
+
+	if (kvm_create_vm_debugfs(kvm, r) < 0) {
+		kvm_put_kvm(kvm);
+		return -ENOMEM;
+	}
 
 	return r;
 }
@@ -3388,15 +3454,114 @@ static struct notifier_block kvm_cpu_notifier = {
 	.notifier_call = kvm_cpu_hotplug,
 };
 
+static int kvm_debugfs_open(struct inode *inode, struct file *file,
+			   int (*get)(void *, u64 *), int (*set)(void *, u64),
+			   const char *fmt)
+{
+	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)
+					  inode->i_private;
+
+	/* The debugfs files are a reference to the kvm struct which
+	 * is still valid when kvm_destroy_vm is called.
+	 * To avoid the race between open and the removal of the debugfs
+	 * directory we test against the users count.
+	 */
+	if (!atomic_add_unless(&stat_data->kvm->users_count, 1, 0))
+		return -ENOENT;
+
+	if (simple_attr_open(inode, file, get, set, fmt)) {
+		kvm_put_kvm(stat_data->kvm);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int kvm_debugfs_release(struct inode *inode, struct file *file)
+{
+	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)
+					  inode->i_private;
+
+	simple_attr_release(inode, file);
+	kvm_put_kvm(stat_data->kvm);
+
+	return 0;
+}
+
+static int vm_stat_get_per_vm(void *data, u64 *val)
+{
+	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
+
+	*val = *(u32 *)((void *)stat_data->kvm + stat_data->offset);
+
+	return 0;
+}
+
+static int vm_stat_get_per_vm_open(struct inode *inode, struct file *file)
+{
+	__simple_attr_check_format("%llu\n", 0ull);
+	return kvm_debugfs_open(inode, file, vm_stat_get_per_vm,
+				NULL, "%llu\n");
+}
+
+static const struct file_operations vm_stat_get_per_vm_fops = {
+	.owner   = THIS_MODULE,
+	.open    = vm_stat_get_per_vm_open,
+	.release = kvm_debugfs_release,
+	.read    = simple_attr_read,
+	.write   = simple_attr_write,
+	.llseek  = generic_file_llseek,
+};
+
+static int vcpu_stat_get_per_vm(void *data, u64 *val)
+{
+	int i;
+	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
+	struct kvm_vcpu *vcpu;
+
+	*val = 0;
+
+	kvm_for_each_vcpu(i, vcpu, stat_data->kvm)
+		*val += *(u32 *)((void *)vcpu + stat_data->offset);
+
+	return 0;
+}
+
+static int vcpu_stat_get_per_vm_open(struct inode *inode, struct file *file)
+{
+	__simple_attr_check_format("%llu\n", 0ull);
+	return kvm_debugfs_open(inode, file, vcpu_stat_get_per_vm,
+				 NULL, "%llu\n");
+}
+
+static const struct file_operations vcpu_stat_get_per_vm_fops = {
+	.owner   = THIS_MODULE,
+	.open    = vcpu_stat_get_per_vm_open,
+	.release = kvm_debugfs_release,
+	.read    = simple_attr_read,
+	.write   = simple_attr_write,
+	.llseek  = generic_file_llseek,
+};
+
+static const struct file_operations *stat_fops_per_vm[] = {
+	[KVM_STAT_VCPU] = &vcpu_stat_get_per_vm_fops,
+	[KVM_STAT_VM]   = &vm_stat_get_per_vm_fops,
+};
+
 static int vm_stat_get(void *_offset, u64 *val)
 {
 	unsigned offset = (long)_offset;
 	struct kvm *kvm;
+	struct kvm_stat_data stat_tmp = {.offset = offset};
+	u64 tmp_val;
 
 	*val = 0;
 	spin_lock(&kvm_lock);
-	list_for_each_entry(kvm, &vm_list, vm_list)
-		*val += *(u32 *)((void *)kvm + offset);
+	list_for_each_entry(kvm, &vm_list, vm_list) {
+		stat_tmp.kvm = kvm;
+		vm_stat_get_per_vm((void *)&stat_tmp, &tmp_val);
+		*val += tmp_val;
+	}
 	spin_unlock(&kvm_lock);
 	return 0;
 }
@@ -3407,15 +3572,16 @@ static int vcpu_stat_get(void *_offset, u64 *val)
 {
 	unsigned offset = (long)_offset;
 	struct kvm *kvm;
-	struct kvm_vcpu *vcpu;
-	int i;
+	struct kvm_stat_data stat_tmp = {.offset = offset};
+	u64 tmp_val;
 
 	*val = 0;
 	spin_lock(&kvm_lock);
-	list_for_each_entry(kvm, &vm_list, vm_list)
-		kvm_for_each_vcpu(i, vcpu, kvm)
-			*val += *(u32 *)((void *)vcpu + offset);
-
+	list_for_each_entry(kvm, &vm_list, vm_list) {
+		stat_tmp.kvm = kvm;
+		vcpu_stat_get_per_vm((void *)&stat_tmp, &tmp_val);
+		*val += tmp_val;
+	}
 	spin_unlock(&kvm_lock);
 	return 0;
 }
@@ -3436,7 +3602,8 @@ static int kvm_init_debug(void)
 	if (kvm_debugfs_dir == NULL)
 		goto out;
 
-	for (p = debugfs_entries; p->name; ++p) {
+	kvm_debugfs_num_entries = 0;
+	for (p = debugfs_entries; p->name; ++p, kvm_debugfs_num_entries++) {
 		if (!debugfs_create_file(p->name, 0444, kvm_debugfs_dir,
 					 (void *)(long)p->offset,
 					 stat_fops[p->kind]))
-- 
2.3.0

Re: [PATCH] KVM: Create debugfs statistics for each VM

[Christian Borntraeger]

On 01/28/2016 04:49 PM, Janosch Frank wrote:
> KVM statistics for VMs (no. of exits, halts and other special
> instructions) are currently only available in a summarized manner for
> all VMs. They are exported to userland through files in the kvm
> debugfs directory and used for performance monitoring, as well as VM
> problem detection with helper tools like kvm_stat. If a VM has
> problems and therefore creates a large number of exits, one can not
> easily find out which one it is, as there is no VM specific data.
> 
> This patch adds a kvm debugfs subdirectory for each VM, which is named
> after its pid and file descriptor. They contain the same kind of files
> that are already in the kvm debugfs directory, but the data that is
> exported through them is now VM specific.
> 
> CC: Dan Carpenter <dan.carpenter@oracle.com> [includes fixes by Dan Carpenter]
> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com>

for s390:
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com> 


some questions below

[...]
> 
> +/* Worst case buffer size needed for holding an integer. */
> +#define ITOA_MAX_LEN 12

4294967295 has 10, so this is to cover the \0 and a potential "-", correct?


[...]
> @@ -3436,7 +3602,8 @@ static int kvm_init_debug(void)
>  	if (kvm_debugfs_dir == NULL)
>  		goto out;
> 
> -	for (p = debugfs_entries; p->name; ++p) {
> +	kvm_debugfs_num_entries = 0;
> +	for (p = debugfs_entries; p->name; ++p, kvm_debugfs_num_entries++) {

Looks like we cannot use ARRAY_SIZE(kvm_stats_debugfs_item), so unless somebody
has a better idea we have to stick with kvm_debugfs_num_entries being calculated.


>  		if (!debugfs_create_file(p->name, 0444, kvm_debugfs_dir,
>  					 (void *)(long)p->offset,
>  					 stat_fops[p->kind]))
> 

Re: [PATCH] KVM: Create debugfs statistics for each VM

[Janosch Frank]

On 02/01/2016 12:45 PM, Christian Borntraeger wrote:
> On 01/28/2016 04:49 PM, Janosch Frank wrote:
>> KVM statistics for VMs (no. of exits, halts and other special
>> instructions) are currently only available in a summarized manner for
>> all VMs. They are exported to userland through files in the kvm
>> debugfs directory and used for performance monitoring, as well as VM
>> problem detection with helper tools like kvm_stat. If a VM has
>> problems and therefore creates a large number of exits, one can not
>> easily find out which one it is, as there is no VM specific data.
>>
>> This patch adds a kvm debugfs subdirectory for each VM, which is named
>> after its pid and file descriptor. They contain the same kind of files
>> that are already in the kvm debugfs directory, but the data that is
>> exported through them is now VM specific.
>>
>> CC: Dan Carpenter <dan.carpenter@oracle.com> [includes fixes by Dan Carpenter]
>> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> 
> for s390:
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com> 
> 
> 
> some questions below
> 
> [...]
>>
>> +/* Worst case buffer size needed for holding an integer. */
>> +#define ITOA_MAX_LEN 12
> 
> 4294967295 has 10, so this is to cover the \0 and a potential "-", correct?

Correct
For the dir name we have 2 * ITOA_MAX_LEN, as it is %d-%d an we only
need one \0 which accounts for the additional hyphen.
I'm open for suggestions about how to make this clearer.

> 
> 
> [...]
>> @@ -3436,7 +3602,8 @@ static int kvm_init_debug(void)
>>  	if (kvm_debugfs_dir == NULL)
>>  		goto out;
>>
>> -	for (p = debugfs_entries; p->name; ++p) {
>> +	kvm_debugfs_num_entries = 0;
>> +	for (p = debugfs_entries; p->name; ++p, kvm_debugfs_num_entries++) {
> 
> Looks like we cannot use ARRAY_SIZE(kvm_stats_debugfs_item), so unless somebody
> has a better idea we have to stick with kvm_debugfs_num_entries being calculated.
> 
> 
>>  		if (!debugfs_create_file(p->name, 0444, kvm_debugfs_dir,
>>  					 (void *)(long)p->offset,
>>  					 stat_fops[p->kind]))
>>
> 

Re: [PATCH] KVM: Create debugfs statistics for each VM

[Christian Borntraeger]

On 01/28/2016 04:49 PM, Janosch Frank wrote:
> KVM statistics for VMs (no. of exits, halts and other special
> instructions) are currently only available in a summarized manner for
> all VMs. They are exported to userland through files in the kvm
> debugfs directory and used for performance monitoring, as well as VM
> problem detection with helper tools like kvm_stat. If a VM has
> problems and therefore creates a large number of exits, one can not
> easily find out which one it is, as there is no VM specific data.
> 
> This patch adds a kvm debugfs subdirectory for each VM, which is named
> after its pid and file descriptor. They contain the same kind of files
> that are already in the kvm debugfs directory, but the data that is
> exported through them is now VM specific.
> 
> CC: Dan Carpenter <dan.carpenter@oracle.com> [includes fixes by Dan Carpenter]
> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com>

FWIW, the newly created subfolders,

require QEMU commit 6590045e5dd2fb0b1d7cdc047ae0c52fd4bb5276
    scripts/kvm/kvm_stat: Replaced os.listdir with os.walk

Otherwise you might get errors like 

Traceback (most recent call last):
  File "scripts/kvm/kvm_stat", line 640, in <module>
    curses.wrapper(tui, stats)
  File "/usr/lib64/python2.7/curses/wrapper.py", line 43, in wrapper
    return func(stdscr, *args, **kwds)
  File "scripts/kvm/kvm_stat", line 547, in tui
    refresh(sleeptime)
  File "scripts/kvm/kvm_stat", line 523, in refresh
    s = stats.get()
  File "scripts/kvm/kvm_stat", line 483, in get
    new = d.read()
  File "scripts/kvm/kvm_stat", line 37, in read
    return dict([(key, val(key)) for key in self._fields])
  File "scripts/kvm/kvm_stat", line 36, in val
    return int(file(self.base + '/' + key).read())
IOError: [Errno 21] Is a directory: '/sys/kernel/debug/kvm/13123-31'

when using the "-d" option.


Paolo, I still think that this a valuable addon for debugging. Any guidance?


> ---
>  include/linux/kvm_host.h |   7 ++
>  virt/kvm/kvm_main.c      | 187 ++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 184 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f707f74..3f237e1 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -406,6 +406,8 @@ struct kvm {
>  #endif
>  	long tlbs_dirty;
>  	struct list_head devices;
> +	struct dentry *debugfs_dentry;
> +	struct kvm_stat_data **debugfs_stat_data;
>  };
> 
>  #define kvm_err(fmt, ...) \
> @@ -982,6 +984,11 @@ enum kvm_stat_kind {
>  	KVM_STAT_VCPU,
>  };
> 
> +struct kvm_stat_data {
> +	int offset;
> +	struct kvm *kvm;
> +};
> +
>  struct kvm_stats_debugfs_item {
>  	const char *name;
>  	int offset;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 314c777..d530e60 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -63,6 +63,9 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/kvm.h>
> 
> +/* Worst case buffer size needed for holding an integer. */
> +#define ITOA_MAX_LEN 12
> +
>  MODULE_AUTHOR("Qumranet");
>  MODULE_LICENSE("GPL");
> 
> @@ -100,6 +103,9 @@ static __read_mostly struct preempt_ops kvm_preempt_ops;
>  struct dentry *kvm_debugfs_dir;
>  EXPORT_SYMBOL_GPL(kvm_debugfs_dir);
> 
> +static int kvm_debugfs_num_entries;
> +static const struct file_operations *stat_fops_per_vm[];
> +
>  static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl,
>  			   unsigned long arg);
>  #ifdef CONFIG_KVM_COMPAT
> @@ -529,6 +535,58 @@ static void kvm_free_memslots(struct kvm *kvm, struct kvm_memslots *slots)
>  	kvfree(slots);
>  }
> 
> +static void kvm_destroy_vm_debugfs(struct kvm *kvm)
> +{
> +	int i;
> +
> +	if (!kvm->debugfs_dentry)
> +		return;
> +
> +	debugfs_remove_recursive(kvm->debugfs_dentry);
> +
> +	for (i = 0; i < kvm_debugfs_num_entries; i++)
> +		kfree(kvm->debugfs_stat_data[i]);
> +	kfree(kvm->debugfs_stat_data);
> +}
> +
> +static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
> +{
> +	char dir_name[ITOA_MAX_LEN * 2];
> +	struct kvm_stat_data *stat_data;
> +	struct kvm_stats_debugfs_item *p;
> +
> +	if (!debugfs_initialized())
> +		return 0;
> +
> +	snprintf(dir_name, sizeof(dir_name), "%d-%d", task_pid_nr(current), fd);
> +	kvm->debugfs_dentry = debugfs_create_dir(dir_name,
> +						 kvm_debugfs_dir);
> +	if (!kvm->debugfs_dentry)
> +		return -ENOMEM;
> +
> +	kvm->debugfs_stat_data = kcalloc(kvm_debugfs_num_entries,
> +					 sizeof(*kvm->debugfs_stat_data),
> +					 GFP_KERNEL);
> +	if (!kvm->debugfs_stat_data)
> +		return -ENOMEM;
> +
> +	for (p = debugfs_entries; p->name; p++) {
> +		stat_data = kzalloc(sizeof(*stat_data), GFP_KERNEL);
> +		if (!stat_data)
> +			return -ENOMEM;
> +
> +		stat_data->kvm = kvm;
> +		stat_data->offset = p->offset;
> +		kvm->debugfs_stat_data[p - debugfs_entries] = stat_data;
> +		if (!debugfs_create_file(p->name, 0444,
> +					 kvm->debugfs_dentry,
> +					 stat_data,
> +					 stat_fops_per_vm[p->kind]))
> +			return -ENOMEM;
> +	}
> +	return 0;
> +}
> +
>  static struct kvm *kvm_create_vm(unsigned long type)
>  {
>  	int r, i;
> @@ -636,6 +694,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
>  	int i;
>  	struct mm_struct *mm = kvm->mm;
> 
> +	kvm_destroy_vm_debugfs(kvm);
>  	kvm_arch_sync_events(kvm);
>  	spin_lock(&kvm_lock);
>  	list_del(&kvm->vm_list);
> @@ -2962,8 +3021,15 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
>  	}
>  #endif
>  	r = anon_inode_getfd("kvm-vm", &kvm_vm_fops, kvm, O_RDWR | O_CLOEXEC);
> -	if (r < 0)
> +	if (r < 0) {
>  		kvm_put_kvm(kvm);
> +		return r;
> +	}
> +
> +	if (kvm_create_vm_debugfs(kvm, r) < 0) {
> +		kvm_put_kvm(kvm);
> +		return -ENOMEM;
> +	}
> 
>  	return r;
>  }
> @@ -3388,15 +3454,114 @@ static struct notifier_block kvm_cpu_notifier = {
>  	.notifier_call = kvm_cpu_hotplug,
>  };
> 
> +static int kvm_debugfs_open(struct inode *inode, struct file *file,
> +			   int (*get)(void *, u64 *), int (*set)(void *, u64),
> +			   const char *fmt)
> +{
> +	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)
> +					  inode->i_private;
> +
> +	/* The debugfs files are a reference to the kvm struct which
> +	 * is still valid when kvm_destroy_vm is called.
> +	 * To avoid the race between open and the removal of the debugfs
> +	 * directory we test against the users count.
> +	 */
> +	if (!atomic_add_unless(&stat_data->kvm->users_count, 1, 0))
> +		return -ENOENT;
> +
> +	if (simple_attr_open(inode, file, get, set, fmt)) {
> +		kvm_put_kvm(stat_data->kvm);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int kvm_debugfs_release(struct inode *inode, struct file *file)
> +{
> +	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)
> +					  inode->i_private;
> +
> +	simple_attr_release(inode, file);
> +	kvm_put_kvm(stat_data->kvm);
> +
> +	return 0;
> +}
> +
> +static int vm_stat_get_per_vm(void *data, u64 *val)
> +{
> +	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
> +
> +	*val = *(u32 *)((void *)stat_data->kvm + stat_data->offset);
> +
> +	return 0;
> +}
> +
> +static int vm_stat_get_per_vm_open(struct inode *inode, struct file *file)
> +{
> +	__simple_attr_check_format("%llu\n", 0ull);
> +	return kvm_debugfs_open(inode, file, vm_stat_get_per_vm,
> +				NULL, "%llu\n");
> +}
> +
> +static const struct file_operations vm_stat_get_per_vm_fops = {
> +	.owner   = THIS_MODULE,
> +	.open    = vm_stat_get_per_vm_open,
> +	.release = kvm_debugfs_release,
> +	.read    = simple_attr_read,
> +	.write   = simple_attr_write,
> +	.llseek  = generic_file_llseek,
> +};
> +
> +static int vcpu_stat_get_per_vm(void *data, u64 *val)
> +{
> +	int i;
> +	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
> +	struct kvm_vcpu *vcpu;
> +
> +	*val = 0;
> +
> +	kvm_for_each_vcpu(i, vcpu, stat_data->kvm)
> +		*val += *(u32 *)((void *)vcpu + stat_data->offset);
> +
> +	return 0;
> +}
> +
> +static int vcpu_stat_get_per_vm_open(struct inode *inode, struct file *file)
> +{
> +	__simple_attr_check_format("%llu\n", 0ull);
> +	return kvm_debugfs_open(inode, file, vcpu_stat_get_per_vm,
> +				 NULL, "%llu\n");
> +}
> +
> +static const struct file_operations vcpu_stat_get_per_vm_fops = {
> +	.owner   = THIS_MODULE,
> +	.open    = vcpu_stat_get_per_vm_open,
> +	.release = kvm_debugfs_release,
> +	.read    = simple_attr_read,
> +	.write   = simple_attr_write,
> +	.llseek  = generic_file_llseek,
> +};
> +
> +static const struct file_operations *stat_fops_per_vm[] = {
> +	[KVM_STAT_VCPU] = &vcpu_stat_get_per_vm_fops,
> +	[KVM_STAT_VM]   = &vm_stat_get_per_vm_fops,
> +};
> +
>  static int vm_stat_get(void *_offset, u64 *val)
>  {
>  	unsigned offset = (long)_offset;
>  	struct kvm *kvm;
> +	struct kvm_stat_data stat_tmp = {.offset = offset};
> +	u64 tmp_val;
> 
>  	*val = 0;
>  	spin_lock(&kvm_lock);
> -	list_for_each_entry(kvm, &vm_list, vm_list)
> -		*val += *(u32 *)((void *)kvm + offset);
> +	list_for_each_entry(kvm, &vm_list, vm_list) {
> +		stat_tmp.kvm = kvm;
> +		vm_stat_get_per_vm((void *)&stat_tmp, &tmp_val);
> +		*val += tmp_val;
> +	}
>  	spin_unlock(&kvm_lock);
>  	return 0;
>  }
> @@ -3407,15 +3572,16 @@ static int vcpu_stat_get(void *_offset, u64 *val)
>  {
>  	unsigned offset = (long)_offset;
>  	struct kvm *kvm;
> -	struct kvm_vcpu *vcpu;
> -	int i;
> +	struct kvm_stat_data stat_tmp = {.offset = offset};
> +	u64 tmp_val;
> 
>  	*val = 0;
>  	spin_lock(&kvm_lock);
> -	list_for_each_entry(kvm, &vm_list, vm_list)
> -		kvm_for_each_vcpu(i, vcpu, kvm)
> -			*val += *(u32 *)((void *)vcpu + offset);
> -
> +	list_for_each_entry(kvm, &vm_list, vm_list) {
> +		stat_tmp.kvm = kvm;
> +		vcpu_stat_get_per_vm((void *)&stat_tmp, &tmp_val);
> +		*val += tmp_val;
> +	}
>  	spin_unlock(&kvm_lock);
>  	return 0;
>  }
> @@ -3436,7 +3602,8 @@ static int kvm_init_debug(void)
>  	if (kvm_debugfs_dir == NULL)
>  		goto out;
> 
> -	for (p = debugfs_entries; p->name; ++p) {
> +	kvm_debugfs_num_entries = 0;
> +	for (p = debugfs_entries; p->name; ++p, kvm_debugfs_num_entries++) {
>  		if (!debugfs_create_file(p->name, 0444, kvm_debugfs_dir,
>  					 (void *)(long)p->offset,
>  					 stat_fops[p->kind]))
> 

Re: [PATCH] KVM: Create debugfs statistics for each VM

[Paolo Bonzini]

On 02/02/2016 15:14, Christian Borntraeger wrote:
> FWIW, the newly created subfolders,
> 
> require QEMU commit 6590045e5dd2fb0b1d7cdc047ae0c52fd4bb5276
>     scripts/kvm/kvm_stat: Replaced os.listdir with os.walk
> 
> Otherwise you might get errors like 
> 
> Traceback (most recent call last):
>   File "scripts/kvm/kvm_stat", line 640, in <module>
>     curses.wrapper(tui, stats)
>   File "/usr/lib64/python2.7/curses/wrapper.py", line 43, in wrapper
>     return func(stdscr, *args, **kwds)
>   File "scripts/kvm/kvm_stat", line 547, in tui
>     refresh(sleeptime)
>   File "scripts/kvm/kvm_stat", line 523, in refresh
>     s = stats.get()
>   File "scripts/kvm/kvm_stat", line 483, in get
>     new = d.read()
>   File "scripts/kvm/kvm_stat", line 37, in read
>     return dict([(key, val(key)) for key in self._fields])
>   File "scripts/kvm/kvm_stat", line 36, in val
>     return int(file(self.base + '/' + key).read())
> IOError: [Errno 21] Is a directory: '/sys/kernel/debug/kvm/13123-31'
> 
> when using the "-d" option.
> 
> 
> Paolo, I still think that this a valuable addon for debugging. Any guidance?

Yeah, I agree...  Do we want to add a module parameter for this, and/or
a kernel configuration that for now defaults to N?

An alternative is to move kvm_stat from QEMU to tools/.

Paolo

Re: [PATCH] KVM: Create debugfs statistics for each VM

[Christian Borntraeger]

On 02/04/2016 02:05 PM, Paolo Bonzini wrote:
> 
> 
> On 02/02/2016 15:14, Christian Borntraeger wrote:
>> FWIW, the newly created subfolders,
>>
>> require QEMU commit 6590045e5dd2fb0b1d7cdc047ae0c52fd4bb5276
>>     scripts/kvm/kvm_stat: Replaced os.listdir with os.walk
>>
>> Otherwise you might get errors like 
>>
>> Traceback (most recent call last):
>>   File "scripts/kvm/kvm_stat", line 640, in <module>
>>     curses.wrapper(tui, stats)
>>   File "/usr/lib64/python2.7/curses/wrapper.py", line 43, in wrapper
>>     return func(stdscr, *args, **kwds)
>>   File "scripts/kvm/kvm_stat", line 547, in tui
>>     refresh(sleeptime)
>>   File "scripts/kvm/kvm_stat", line 523, in refresh
>>     s = stats.get()
>>   File "scripts/kvm/kvm_stat", line 483, in get
>>     new = d.read()
>>   File "scripts/kvm/kvm_stat", line 37, in read
>>     return dict([(key, val(key)) for key in self._fields])
>>   File "scripts/kvm/kvm_stat", line 36, in val
>>     return int(file(self.base + '/' + key).read())
>> IOError: [Errno 21] Is a directory: '/sys/kernel/debug/kvm/13123-31'
>>
>> when using the "-d" option.
>>
>>
>> Paolo, I still think that this a valuable addon for debugging. Any guidance?
> 
> Yeah, I agree...  Do we want to add a module parameter for this, and/or
> a kernel configuration that for now defaults to N?
> 


> An alternative is to move kvm_stat from QEMU to tools/.

Hmm, that is probably the best solution, given that it has no dependency
on QEMU but heavily depends on the kernel module.

Re: [PATCH] KVM: Create debugfs statistics for each VM

[Janosch Frank]

On 02/04/2016 02:10 PM, Christian Borntraeger wrote:
> On 02/04/2016 02:05 PM, Paolo Bonzini wrote:
>>
>>
>> On 02/02/2016 15:14, Christian Borntraeger wrote:
>>> FWIW, the newly created subfolders,
>>>
>>> require QEMU commit 6590045e5dd2fb0b1d7cdc047ae0c52fd4bb5276
>>>     scripts/kvm/kvm_stat: Replaced os.listdir with os.walk
>>>
>>> Otherwise you might get errors like 
>>>
>>> Traceback (most recent call last):
>>>   File "scripts/kvm/kvm_stat", line 640, in <module>
>>>     curses.wrapper(tui, stats)
>>>   File "/usr/lib64/python2.7/curses/wrapper.py", line 43, in wrapper
>>>     return func(stdscr, *args, **kwds)
>>>   File "scripts/kvm/kvm_stat", line 547, in tui
>>>     refresh(sleeptime)
>>>   File "scripts/kvm/kvm_stat", line 523, in refresh
>>>     s = stats.get()
>>>   File "scripts/kvm/kvm_stat", line 483, in get
>>>     new = d.read()
>>>   File "scripts/kvm/kvm_stat", line 37, in read
>>>     return dict([(key, val(key)) for key in self._fields])
>>>   File "scripts/kvm/kvm_stat", line 36, in val
>>>     return int(file(self.base + '/' + key).read())
>>> IOError: [Errno 21] Is a directory: '/sys/kernel/debug/kvm/13123-31'
>>>
>>> when using the "-d" option.
>>>
>>>
>>> Paolo, I still think that this a valuable addon for debugging. Any guidance?
>>
>> Yeah, I agree...  Do we want to add a module parameter for this, and/or
>> a kernel configuration that for now defaults to N?
>>
> 
> 
>> An alternative is to move kvm_stat from QEMU to tools/.
> 
> Hmm, that is probably the best solution, given that it has no dependency
> on QEMU but heavily depends on the kernel module.

So, what's the plan then?

Give me some pointers and I'm happy to do the work, but I currently
don't have enough experience to know/guess what is wanted.

I still have about 3 or 4 patches for kvm_stat in my queue that will add
the filtering support for debugfs AND tracefs. One of them also adds
documentation to the script, as well as to the man page and I might add
one that brings py3 support.
Most of them are ready, but before finishing the filtering support I
wanted to wait until the kernel patch was in next.

Do you (Paolo) want to move this patch through kvm-next and the
corresponding python patches through qemu and later on move the script
to the kernel?

Cheers
Janosch

Re: [PATCH] KVM: Create debugfs statistics for each VM

[Christian Borntraeger]

On 02/05/2016 10:02 AM, Janosch Frank wrote:
> On 02/04/2016 02:10 PM, Christian Borntraeger wrote:
>> On 02/04/2016 02:05 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 02/02/2016 15:14, Christian Borntraeger wrote:
>>>> FWIW, the newly created subfolders,
>>>>
>>>> require QEMU commit 6590045e5dd2fb0b1d7cdc047ae0c52fd4bb5276
>>>>     scripts/kvm/kvm_stat: Replaced os.listdir with os.walk
>>>>
>>>> Otherwise you might get errors like 
>>>>
>>>> Traceback (most recent call last):
>>>>   File "scripts/kvm/kvm_stat", line 640, in <module>
>>>>     curses.wrapper(tui, stats)
>>>>   File "/usr/lib64/python2.7/curses/wrapper.py", line 43, in wrapper
>>>>     return func(stdscr, *args, **kwds)
>>>>   File "scripts/kvm/kvm_stat", line 547, in tui
>>>>     refresh(sleeptime)
>>>>   File "scripts/kvm/kvm_stat", line 523, in refresh
>>>>     s = stats.get()
>>>>   File "scripts/kvm/kvm_stat", line 483, in get
>>>>     new = d.read()
>>>>   File "scripts/kvm/kvm_stat", line 37, in read
>>>>     return dict([(key, val(key)) for key in self._fields])
>>>>   File "scripts/kvm/kvm_stat", line 36, in val
>>>>     return int(file(self.base + '/' + key).read())
>>>> IOError: [Errno 21] Is a directory: '/sys/kernel/debug/kvm/13123-31'
>>>>
>>>> when using the "-d" option.
>>>>
>>>>
>>>> Paolo, I still think that this a valuable addon for debugging. Any guidance?
>>>
>>> Yeah, I agree...  Do we want to add a module parameter for this, and/or
>>> a kernel configuration that for now defaults to N?

Hmm, a new Kconfig might also be a good idea. 
There is another problem that kvm_stat wants to have /sys/kernel/debug/tracing,
which is only available with CONFIG_TRACING. CONFIG_TRACING cannot be selected by
itself, so you have to enable some other tracer (like function tracing) that
has nothing to do with KVM, which then selects GENERIC_TRACER which selects
CONFIG_TRACING and then enables the kvm trace points.

So maybe a 
config KVM_PER_GUEST_TRACING
that selects GENERIC_TRACER and enables that feature?


>>>
>>
>>
>>> An alternative is to move kvm_stat from QEMU to tools/.
>>
>> Hmm, that is probably the best solution, given that it has no dependency
>> on QEMU but heavily depends on the kernel module.
> 
> So, what's the plan then?
> 
> Give me some pointers and I'm happy to do the work, but I currently
> don't have enough experience to know/guess what is wanted.
> 
> I still have about 3 or 4 patches for kvm_stat in my queue that will add
> the filtering support for debugfs AND tracefs. One of them also adds
> documentation to the script, as well as to the man page and I might add
> one that brings py3 support.
> Most of them are ready, but before finishing the filtering support I
> wanted to wait until the kernel patch was in next.
> 
> Do you (Paolo) want to move this patch through kvm-next and the
> corresponding python patches through qemu and later on move the script
> to the kernel?

Re: [PATCH] KVM: Create debugfs statistics for each VM

[Paolo Bonzini]

On 04/02/2016 14:10, Christian Borntraeger wrote:
>>> >>
>>> >> Paolo, I still think that this a valuable addon for debugging. Any guidance?
>> > 
>> > Yeah, I agree...  Do we want to add a module parameter for this, and/or
>> > a kernel configuration that for now defaults to N?
>> > 
> 
>> > An alternative is to move kvm_stat from QEMU to tools/.
> Hmm, that is probably the best solution, given that it has no dependency
> on QEMU but heavily depends on the kernel module.

Ok, let's do that for Linux 4.5 and QEMU 2.6.

Paolo

Re: [PATCH] KVM: Create debugfs statistics for each VM

[Paolo Bonzini]

On 05/02/2016 11:14, Christian Borntraeger wrote:
> > > Paolo, I still think that this a valuable addon for debugging. Any guidance?
> >
> > Yeah, I agree...  Do we want to add a module parameter for this, and/or
> > a kernel configuration that for now defaults to N?
>
> Hmm, a new Kconfig might also be a good idea. 

I think a new Kconfig is unnecessary.  Let's just move the script to the
kernel; once it's moved, it is installed together with the kernel and
the Kconfig is pointless.

Paolo