| Message ID | 20191212092206.7732-1-milanpa@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] kvm: Refactor handling of VM debugfs files | expand |
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
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
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.
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/
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]); } }
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(-)