All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] kvm: Refactor handling of VM debugfs files
@ 2019-12-10 16:07 Milan Pandurov
  2019-12-11  2:56   ` kbuild test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Milan Pandurov @ 2019-12-10 16:07 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, rkrcmar, graf, borntraeger

By storing kvm_stat_kind inside kvm_stat_data struct we can remove
duplicated code and remove usage of temporary kvm_stat_data struct
inside vm_stat_get et al.

Signed-off-by: Milan Pandurov <milanpa@amazon.de>
---
 include/linux/kvm_host.h |   1 +
 virt/kvm/kvm_main.c      | 118 +++++++++++++++++----------------------
 2 files changed, 53 insertions(+), 66 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 7ed1e2f8641e..212d5117efda 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1112,6 +1112,7 @@ struct kvm_stat_data {
 	int offset;
 	int mode;
 	struct kvm *kvm;
+	enum kvm_stat_kind kind;
 };
 
 struct kvm_stats_debugfs_item {
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 00268290dcbd..155f144fcc7c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -113,7 +113,7 @@ 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 const struct file_operations stat_fops_per_vm;
 
 static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl,
 			   unsigned long arg);
@@ -652,9 +652,10 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 		stat_data->kvm = kvm;
 		stat_data->offset = p->offset;
 		stat_data->mode = p->mode ? p->mode : 0644;
+		stat_data->kind = p->kind;
 		kvm->debugfs_stat_data[p - debugfs_entries] = stat_data;
 		debugfs_create_file(p->name, stat_data->mode, kvm->debugfs_dentry,
-				    stat_data, stat_fops_per_vm[p->kind]);
+				    stat_data, &stat_fops_per_vm);
 	}
 	return 0;
 }
@@ -4033,105 +4034,96 @@ static int kvm_debugfs_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static int vm_stat_get_per_vm(void *data, u64 *val)
+static int kvm_get_stat_per_vm(struct kvm *kvm, size_t offset, u64 *val)
 {
-	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
-
-	*val = *(ulong *)((void *)stat_data->kvm + stat_data->offset);
+	*val = *(ulong *)((void *)kvm + offset);
 
 	return 0;
 }
 
-static int vm_stat_clear_per_vm(void *data, u64 val)
+static int kvm_clear_stat_per_vm(struct kvm *kvm, size_t offset)
 {
-	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
-
-	if (val)
-		return -EINVAL;
-
-	*(ulong *)((void *)stat_data->kvm + stat_data->offset) = 0;
+	*(ulong *)((void *)kvm + offset) = 0;
 
 	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,
-				vm_stat_clear_per_vm, "%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  = no_llseek,
-};
-
-static int vcpu_stat_get_per_vm(void *data, u64 *val)
+static int kvm_get_stat_per_vcpu(struct kvm *kvm, size_t offset, 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 += *(u64 *)((void *)vcpu + stat_data->offset);
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		*val += *(u64 *)((void *)vcpu + offset);
 
 	return 0;
 }
 
-static int vcpu_stat_clear_per_vm(void *data, u64 val)
+static int kvm_clear_stat_per_vcpu(struct kvm *kvm, size_t offset)
 {
 	int i;
-	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
 	struct kvm_vcpu *vcpu;
 
-	if (val)
-		return -EINVAL;
-
-	kvm_for_each_vcpu(i, vcpu, stat_data->kvm)
-		*(u64 *)((void *)vcpu + stat_data->offset) = 0;
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		*(u64 *)((void *)vcpu + offset) = 0;
 
 	return 0;
 }
 
-static int vcpu_stat_get_per_vm_open(struct inode *inode, struct file *file)
+struct kvm_stat_operations {
+	int (*get)(struct kvm *kvm, size_t offset, u64 *val);
+	int (*clear)(struct kvm *kvm, size_t offset);
+};
+
+static const struct kvm_stat_operations kvm_stat_ops[] = {
+	[KVM_STAT_VM] = { .get = kvm_get_stat_per_vm,
+			  .clear = kvm_clear_stat_per_vm },
+	[KVM_STAT_VCPU] = { .get = kvm_get_stat_per_vcpu,
+			    .clear = kvm_clear_stat_per_vcpu },
+};
+
+static int kvm_stat_data_get(void *data, u64 *val)
+{
+	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
+	return kvm_stat_ops[stat_data->kind].get(stat_data->kvm,
+						 stat_data->offset, val);
+}
+
+static int kvm_stat_data_clear(void *data, u64 val)
+{
+	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
+	return kvm_stat_ops[stat_data->kind].clear(stat_data->kvm,
+						   stat_data->offset);
+}
+
+static int kvm_stat_data_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,
-				 vcpu_stat_clear_per_vm, "%llu\n");
+	return kvm_debugfs_open(inode, file, kvm_stat_data_get,
+				kvm_stat_data_clear, "%llu\n");
 }
 
-static const struct file_operations vcpu_stat_get_per_vm_fops = {
-	.owner   = THIS_MODULE,
-	.open    = vcpu_stat_get_per_vm_open,
+static const struct file_operations stat_fops_per_vm = {
+	.owner = THIS_MODULE,
+	.open = kvm_stat_data_open,
 	.release = kvm_debugfs_release,
-	.read    = simple_attr_read,
-	.write   = simple_attr_write,
-	.llseek  = no_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,
+	.read = simple_attr_read,
+	.write = simple_attr_write,
+	.llseek = no_llseek,
 };
 
 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;
 	mutex_lock(&kvm_lock);
 	list_for_each_entry(kvm, &vm_list, vm_list) {
-		stat_tmp.kvm = kvm;
-		vm_stat_get_per_vm((void *)&stat_tmp, &tmp_val);
+		vm_stat_get_per_vm(kvm, offset, &tmp_val);
 		*val += tmp_val;
 	}
 	mutex_unlock(&kvm_lock);
@@ -4142,15 +4134,13 @@ static int vm_stat_clear(void *_offset, u64 val)
 {
 	unsigned offset = (long)_offset;
 	struct kvm *kvm;
-	struct kvm_stat_data stat_tmp = {.offset = offset};
 
 	if (val)
 		return -EINVAL;
 
 	mutex_lock(&kvm_lock);
 	list_for_each_entry(kvm, &vm_list, vm_list) {
-		stat_tmp.kvm = kvm;
-		vm_stat_clear_per_vm((void *)&stat_tmp, 0);
+		vm_stat_clear_per_vm(kvm, offset);
 	}
 	mutex_unlock(&kvm_lock);
 
@@ -4163,14 +4153,12 @@ static int vcpu_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;
 	mutex_lock(&kvm_lock);
 	list_for_each_entry(kvm, &vm_list, vm_list) {
-		stat_tmp.kvm = kvm;
-		vcpu_stat_get_per_vm((void *)&stat_tmp, &tmp_val);
+		vcpu_stat_get_per_vm(kvm, offset, &tmp_val);
 		*val += tmp_val;
 	}
 	mutex_unlock(&kvm_lock);
@@ -4181,15 +4169,13 @@ static int vcpu_stat_clear(void *_offset, u64 val)
 {
 	unsigned offset = (long)_offset;
 	struct kvm *kvm;
-	struct kvm_stat_data stat_tmp = {.offset = offset};
 
 	if (val)
 		return -EINVAL;
 
 	mutex_lock(&kvm_lock);
 	list_for_each_entry(kvm, &vm_list, vm_list) {
-		stat_tmp.kvm = kvm;
-		vcpu_stat_clear_per_vm((void *)&stat_tmp, 0);
+		kvm_clear_stat_per_vcpu(kvm, offset);
 	}
 	mutex_unlock(&kvm_lock);
 
-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] kvm: Refactor handling of VM debugfs files
  2019-12-10 16:07 [PATCH] kvm: Refactor handling of VM debugfs files Milan Pandurov
@ 2019-12-11  2:56   ` kbuild test robot
  2019-12-11  7:16 ` Alexander Graf
  2019-12-11 16:33   ` kbuild test robot
  2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2019-12-11  2:56 UTC (permalink / raw)
  To: Milan Pandurov; +Cc: kbuild-all, kvm, pbonzini, rkrcmar, graf, borntraeger

[-- Attachment #1: Type: text/plain, Size: 3775 bytes --]

Hi Milan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on vhost/linux-next v5.5-rc1 next-20191210]
[cannot apply to kvms390/next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Milan-Pandurov/kvm-Refactor-handling-of-VM-debugfs-files/20191211-063504
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: s390-defconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=s390 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/s390/kvm/../../../virt/kvm/kvm_main.c: In function 'vm_stat_get':
>> arch/s390/kvm/../../../virt/kvm/kvm_main.c:4126:3: error: implicit declaration of function 'vm_stat_get_per_vm'; did you mean 'vm_stat_get'? [-Werror=implicit-function-declaration]
      vm_stat_get_per_vm(kvm, offset, &tmp_val);
      ^~~~~~~~~~~~~~~~~~
      vm_stat_get
   arch/s390/kvm/../../../virt/kvm/kvm_main.c: In function 'vm_stat_clear':
>> arch/s390/kvm/../../../virt/kvm/kvm_main.c:4143:3: error: implicit declaration of function 'vm_stat_clear_per_vm'; did you mean 'vm_stat_clear'? [-Werror=implicit-function-declaration]
      vm_stat_clear_per_vm(kvm, offset);
      ^~~~~~~~~~~~~~~~~~~~
      vm_stat_clear
   arch/s390/kvm/../../../virt/kvm/kvm_main.c: In function 'vcpu_stat_get':
>> arch/s390/kvm/../../../virt/kvm/kvm_main.c:4161:3: error: implicit declaration of function 'vcpu_stat_get_per_vm'; did you mean 'vcpu_stat_get'? [-Werror=implicit-function-declaration]
      vcpu_stat_get_per_vm(kvm, offset, &tmp_val);
      ^~~~~~~~~~~~~~~~~~~~
      vcpu_stat_get
   cc1: some warnings being treated as errors

vim +4126 arch/s390/kvm/../../../virt/kvm/kvm_main.c

  4116	
  4117	static int vm_stat_get(void *_offset, u64 *val)
  4118	{
  4119		unsigned offset = (long)_offset;
  4120		struct kvm *kvm;
  4121		u64 tmp_val;
  4122	
  4123		*val = 0;
  4124		mutex_lock(&kvm_lock);
  4125		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4126			vm_stat_get_per_vm(kvm, offset, &tmp_val);
  4127			*val += tmp_val;
  4128		}
  4129		mutex_unlock(&kvm_lock);
  4130		return 0;
  4131	}
  4132	
  4133	static int vm_stat_clear(void *_offset, u64 val)
  4134	{
  4135		unsigned offset = (long)_offset;
  4136		struct kvm *kvm;
  4137	
  4138		if (val)
  4139			return -EINVAL;
  4140	
  4141		mutex_lock(&kvm_lock);
  4142		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4143			vm_stat_clear_per_vm(kvm, offset);
  4144		}
  4145		mutex_unlock(&kvm_lock);
  4146	
  4147		return 0;
  4148	}
  4149	
  4150	DEFINE_SIMPLE_ATTRIBUTE(vm_stat_fops, vm_stat_get, vm_stat_clear, "%llu\n");
  4151	
  4152	static int vcpu_stat_get(void *_offset, u64 *val)
  4153	{
  4154		unsigned offset = (long)_offset;
  4155		struct kvm *kvm;
  4156		u64 tmp_val;
  4157	
  4158		*val = 0;
  4159		mutex_lock(&kvm_lock);
  4160		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4161			vcpu_stat_get_per_vm(kvm, offset, &tmp_val);
  4162			*val += tmp_val;
  4163		}
  4164		mutex_unlock(&kvm_lock);
  4165		return 0;
  4166	}
  4167	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18755 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kvm: Refactor handling of VM debugfs files
@ 2019-12-11  2:56   ` kbuild test robot
  0 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2019-12-11  2:56 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3878 bytes --]

Hi Milan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on vhost/linux-next v5.5-rc1 next-20191210]
[cannot apply to kvms390/next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Milan-Pandurov/kvm-Refactor-handling-of-VM-debugfs-files/20191211-063504
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: s390-defconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=s390 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/s390/kvm/../../../virt/kvm/kvm_main.c: In function 'vm_stat_get':
>> arch/s390/kvm/../../../virt/kvm/kvm_main.c:4126:3: error: implicit declaration of function 'vm_stat_get_per_vm'; did you mean 'vm_stat_get'? [-Werror=implicit-function-declaration]
      vm_stat_get_per_vm(kvm, offset, &tmp_val);
      ^~~~~~~~~~~~~~~~~~
      vm_stat_get
   arch/s390/kvm/../../../virt/kvm/kvm_main.c: In function 'vm_stat_clear':
>> arch/s390/kvm/../../../virt/kvm/kvm_main.c:4143:3: error: implicit declaration of function 'vm_stat_clear_per_vm'; did you mean 'vm_stat_clear'? [-Werror=implicit-function-declaration]
      vm_stat_clear_per_vm(kvm, offset);
      ^~~~~~~~~~~~~~~~~~~~
      vm_stat_clear
   arch/s390/kvm/../../../virt/kvm/kvm_main.c: In function 'vcpu_stat_get':
>> arch/s390/kvm/../../../virt/kvm/kvm_main.c:4161:3: error: implicit declaration of function 'vcpu_stat_get_per_vm'; did you mean 'vcpu_stat_get'? [-Werror=implicit-function-declaration]
      vcpu_stat_get_per_vm(kvm, offset, &tmp_val);
      ^~~~~~~~~~~~~~~~~~~~
      vcpu_stat_get
   cc1: some warnings being treated as errors

vim +4126 arch/s390/kvm/../../../virt/kvm/kvm_main.c

  4116	
  4117	static int vm_stat_get(void *_offset, u64 *val)
  4118	{
  4119		unsigned offset = (long)_offset;
  4120		struct kvm *kvm;
  4121		u64 tmp_val;
  4122	
  4123		*val = 0;
  4124		mutex_lock(&kvm_lock);
  4125		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4126			vm_stat_get_per_vm(kvm, offset, &tmp_val);
  4127			*val += tmp_val;
  4128		}
  4129		mutex_unlock(&kvm_lock);
  4130		return 0;
  4131	}
  4132	
  4133	static int vm_stat_clear(void *_offset, u64 val)
  4134	{
  4135		unsigned offset = (long)_offset;
  4136		struct kvm *kvm;
  4137	
  4138		if (val)
  4139			return -EINVAL;
  4140	
  4141		mutex_lock(&kvm_lock);
  4142		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4143			vm_stat_clear_per_vm(kvm, offset);
  4144		}
  4145		mutex_unlock(&kvm_lock);
  4146	
  4147		return 0;
  4148	}
  4149	
  4150	DEFINE_SIMPLE_ATTRIBUTE(vm_stat_fops, vm_stat_get, vm_stat_clear, "%llu\n");
  4151	
  4152	static int vcpu_stat_get(void *_offset, u64 *val)
  4153	{
  4154		unsigned offset = (long)_offset;
  4155		struct kvm *kvm;
  4156		u64 tmp_val;
  4157	
  4158		*val = 0;
  4159		mutex_lock(&kvm_lock);
  4160		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4161			vcpu_stat_get_per_vm(kvm, offset, &tmp_val);
  4162			*val += tmp_val;
  4163		}
  4164		mutex_unlock(&kvm_lock);
  4165		return 0;
  4166	}
  4167	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 18755 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kvm: Refactor handling of VM debugfs files
  2019-12-10 16:07 [PATCH] kvm: Refactor handling of VM debugfs files Milan Pandurov
  2019-12-11  2:56   ` kbuild test robot
@ 2019-12-11  7:16 ` Alexander Graf
  2019-12-11 16:33   ` kbuild test robot
  2 siblings, 0 replies; 6+ messages in thread
From: Alexander Graf @ 2019-12-11  7:16 UTC (permalink / raw)
  To: Milan Pandurov, kvm; +Cc: pbonzini, rkrcmar, borntraeger



On 10.12.19 18:07, Milan Pandurov wrote:
> By storing kvm_stat_kind inside kvm_stat_data struct we can remove
> duplicated code and remove usage of temporary kvm_stat_data struct
> inside vm_stat_get et al.
> 
> Signed-off-by: Milan Pandurov <milanpa@amazon.de>
> ---
>   include/linux/kvm_host.h |   1 +
>   virt/kvm/kvm_main.c      | 118 +++++++++++++++++----------------------
>   2 files changed, 53 insertions(+), 66 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 7ed1e2f8641e..212d5117efda 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1112,6 +1112,7 @@ struct kvm_stat_data {
>   	int offset;
>   	int mode;
>   	struct kvm *kvm;
> +	enum kvm_stat_kind kind;

Why don't we just add a pointer to the debugfs_item here? That would 
make offset, mode and kind redundant. They are pretty much just copies 
from the template and immutable afterwards, no?

>   };
>   
>   struct kvm_stats_debugfs_item {
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 00268290dcbd..155f144fcc7c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -113,7 +113,7 @@ 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 const struct file_operations stat_fops_per_vm;
>   
>   static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl,
>   			   unsigned long arg);
> @@ -652,9 +652,10 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
>   		stat_data->kvm = kvm;
>   		stat_data->offset = p->offset;
>   		stat_data->mode = p->mode ? p->mode : 0644;
> +		stat_data->kind = p->kind;
>   		kvm->debugfs_stat_data[p - debugfs_entries] = stat_data;
>   		debugfs_create_file(p->name, stat_data->mode, kvm->debugfs_dentry,
> -				    stat_data, stat_fops_per_vm[p->kind]);
> +				    stat_data, &stat_fops_per_vm);
>   	}
>   	return 0;
>   }
> @@ -4033,105 +4034,96 @@ static int kvm_debugfs_release(struct inode *inode, struct file *file)
>   	return 0;
>   }
>   
> -static int vm_stat_get_per_vm(void *data, u64 *val)
> +static int kvm_get_stat_per_vm(struct kvm *kvm, size_t offset, u64 *val)
>   {
> -	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
> -
> -	*val = *(ulong *)((void *)stat_data->kvm + stat_data->offset);
> +	*val = *(ulong *)((void *)kvm + offset);
>   
>   	return 0;
>   }
>   
> -static int vm_stat_clear_per_vm(void *data, u64 val)
> +static int kvm_clear_stat_per_vm(struct kvm *kvm, size_t offset)
>   {
> -	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
> -
> -	if (val)
> -		return -EINVAL;
> -
> -	*(ulong *)((void *)stat_data->kvm + stat_data->offset) = 0;
> +	*(ulong *)((void *)kvm + offset) = 0;
>   
>   	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,
> -				vm_stat_clear_per_vm, "%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  = no_llseek,
> -};
> -
> -static int vcpu_stat_get_per_vm(void *data, u64 *val)
> +static int kvm_get_stat_per_vcpu(struct kvm *kvm, size_t offset, 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 += *(u64 *)((void *)vcpu + stat_data->offset);
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		*val += *(u64 *)((void *)vcpu + offset);
>   
>   	return 0;
>   }
>   
> -static int vcpu_stat_clear_per_vm(void *data, u64 val)
> +static int kvm_clear_stat_per_vcpu(struct kvm *kvm, size_t offset)
>   {
>   	int i;
> -	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
>   	struct kvm_vcpu *vcpu;
>   
> -	if (val)
> -		return -EINVAL;
> -
> -	kvm_for_each_vcpu(i, vcpu, stat_data->kvm)
> -		*(u64 *)((void *)vcpu + stat_data->offset) = 0;
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		*(u64 *)((void *)vcpu + offset) = 0;
>   
>   	return 0;
>   }
>   
> -static int vcpu_stat_get_per_vm_open(struct inode *inode, struct file *file)
> +struct kvm_stat_operations {
> +	int (*get)(struct kvm *kvm, size_t offset, u64 *val);
> +	int (*clear)(struct kvm *kvm, size_t offset);
> +};
> +
> +static const struct kvm_stat_operations kvm_stat_ops[] = {
> +	[KVM_STAT_VM] = { .get = kvm_get_stat_per_vm,
> +			  .clear = kvm_clear_stat_per_vm },
> +	[KVM_STAT_VCPU] = { .get = kvm_get_stat_per_vcpu,
> +			    .clear = kvm_clear_stat_per_vcpu },
> +};
> +
> +static int kvm_stat_data_get(void *data, u64 *val)
> +{
> +	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
> +	return kvm_stat_ops[stat_data->kind].get(stat_data->kvm,
> +						 stat_data->offset, val);

I'm fairly confident the compiler will produce much better code if you 
make this an explicit if/else branch or switch statement rather than go 
through the ops indirection.

> +}
> +
> +static int kvm_stat_data_clear(void *data, u64 val)
> +{
> +	struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
> +	return kvm_stat_ops[stat_data->kind].clear(stat_data->kvm,
> +						   stat_data->offset);

This introduces a change in behavior. Before, on set of a value != 0 you 
would get -EINVAL. Now, the value is just silently ignored. Please add 
back the val != 0 check here.


Apart from the comments above, the patch looks like a simple, useful 
cleanup to me.


Thanks a lot,

Alex

> +}
> +
> +static int kvm_stat_data_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,
> -				 vcpu_stat_clear_per_vm, "%llu\n");
> +	return kvm_debugfs_open(inode, file, kvm_stat_data_get,
> +				kvm_stat_data_clear, "%llu\n");
>   }
>   
> -static const struct file_operations vcpu_stat_get_per_vm_fops = {
> -	.owner   = THIS_MODULE,
> -	.open    = vcpu_stat_get_per_vm_open,
> +static const struct file_operations stat_fops_per_vm = {
> +	.owner = THIS_MODULE,
> +	.open = kvm_stat_data_open,
>   	.release = kvm_debugfs_release,
> -	.read    = simple_attr_read,
> -	.write   = simple_attr_write,
> -	.llseek  = no_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,
> +	.read = simple_attr_read,
> +	.write = simple_attr_write,
> +	.llseek = no_llseek,
>   };
>   
>   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;
>   	mutex_lock(&kvm_lock);
>   	list_for_each_entry(kvm, &vm_list, vm_list) {
> -		stat_tmp.kvm = kvm;
> -		vm_stat_get_per_vm((void *)&stat_tmp, &tmp_val);
> +		vm_stat_get_per_vm(kvm, offset, &tmp_val);
>   		*val += tmp_val;
>   	}
>   	mutex_unlock(&kvm_lock);
> @@ -4142,15 +4134,13 @@ static int vm_stat_clear(void *_offset, u64 val)
>   {
>   	unsigned offset = (long)_offset;
>   	struct kvm *kvm;
> -	struct kvm_stat_data stat_tmp = {.offset = offset};
>   
>   	if (val)
>   		return -EINVAL;
>   
>   	mutex_lock(&kvm_lock);
>   	list_for_each_entry(kvm, &vm_list, vm_list) {
> -		stat_tmp.kvm = kvm;
> -		vm_stat_clear_per_vm((void *)&stat_tmp, 0);
> +		vm_stat_clear_per_vm(kvm, offset);
>   	}
>   	mutex_unlock(&kvm_lock);
>   
> @@ -4163,14 +4153,12 @@ static int vcpu_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;
>   	mutex_lock(&kvm_lock);
>   	list_for_each_entry(kvm, &vm_list, vm_list) {
> -		stat_tmp.kvm = kvm;
> -		vcpu_stat_get_per_vm((void *)&stat_tmp, &tmp_val);
> +		vcpu_stat_get_per_vm(kvm, offset, &tmp_val);
>   		*val += tmp_val;
>   	}
>   	mutex_unlock(&kvm_lock);
> @@ -4181,15 +4169,13 @@ static int vcpu_stat_clear(void *_offset, u64 val)
>   {
>   	unsigned offset = (long)_offset;
>   	struct kvm *kvm;
> -	struct kvm_stat_data stat_tmp = {.offset = offset};
>   
>   	if (val)
>   		return -EINVAL;
>   
>   	mutex_lock(&kvm_lock);
>   	list_for_each_entry(kvm, &vm_list, vm_list) {
> -		stat_tmp.kvm = kvm;
> -		vcpu_stat_clear_per_vm((void *)&stat_tmp, 0);
> +		kvm_clear_stat_per_vcpu(kvm, offset);
>   	}
>   	mutex_unlock(&kvm_lock);
>   
> 



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kvm: Refactor handling of VM debugfs files
  2019-12-10 16:07 [PATCH] kvm: Refactor handling of VM debugfs files Milan Pandurov
@ 2019-12-11 16:33   ` kbuild test robot
  2019-12-11  7:16 ` Alexander Graf
  2019-12-11 16:33   ` kbuild test robot
  2 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2019-12-11 16:33 UTC (permalink / raw)
  To: Milan Pandurov; +Cc: kbuild-all, kvm, pbonzini, rkrcmar, graf, borntraeger

[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]

Hi Milan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on vhost/linux-next v5.5-rc1 next-20191210]
[cannot apply to kvms390/next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Milan-Pandurov/kvm-Refactor-handling-of-VM-debugfs-files/20191211-063504
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: mips-malta_kvm_defconfig (attached as .config)
compiler: mipsel-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/mips/kvm/../../../virt/kvm/kvm_main.c: In function 'vm_stat_get':
>> arch/mips/kvm/../../../virt/kvm/kvm_main.c:4126:3: error: implicit declaration of function 'vm_stat_get_per_vm'; did you mean 'vm_stat_get'? [-Werror=implicit-function-declaration]
      vm_stat_get_per_vm(kvm, offset, &tmp_val);
      ^~~~~~~~~~~~~~~~~~
      vm_stat_get
   arch/mips/kvm/../../../virt/kvm/kvm_main.c: In function 'vm_stat_clear':
>> arch/mips/kvm/../../../virt/kvm/kvm_main.c:4143:3: error: implicit declaration of function 'vm_stat_clear_per_vm'; did you mean 'vm_stat_clear'? [-Werror=implicit-function-declaration]
      vm_stat_clear_per_vm(kvm, offset);
      ^~~~~~~~~~~~~~~~~~~~
      vm_stat_clear
   arch/mips/kvm/../../../virt/kvm/kvm_main.c: In function 'vcpu_stat_get':
>> arch/mips/kvm/../../../virt/kvm/kvm_main.c:4161:3: error: implicit declaration of function 'vcpu_stat_get_per_vm'; did you mean 'vcpu_stat_get'? [-Werror=implicit-function-declaration]
      vcpu_stat_get_per_vm(kvm, offset, &tmp_val);
      ^~~~~~~~~~~~~~~~~~~~
      vcpu_stat_get
   cc1: all warnings being treated as errors

vim +4126 arch/mips/kvm/../../../virt/kvm/kvm_main.c

  4116	
  4117	static int vm_stat_get(void *_offset, u64 *val)
  4118	{
  4119		unsigned offset = (long)_offset;
  4120		struct kvm *kvm;
  4121		u64 tmp_val;
  4122	
  4123		*val = 0;
  4124		mutex_lock(&kvm_lock);
  4125		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4126			vm_stat_get_per_vm(kvm, offset, &tmp_val);
  4127			*val += tmp_val;
  4128		}
  4129		mutex_unlock(&kvm_lock);
  4130		return 0;
  4131	}
  4132	
  4133	static int vm_stat_clear(void *_offset, u64 val)
  4134	{
  4135		unsigned offset = (long)_offset;
  4136		struct kvm *kvm;
  4137	
  4138		if (val)
  4139			return -EINVAL;
  4140	
  4141		mutex_lock(&kvm_lock);
  4142		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4143			vm_stat_clear_per_vm(kvm, offset);
  4144		}
  4145		mutex_unlock(&kvm_lock);
  4146	
  4147		return 0;
  4148	}
  4149	
  4150	DEFINE_SIMPLE_ATTRIBUTE(vm_stat_fops, vm_stat_get, vm_stat_clear, "%llu\n");
  4151	
  4152	static int vcpu_stat_get(void *_offset, u64 *val)
  4153	{
  4154		unsigned offset = (long)_offset;
  4155		struct kvm *kvm;
  4156		u64 tmp_val;
  4157	
  4158		*val = 0;
  4159		mutex_lock(&kvm_lock);
  4160		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4161			vcpu_stat_get_per_vm(kvm, offset, &tmp_val);
  4162			*val += tmp_val;
  4163		}
  4164		mutex_unlock(&kvm_lock);
  4165		return 0;
  4166	}
  4167	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20620 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kvm: Refactor handling of VM debugfs files
@ 2019-12-11 16:33   ` kbuild test robot
  0 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2019-12-11 16:33 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3889 bytes --]

Hi Milan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on vhost/linux-next v5.5-rc1 next-20191210]
[cannot apply to kvms390/next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Milan-Pandurov/kvm-Refactor-handling-of-VM-debugfs-files/20191211-063504
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: mips-malta_kvm_defconfig (attached as .config)
compiler: mipsel-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/mips/kvm/../../../virt/kvm/kvm_main.c: In function 'vm_stat_get':
>> arch/mips/kvm/../../../virt/kvm/kvm_main.c:4126:3: error: implicit declaration of function 'vm_stat_get_per_vm'; did you mean 'vm_stat_get'? [-Werror=implicit-function-declaration]
      vm_stat_get_per_vm(kvm, offset, &tmp_val);
      ^~~~~~~~~~~~~~~~~~
      vm_stat_get
   arch/mips/kvm/../../../virt/kvm/kvm_main.c: In function 'vm_stat_clear':
>> arch/mips/kvm/../../../virt/kvm/kvm_main.c:4143:3: error: implicit declaration of function 'vm_stat_clear_per_vm'; did you mean 'vm_stat_clear'? [-Werror=implicit-function-declaration]
      vm_stat_clear_per_vm(kvm, offset);
      ^~~~~~~~~~~~~~~~~~~~
      vm_stat_clear
   arch/mips/kvm/../../../virt/kvm/kvm_main.c: In function 'vcpu_stat_get':
>> arch/mips/kvm/../../../virt/kvm/kvm_main.c:4161:3: error: implicit declaration of function 'vcpu_stat_get_per_vm'; did you mean 'vcpu_stat_get'? [-Werror=implicit-function-declaration]
      vcpu_stat_get_per_vm(kvm, offset, &tmp_val);
      ^~~~~~~~~~~~~~~~~~~~
      vcpu_stat_get
   cc1: all warnings being treated as errors

vim +4126 arch/mips/kvm/../../../virt/kvm/kvm_main.c

  4116	
  4117	static int vm_stat_get(void *_offset, u64 *val)
  4118	{
  4119		unsigned offset = (long)_offset;
  4120		struct kvm *kvm;
  4121		u64 tmp_val;
  4122	
  4123		*val = 0;
  4124		mutex_lock(&kvm_lock);
  4125		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4126			vm_stat_get_per_vm(kvm, offset, &tmp_val);
  4127			*val += tmp_val;
  4128		}
  4129		mutex_unlock(&kvm_lock);
  4130		return 0;
  4131	}
  4132	
  4133	static int vm_stat_clear(void *_offset, u64 val)
  4134	{
  4135		unsigned offset = (long)_offset;
  4136		struct kvm *kvm;
  4137	
  4138		if (val)
  4139			return -EINVAL;
  4140	
  4141		mutex_lock(&kvm_lock);
  4142		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4143			vm_stat_clear_per_vm(kvm, offset);
  4144		}
  4145		mutex_unlock(&kvm_lock);
  4146	
  4147		return 0;
  4148	}
  4149	
  4150	DEFINE_SIMPLE_ATTRIBUTE(vm_stat_fops, vm_stat_get, vm_stat_clear, "%llu\n");
  4151	
  4152	static int vcpu_stat_get(void *_offset, u64 *val)
  4153	{
  4154		unsigned offset = (long)_offset;
  4155		struct kvm *kvm;
  4156		u64 tmp_val;
  4157	
  4158		*val = 0;
  4159		mutex_lock(&kvm_lock);
  4160		list_for_each_entry(kvm, &vm_list, vm_list) {
> 4161			vcpu_stat_get_per_vm(kvm, offset, &tmp_val);
  4162			*val += tmp_val;
  4163		}
  4164		mutex_unlock(&kvm_lock);
  4165		return 0;
  4166	}
  4167	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 20620 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-12-11 16:34 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-10 16:07 [PATCH] kvm: Refactor handling of VM debugfs files Milan Pandurov
2019-12-11  2:56 ` kbuild test robot
2019-12-11  2:56   ` kbuild test robot
2019-12-11  7:16 ` Alexander Graf
2019-12-11 16:33 ` kbuild test robot
2019-12-11 16:33   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.