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

["Pandurov, Milan" <milanpa@xxxxxxxxxx>]

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