* [PATCH v2] kvm: Refactor handling of VM debugfs files
@ 2019-12-12 9:22 Milan Pandurov
2019-12-12 9:34 ` Alexander Graf
0 siblings, 1 reply; 5+ messages in thread
From: Milan Pandurov @ 2019-12-12 9:22 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, rkrcmar, graf, borntraeger
We can store reference to kvm_stats_debugfs_item instead of copying
its values to kvm_stat_data.
This allows us to remove duplicated code and usage of temporary
kvm_stat_data inside vm_stat_get et al.
Signed-off-by: Milan Pandurov <milanpa@amazon.de>
---
v1 -> v2:
- fix compile issues
- address comments
---
include/linux/kvm_host.h | 7 +-
virt/kvm/kvm_main.c | 154 +++++++++++++++++++++------------------
2 files changed, 90 insertions(+), 71 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 7ed1e2f8641e..d3f2c0eae857 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1109,9 +1109,8 @@ enum kvm_stat_kind {
};
struct kvm_stat_data {
- int offset;
- int mode;
struct kvm *kvm;
+ struct kvm_stats_debugfs_item *dbgfs_item;
};
struct kvm_stats_debugfs_item {
@@ -1120,6 +1119,10 @@ struct kvm_stats_debugfs_item {
enum kvm_stat_kind kind;
int mode;
};
+
+#define KVM_DBGFS_GET_MODE(dbgfs_item) \
+ ((dbgfs_item)->mode ? (dbgfs_item)->mode : 0644)
+
extern struct kvm_stats_debugfs_item debugfs_entries[];
extern struct dentry *kvm_debugfs_dir;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 00268290dcbd..5d2e8ad40975 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);
@@ -650,11 +650,11 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
return -ENOMEM;
stat_data->kvm = kvm;
- stat_data->offset = p->offset;
- stat_data->mode = p->mode ? p->mode : 0644;
+ stat_data->dbgfs_item = p;
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]);
+ debugfs_create_file(p->name, KVM_DBGFS_GET_MODE(p),
+ kvm->debugfs_dentry, stat_data,
+ &stat_fops_per_vm);
}
return 0;
}
@@ -4013,8 +4013,9 @@ static int kvm_debugfs_open(struct inode *inode, struct file *file,
return -ENOENT;
if (simple_attr_open(inode, file, get,
- stat_data->mode & S_IWUGO ? set : NULL,
- fmt)) {
+ KVM_DBGFS_GET_MODE(stat_data->dbgfs_item) & 0222
+ ? set : NULL,
+ fmt)) {
kvm_put_kvm(stat_data->kvm);
return -ENOMEM;
}
@@ -4033,105 +4034,127 @@ 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 *)kvm + offset);
+
+ return 0;
+}
- *val = *(ulong *)((void *)stat_data->kvm + stat_data->offset);
+static int kvm_clear_stat_per_vm(struct kvm *kvm, size_t offset)
+{
+ *(ulong *)((void *)kvm + offset) = 0;
return 0;
}
-static int vm_stat_clear_per_vm(void *data, u64 val)
+static int kvm_get_stat_per_vcpu(struct kvm *kvm, size_t offset, u64 *val)
{
- struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
+ int i;
+ struct kvm_vcpu *vcpu;
- if (val)
- return -EINVAL;
+ *val = 0;
- *(ulong *)((void *)stat_data->kvm + stat_data->offset) = 0;
+ kvm_for_each_vcpu(i, vcpu, kvm)
+ *val += *(u64 *)((void *)vcpu + offset);
return 0;
}
-static int vm_stat_get_per_vm_open(struct inode *inode, struct file *file)
+static int kvm_clear_stat_per_vcpu(struct kvm *kvm, size_t offset)
{
- __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");
+ int i;
+ struct kvm_vcpu *vcpu;
+
+ kvm_for_each_vcpu(i, vcpu, kvm)
+ *(u64 *)((void *)vcpu + offset) = 0;
+
+ return 0;
}
-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,
+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 vcpu_stat_get_per_vm(void *data, u64 *val)
+static int kvm_stat_data_get(void *data, u64 *val)
{
- int i;
+ int r = 0;
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);
+ switch (stat_data->dbgfs_item->kind) {
+ case KVM_STAT_VM:
+ r = kvm_get_stat_per_vm(stat_data->kvm,
+ stat_data->dbgfs_item->offset, val);
+ break;
+ case KVM_STAT_VCPU:
+ r = kvm_get_stat_per_vcpu(stat_data->kvm,
+ stat_data->dbgfs_item->offset, val);
+ break;
+ default:
+ r = -EFAULT;
+ }
- return 0;
+ return r;
}
-static int vcpu_stat_clear_per_vm(void *data, u64 val)
+static int kvm_stat_data_clear(void *data, u64 val)
{
- int i;
+ int r = 0;
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;
+ switch (stat_data->dbgfs_item->kind) {
+ case KVM_STAT_VM:
+ r = kvm_clear_stat_per_vm(stat_data->kvm,
+ stat_data->dbgfs_item->offset);
+ break;
+ case KVM_STAT_VCPU:
+ r = kvm_clear_stat_per_vcpu(stat_data->kvm,
+ stat_data->dbgfs_item->offset);
+ break;
+ default:
+ r = -EFAULT;
+ }
- return 0;
+ return r;
}
-static int vcpu_stat_get_per_vm_open(struct inode *inode, struct file *file)
+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);
+ kvm_get_stat_per_vm(kvm, offset, &tmp_val);
*val += tmp_val;
}
mutex_unlock(&kvm_lock);
@@ -4142,15 +4165,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);
+ kvm_clear_stat_per_vm(kvm, offset);
}
mutex_unlock(&kvm_lock);
@@ -4163,14 +4184,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);
+ kvm_get_stat_per_vcpu(kvm, offset, &tmp_val);
*val += tmp_val;
}
mutex_unlock(&kvm_lock);
@@ -4181,15 +4200,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);
@@ -4262,9 +4279,8 @@ static void kvm_init_debug(void)
kvm_debugfs_num_entries = 0;
for (p = debugfs_entries; p->name; ++p, kvm_debugfs_num_entries++) {
- int mode = p->mode ? p->mode : 0644;
- debugfs_create_file(p->name, mode, kvm_debugfs_dir,
- (void *)(long)p->offset,
+ debugfs_create_file(p->name, KVM_DBGFS_GET_MODE(p),
+ kvm_debugfs_dir, (void *)(long)p->offset,
stat_fops[p->kind]);
}
}
--
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] 5+ messages in thread
* Re: [PATCH v2] kvm: Refactor handling of VM debugfs files
2019-12-12 9:22 [PATCH v2] kvm: Refactor handling of VM debugfs files Milan Pandurov
@ 2019-12-12 9:34 ` Alexander Graf
2019-12-12 14:08 ` Pandurov, Milan
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Graf @ 2019-12-12 9:34 UTC (permalink / raw)
To: Milan Pandurov, kvm; +Cc: pbonzini, rkrcmar, borntraeger
On 12.12.19 11:22, Milan Pandurov wrote:
> We can store reference to kvm_stats_debugfs_item instead of copying
> its values to kvm_stat_data.
> This allows us to remove duplicated code and usage of temporary
> kvm_stat_data inside vm_stat_get et al.
>
> Signed-off-by: Milan Pandurov <milanpa@amazon.de>
>
> ---
> v1 -> v2:
> - fix compile issues
> - address comments
> ---
> include/linux/kvm_host.h | 7 +-
> virt/kvm/kvm_main.c | 154 +++++++++++++++++++++------------------
> 2 files changed, 90 insertions(+), 71 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 7ed1e2f8641e..d3f2c0eae857 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1109,9 +1109,8 @@ enum kvm_stat_kind {
> };
>
> struct kvm_stat_data {
> - int offset;
> - int mode;
> struct kvm *kvm;
> + struct kvm_stats_debugfs_item *dbgfs_item;
> };
>
> struct kvm_stats_debugfs_item {
> @@ -1120,6 +1119,10 @@ struct kvm_stats_debugfs_item {
> enum kvm_stat_kind kind;
> int mode;
> };
> +
> +#define KVM_DBGFS_GET_MODE(dbgfs_item) \
> + ((dbgfs_item)->mode ? (dbgfs_item)->mode : 0644)
> +
> extern struct kvm_stats_debugfs_item debugfs_entries[];
> extern struct dentry *kvm_debugfs_dir;
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 00268290dcbd..5d2e8ad40975 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);
> @@ -650,11 +650,11 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
> return -ENOMEM;
>
> stat_data->kvm = kvm;
> - stat_data->offset = p->offset;
> - stat_data->mode = p->mode ? p->mode : 0644;
> + stat_data->dbgfs_item = p;
> 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]);
> + debugfs_create_file(p->name, KVM_DBGFS_GET_MODE(p),
> + kvm->debugfs_dentry, stat_data,
> + &stat_fops_per_vm);
> }
> return 0;
> }
> @@ -4013,8 +4013,9 @@ static int kvm_debugfs_open(struct inode *inode, struct file *file,
> return -ENOENT;
>
> if (simple_attr_open(inode, file, get,
> - stat_data->mode & S_IWUGO ? set : NULL,
> - fmt)) {
> + KVM_DBGFS_GET_MODE(stat_data->dbgfs_item) & 0222
Why do you change the mask from S_IWUGO to 0222?
> + ? set : NULL,
> + fmt)) {
> kvm_put_kvm(stat_data->kvm);
> return -ENOMEM;
> }
> @@ -4033,105 +4034,127 @@ 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 *)kvm + offset);
> +
> + return 0;
> +}
>
> - *val = *(ulong *)((void *)stat_data->kvm + stat_data->offset);
> +static int kvm_clear_stat_per_vm(struct kvm *kvm, size_t offset)
> +{
> + *(ulong *)((void *)kvm + offset) = 0;
>
> return 0;
> }
>
> -static int vm_stat_clear_per_vm(void *data, u64 val)
> +static int kvm_get_stat_per_vcpu(struct kvm *kvm, size_t offset, u64 *val)
> {
> - struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
> + int i;
> + struct kvm_vcpu *vcpu;
>
> - if (val)
> - return -EINVAL;
> + *val = 0;
>
> - *(ulong *)((void *)stat_data->kvm + stat_data->offset) = 0;
> + kvm_for_each_vcpu(i, vcpu, kvm)
> + *val += *(u64 *)((void *)vcpu + offset);
>
> return 0;
> }
>
> -static int vm_stat_get_per_vm_open(struct inode *inode, struct file *file)
> +static int kvm_clear_stat_per_vcpu(struct kvm *kvm, size_t offset)
> {
> - __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");
> + int i;
> + struct kvm_vcpu *vcpu;
> +
> + kvm_for_each_vcpu(i, vcpu, kvm)
> + *(u64 *)((void *)vcpu + offset) = 0;
> +
> + return 0;
> }
>
> -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,
> +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 },
Why is this struct still here?
> };
>
> -static int vcpu_stat_get_per_vm(void *data, u64 *val)
> +static int kvm_stat_data_get(void *data, u64 *val)
> {
> - int i;
> + int r = 0;
Just initialize this to -EFAULT ...
> 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);
> + switch (stat_data->dbgfs_item->kind) {
> + case KVM_STAT_VM:
> + r = kvm_get_stat_per_vm(stat_data->kvm,
> + stat_data->dbgfs_item->offset, val);
> + break;
> + case KVM_STAT_VCPU:
> + r = kvm_get_stat_per_vcpu(stat_data->kvm,
> + stat_data->dbgfs_item->offset, val);
> + break;
> + default:
> + r = -EFAULT;
... and then drop default: here.
> + }
>
> - return 0;
> + return r;
> }
>
> -static int vcpu_stat_clear_per_vm(void *data, u64 val)
> +static int kvm_stat_data_clear(void *data, u64 val)
> {
> - int i;
> + int r = 0;
Same
Alex
> 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;
> + switch (stat_data->dbgfs_item->kind) {
> + case KVM_STAT_VM:
> + r = kvm_clear_stat_per_vm(stat_data->kvm,
> + stat_data->dbgfs_item->offset);
> + break;
> + case KVM_STAT_VCPU:
> + r = kvm_clear_stat_per_vcpu(stat_data->kvm,
> + stat_data->dbgfs_item->offset);
> + break;
> + default:
> + r = -EFAULT;
> + }
>
> - return 0;
> + return r;
> }
>
> -static int vcpu_stat_get_per_vm_open(struct inode *inode, struct file *file)
> +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);
> + kvm_get_stat_per_vm(kvm, offset, &tmp_val);
> *val += tmp_val;
> }
> mutex_unlock(&kvm_lock);
> @@ -4142,15 +4165,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);
> + kvm_clear_stat_per_vm(kvm, offset);
> }
> mutex_unlock(&kvm_lock);
>
> @@ -4163,14 +4184,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);
> + kvm_get_stat_per_vcpu(kvm, offset, &tmp_val);
> *val += tmp_val;
> }
> mutex_unlock(&kvm_lock);
> @@ -4181,15 +4200,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);
>
> @@ -4262,9 +4279,8 @@ static void kvm_init_debug(void)
>
> kvm_debugfs_num_entries = 0;
> for (p = debugfs_entries; p->name; ++p, kvm_debugfs_num_entries++) {
> - int mode = p->mode ? p->mode : 0644;
> - debugfs_create_file(p->name, mode, kvm_debugfs_dir,
> - (void *)(long)p->offset,
> + debugfs_create_file(p->name, KVM_DBGFS_GET_MODE(p),
> + kvm_debugfs_dir, (void *)(long)p->offset,
> stat_fops[p->kind]);
> }
> }
>
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] 5+ messages in thread
* Re: [PATCH v2] kvm: Refactor handling of VM debugfs files
2019-12-12 9:34 ` Alexander Graf
@ 2019-12-12 14:08 ` Pandurov, Milan
2019-12-12 15:12 ` Christian Borntraeger
0 siblings, 1 reply; 5+ messages in thread
From: Pandurov, Milan @ 2019-12-12 14:08 UTC (permalink / raw)
To: Alexander Graf, Milan Pandurov, kvm; +Cc: pbonzini, rkrcmar, borntraeger
On 12.12.19 10:34, Alexander Graf wrote:
>
>
> On 12.12.19 11:22, Milan Pandurov wrote:
>> We can store reference to kvm_stats_debugfs_item instead of copying
>> its values to kvm_stat_data.
>> This allows us to remove duplicated code and usage of temporary
>> kvm_stat_data inside vm_stat_get et al.
>>
>> Signed-off-by: Milan Pandurov <milanpa@amazon.de>
>>
>> ---
>> v1 -> v2:
>> - fix compile issues
>> - address comments
>> ---
>> include/linux/kvm_host.h | 7 +-
>> virt/kvm/kvm_main.c | 154 +++++++++++++++++++++------------------
>> 2 files changed, 90 insertions(+), 71 deletions(-)
>>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 7ed1e2f8641e..d3f2c0eae857 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -1109,9 +1109,8 @@ enum kvm_stat_kind {
>> };
>> struct kvm_stat_data {
>> - int offset;
>> - int mode;
>> struct kvm *kvm;
>> + struct kvm_stats_debugfs_item *dbgfs_item;
>> };
>> struct kvm_stats_debugfs_item {
>> @@ -1120,6 +1119,10 @@ struct kvm_stats_debugfs_item {
>> enum kvm_stat_kind kind;
>> int mode;
>> };
>> +
>> +#define KVM_DBGFS_GET_MODE(dbgfs_item) \
>> + ((dbgfs_item)->mode ? (dbgfs_item)->mode : 0644)
>> +
>> extern struct kvm_stats_debugfs_item debugfs_entries[];
>> extern struct dentry *kvm_debugfs_dir;
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 00268290dcbd..5d2e8ad40975 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);
>> @@ -650,11 +650,11 @@ static int kvm_create_vm_debugfs(struct kvm
>> *kvm, int fd)
>> return -ENOMEM;
>> stat_data->kvm = kvm;
>> - stat_data->offset = p->offset;
>> - stat_data->mode = p->mode ? p->mode : 0644;
>> + stat_data->dbgfs_item = p;
>> 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]);
>> + debugfs_create_file(p->name, KVM_DBGFS_GET_MODE(p),
>> + kvm->debugfs_dentry, stat_data,
>> + &stat_fops_per_vm);
>> }
>> return 0;
>> }
>> @@ -4013,8 +4013,9 @@ static int kvm_debugfs_open(struct inode
>> *inode, struct file *file,
>> return -ENOENT;
>> if (simple_attr_open(inode, file, get,
>> - stat_data->mode & S_IWUGO ? set : NULL,
>> - fmt)) {
>> + KVM_DBGFS_GET_MODE(stat_data->dbgfs_item) & 0222
>
> Why do you change the mask from S_IWUGO to 0222?
checkpatch was complaining: "Symbolic permissions 'S_IWUGO' are not
preferred. Consider using octal permissions '0222'."
I will change it back to S_IWUGO in next revision to avoid confusion.
>
>> + ? set : NULL,
>> + fmt)) {
>> kvm_put_kvm(stat_data->kvm);
>> return -ENOMEM;
>> }
>> @@ -4033,105 +4034,127 @@ 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 *)kvm + offset);
>> +
>> + return 0;
>> +}
>> - *val = *(ulong *)((void *)stat_data->kvm + stat_data->offset);
>> +static int kvm_clear_stat_per_vm(struct kvm *kvm, size_t offset)
>> +{
>> + *(ulong *)((void *)kvm + offset) = 0;
>> return 0;
>> }
>> -static int vm_stat_clear_per_vm(void *data, u64 val)
>> +static int kvm_get_stat_per_vcpu(struct kvm *kvm, size_t offset, u64
>> *val)
>> {
>> - struct kvm_stat_data *stat_data = (struct kvm_stat_data *)data;
>> + int i;
>> + struct kvm_vcpu *vcpu;
>> - if (val)
>> - return -EINVAL;
>> + *val = 0;
>> - *(ulong *)((void *)stat_data->kvm + stat_data->offset) = 0;
>> + kvm_for_each_vcpu(i, vcpu, kvm)
>> + *val += *(u64 *)((void *)vcpu + offset);
>> return 0;
>> }
>> -static int vm_stat_get_per_vm_open(struct inode *inode, struct
>> file *file)
>> +static int kvm_clear_stat_per_vcpu(struct kvm *kvm, size_t offset)
>> {
>> - __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");
>> + int i;
>> + struct kvm_vcpu *vcpu;
>> +
>> + kvm_for_each_vcpu(i, vcpu, kvm)
>> + *(u64 *)((void *)vcpu + offset) = 0;
>> +
>> + return 0;
>> }
>> -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,
>> +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 },
>
> Why is this struct still here?
>
>> };
>> -static int vcpu_stat_get_per_vm(void *data, u64 *val)
>> +static int kvm_stat_data_get(void *data, u64 *val)
>> {
>> - int i;
>> + int r = 0;
>
> Just initialize this to -EFAULT ...
>
>> 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);
>> + switch (stat_data->dbgfs_item->kind) {
>> + case KVM_STAT_VM:
>> + r = kvm_get_stat_per_vm(stat_data->kvm,
>> + stat_data->dbgfs_item->offset, val);
>> + break;
>> + case KVM_STAT_VCPU:
>> + r = kvm_get_stat_per_vcpu(stat_data->kvm,
>> + stat_data->dbgfs_item->offset, val);
>> + break;
>> + default:
>> + r = -EFAULT;
>
> ... and then drop default: here.
>
>> + }
>> - return 0;
>> + return r;
>> }
>> -static int vcpu_stat_clear_per_vm(void *data, u64 val)
>> +static int kvm_stat_data_clear(void *data, u64 val)
>> {
>> - int i;
>> + int r = 0;
>
> Same
>
>
> Alex
>
>> 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;
>> + switch (stat_data->dbgfs_item->kind) {
>> + case KVM_STAT_VM:
>> + r = kvm_clear_stat_per_vm(stat_data->kvm,
>> + stat_data->dbgfs_item->offset);
>> + break;
>> + case KVM_STAT_VCPU:
>> + r = kvm_clear_stat_per_vcpu(stat_data->kvm,
>> + stat_data->dbgfs_item->offset);
>> + break;
>> + default:
>> + r = -EFAULT;
>> + }
>> - return 0;
>> + return r;
>> }
>> -static int vcpu_stat_get_per_vm_open(struct inode *inode, struct
>> file *file)
>> +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);
>> + kvm_get_stat_per_vm(kvm, offset, &tmp_val);
>> *val += tmp_val;
>> }
>> mutex_unlock(&kvm_lock);
>> @@ -4142,15 +4165,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);
>> + kvm_clear_stat_per_vm(kvm, offset);
>> }
>> mutex_unlock(&kvm_lock);
>> @@ -4163,14 +4184,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);
>> + kvm_get_stat_per_vcpu(kvm, offset, &tmp_val);
>> *val += tmp_val;
>> }
>> mutex_unlock(&kvm_lock);
>> @@ -4181,15 +4200,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);
>> @@ -4262,9 +4279,8 @@ static void kvm_init_debug(void)
>> kvm_debugfs_num_entries = 0;
>> for (p = debugfs_entries; p->name; ++p,
>> kvm_debugfs_num_entries++) {
>> - int mode = p->mode ? p->mode : 0644;
>> - debugfs_create_file(p->name, mode, kvm_debugfs_dir,
>> - (void *)(long)p->offset,
>> + debugfs_create_file(p->name, KVM_DBGFS_GET_MODE(p),
>> + kvm_debugfs_dir, (void *)(long)p->offset,
>> stat_fops[p->kind]);
>> }
>> }
>>
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] 5+ messages in thread
* Re: [PATCH v2] kvm: Refactor handling of VM debugfs files
2019-12-12 14:08 ` Pandurov, Milan
@ 2019-12-12 15:12 ` Christian Borntraeger
2019-12-12 15:16 ` Christian Borntraeger
0 siblings, 1 reply; 5+ messages in thread
From: Christian Borntraeger @ 2019-12-12 15:12 UTC (permalink / raw)
To: Pandurov, Milan, Alexander Graf, Milan Pandurov, kvm; +Cc: pbonzini, rkrcmar
On 12.12.19 15:08, Pandurov, Milan wrote:
>
> On 12.12.19 10:34, Alexander Graf wrote:
>>> @@ -4013,8 +4013,9 @@ static int kvm_debugfs_open(struct inode *inode, struct file *file,
>>> return -ENOENT;
>>> if (simple_attr_open(inode, file, get,
>>> - stat_data->mode & S_IWUGO ? set : NULL,
>>> - fmt)) {
>>> + KVM_DBGFS_GET_MODE(stat_data->dbgfs_item) & 0222
>>
>> Why do you change the mask from S_IWUGO to 0222?
> checkpatch was complaining: "Symbolic permissions 'S_IWUGO' are not preferred. Consider using octal permissions '0222'."
> I will change it back to S_IWUGO in next revision to avoid confusion.
I think most people prefer the octal ones. Linus was very vocal about the S_xxxxx being
incomprehensible, so please keep it octal.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] kvm: Refactor handling of VM debugfs files
2019-12-12 15:12 ` Christian Borntraeger
@ 2019-12-12 15:16 ` Christian Borntraeger
0 siblings, 0 replies; 5+ messages in thread
From: Christian Borntraeger @ 2019-12-12 15:16 UTC (permalink / raw)
To: Pandurov, Milan, Alexander Graf, Milan Pandurov, kvm; +Cc: pbonzini, rkrcmar
On 12.12.19 16:12, Christian Borntraeger wrote:
>
>
> On 12.12.19 15:08, Pandurov, Milan wrote:
>>
>> On 12.12.19 10:34, Alexander Graf wrote:
>
>>>> @@ -4013,8 +4013,9 @@ static int kvm_debugfs_open(struct inode *inode, struct file *file,
>>>> return -ENOENT;
>>>> if (simple_attr_open(inode, file, get,
>>>> - stat_data->mode & S_IWUGO ? set : NULL,
>>>> - fmt)) {
>>>> + KVM_DBGFS_GET_MODE(stat_data->dbgfs_item) & 0222
>>>
>>> Why do you change the mask from S_IWUGO to 0222?
>> checkpatch was complaining: "Symbolic permissions 'S_IWUGO' are not preferred. Consider using octal permissions '0222'."
>> I will change it back to S_IWUGO in next revision to avoid confusion.
>
>
> I think most people prefer the octal ones. Linus was very vocal about the S_xxxxx being
> incomprehensible, so please keep it octal.
https://lwn.net/Articles/696229/
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-12 15:16 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-12 9:22 [PATCH v2] kvm: Refactor handling of VM debugfs files Milan Pandurov
2019-12-12 9:34 ` Alexander Graf
2019-12-12 14:08 ` Pandurov, Milan
2019-12-12 15:12 ` Christian Borntraeger
2019-12-12 15:16 ` Christian Borntraeger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).