[v2] kvm: Refactor handling of VM debugfs files
diff mbox series

Message ID 20191212092206.7732-1-milanpa@amazon.de
State New
Headers show
Series
  • [v2] kvm: Refactor handling of VM debugfs files
Related show

Commit Message

Milan Pandurov Dec. 12, 2019, 9:22 a.m. UTC
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(-)

Comments

Graf (AWS), Alexander Dec. 12, 2019, 9:34 a.m. UTC | #1
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
Pandurov, Milan Dec. 12, 2019, 2:08 p.m. UTC | #2
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
Christian Borntraeger Dec. 12, 2019, 3:12 p.m. UTC | #3
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.
Christian Borntraeger Dec. 12, 2019, 3:16 p.m. UTC | #4
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/

Patch
diff mbox series

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]);
 	}
 }