Re: [PATCH] kvm: Refactor handling of VM debugfs files

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxx>
---
  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






[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux