[PATCH v5 0/1] KVM: trigger uevents when creating or destroying a VM

[PATCH v5 0/1] KVM: trigger uevents when creating or destroying a VM

[Claudio Imbrenda]

This patch adds a few lines to the KVM common code to fire a
KOBJ_CHANGE uevent whenever a KVM VM is created or destroyed. The event
carries five environment variables:

CREATED indicates how many times a new VM has been created. It is
	useful for example to trigger specific actions when the first
	VM is started
COUNT indicates how many VMs are currently active. This can be used for
	logging or monitoring purposes
PID has the pid of the KVM process that has been started or stopped.
	This can be used to perform process-specific tuning.
STATS_PATH contains the path in debugfs to the directory with all the
	runtime statistics for this VM. This is useful for performance
	monitoring and profiling.
EVENT described the type of event, its value can be either "create" or
	"destroy"

Specific udev rules can be then set up in userspace to deal with the
creation or destruction of VMs as needed.

v4 -> v5:
* fixed compilation issue when cgroups are not configured

v3 -> v4:
* fixed subject line
* reworked kvm_uevent_notify_change to use struct kobj_uevent_env and
  add_uevent_var for improved readability.

v2 -> v3:
* added EVENT
* shortened the names of the other variables

v1 -> v2:
* added KVM_VM_PID and KVM_VM_STATS_PATH
* some cleanup, the patch should look nicer now
* rebased on 4.12

Claudio Imbrenda (1):
  KVM: trigger uevents when creating or destroying a VM

 virt/kvm/kvm_main.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

-- 
2.7.4

[PATCH v5 1/1] KVM: trigger uevents when creating or destroying a VM

[Claudio Imbrenda]

This patch adds a few lines to the KVM common code to fire a
KOBJ_CHANGE uevent whenever a KVM VM is created or destroyed. The event
carries five environment variables:

CREATED indicates how many times a new VM has been created. It is
	useful for example to trigger specific actions when the first
	VM is started
COUNT indicates how many VMs are currently active. This can be used for
	logging or monitoring purposes
PID has the pid of the KVM process that has been started or stopped.
	This can be used to perform process-specific tuning.
STATS_PATH contains the path in debugfs to the directory with all the
	runtime statistics for this VM. This is useful for performance
	monitoring and profiling.
EVENT described the type of event, its value can be either "create" or
	"destroy"

Specific udev rules can be then set up in userspace to deal with the
creation or destruction of VMs as needed.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
---
 virt/kvm/kvm_main.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f0fe9d0..24ad3f5 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -130,6 +130,12 @@ EXPORT_SYMBOL_GPL(kvm_rebooting);
 
 static bool largepages_enabled = true;
 
+#define KVM_EVENT_CREATE_VM 0
+#define KVM_EVENT_DESTROY_VM 1
+static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm);
+static unsigned long long kvm_createvm_count;
+static unsigned long long kvm_active_vms;
+
 bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
 {
 	if (pfn_valid(pfn))
@@ -728,6 +734,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 	int i;
 	struct mm_struct *mm = kvm->mm;
 
+	kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm);
 	kvm_destroy_vm_debugfs(kvm);
 	kvm_arch_sync_events(kvm);
 	spin_lock(&kvm_lock);
@@ -3196,6 +3203,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 		fput(file);
 		return -ENOMEM;
 	}
+	kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
 
 	fd_install(r, file);
 	return r;
@@ -3848,6 +3856,67 @@ static const struct file_operations *stat_fops[] = {
 	[KVM_STAT_VM]   = &vm_stat_fops,
 };
 
+static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm)
+{
+	struct kobj_uevent_env *env;
+	char *tmp, *pathbuf = NULL;
+	unsigned long long created, active;
+
+	if (!kvm_dev.this_device || !kvm)
+		return;
+
+	spin_lock(&kvm_lock);
+	if (type == KVM_EVENT_CREATE_VM) {
+		kvm_createvm_count++;
+		kvm_active_vms++;
+	} else if (type == KVM_EVENT_DESTROY_VM) {
+		kvm_active_vms--;
+	}
+	created = kvm_createvm_count;
+	active = kvm_active_vms;
+	spin_unlock(&kvm_lock);
+
+	env = kzalloc(sizeof(*env), GFP_KERNEL);
+	if (!env)
+		return;
+
+	add_uevent_var(env, "CREATED=%llu", created);
+	add_uevent_var(env, "COUNT=%llu", active);
+
+	if (type == KVM_EVENT_CREATE_VM)
+		add_uevent_var(env, "EVENT=create");
+	else if (type == KVM_EVENT_DESTROY_VM)
+		add_uevent_var(env, "EVENT=destroy");
+
+	if (kvm->debugfs_dentry) {
+		char p[ITOA_MAX_LEN];
+
+		snprintf(p, sizeof(p), "%s", kvm->debugfs_dentry->d_name.name);
+		tmp = strchrnul(p + 1, '-');
+		*tmp = '\0';
+		add_uevent_var(env, "PID=%s", p);
+		pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
+		if (pathbuf) {
+			/* sizeof counts the final '\0' */
+			int len = sizeof("STATS_PATH=") - 1;
+			const char *pvar = "STATS_PATH=";
+
+			tmp = dentry_path_raw(kvm->debugfs_dentry,
+					      pathbuf + len,
+					      PATH_MAX - len);
+			if (!IS_ERR(tmp)) {
+				memcpy(tmp - len, pvar, len);
+				env->envp[env->envp_idx++] = tmp - len;
+			}
+		}
+	}
+	/* no need for checks, since we are adding at most only 5 keys */
+	env->envp[env->envp_idx++] = NULL;
+	kobject_uevent_env(&kvm_dev.this_device->kobj, KOBJ_CHANGE, env->envp);
+	kfree(env);
+	kfree(pathbuf);
+}
+
 static int kvm_init_debug(void)
 {
 	int r = -EEXIST;
-- 
2.7.4

Re: [PATCH v5 1/1] KVM: trigger uevents when creating or destroying a VM

[David Hildenbrand]

> +	add_uevent_var(env, "CREATED=%llu", created);
> +	add_uevent_var(env, "COUNT=%llu", active);

I like that much better.

> +
> +	if (type == KVM_EVENT_CREATE_VM)
> +		add_uevent_var(env, "EVENT=create");
> +	else if (type == KVM_EVENT_DESTROY_VM)
> +		add_uevent_var(env, "EVENT=destroy");
> +
> +	if (kvm->debugfs_dentry) {
> +		char p[ITOA_MAX_LEN];

I'd move this also to the top of the function. Or move tmp and pathbuf
in here, so you could also move them to this place.

> +
> +		snprintf(p, sizeof(p), "%s", kvm->debugfs_dentry->d_name.name);

Probably just me, but I prefer ARRAY_SIZE() instead of sizeof() for any
kind of array sizes.

> +		tmp = strchrnul(p + 1, '-');> +		*tmp = '\0';
> +		add_uevent_var(env, "PID=%s", p);
> +		pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> +		if (pathbuf) {
> +			/* sizeof counts the final '\0' */
> +			int len = sizeof("STATS_PATH=") - 1;
> +			const char *pvar = "STATS_PATH=";

Can't you avoid that? See next paragraph.


> +
> +			tmp = dentry_path_raw(kvm->debugfs_dentry,
> +					      pathbuf + len,
> +					      PATH_MAX - len);
> +			if (!IS_ERR(tmp)) {

Why not

tmp = dentry_path_raw(kvm->debugfs_dentry, pathbuf, PATH_MAX);
if (!IS_ERR(tmp)) {
	add_uevent_var(env, "STATS_PATH=s", tmp);
}

and avoid len / pvar / memcpy? And keep internal env size checking
consistent.

(are my tired eyes missing something? :) )


> +				memcpy(tmp - len, pvar, len);
> +				env->envp[env->envp_idx++] = tmp - len;
> +			}
> +		}
> +	}
> +	/* no need for checks, since we are adding at most only 5 keys */
> +	env->envp[env->envp_idx++] = NULL;
> +	kobject_uevent_env(&kvm_dev.this_device->kobj, KOBJ_CHANGE, env->envp);
> +	kfree(env);
> +	kfree(pathbuf);
> +}
> +
>  static int kvm_init_debug(void)
>  {
>  	int r = -EEXIST;
> 


-- 

Thanks,

David

Re: [PATCH v5 1/1] KVM: trigger uevents when creating or destroying a VM

[Claudio Imbrenda]

On Mon, 17 Jul 2017 17:53:51 +0200
David Hildenbrand <david@redhat.com> wrote:

> > +	add_uevent_var(env, "CREATED=%llu", created);
> > +	add_uevent_var(env, "COUNT=%llu", active);  
> 
> I like that much better.
> 
> > +
> > +	if (type == KVM_EVENT_CREATE_VM)
> > +		add_uevent_var(env, "EVENT=create");
> > +	else if (type == KVM_EVENT_DESTROY_VM)
> > +		add_uevent_var(env, "EVENT=destroy");
> > +
> > +	if (kvm->debugfs_dentry) {
> > +		char p[ITOA_MAX_LEN];  
> 
> I'd move this also to the top of the function. Or move tmp and pathbuf
> in here, so you could also move them to this place.

yeah it would probably look cleaner, I'll fix it.

> > +
> > +		snprintf(p, sizeof(p), "%s",
> > kvm->debugfs_dentry->d_name.name);  
> 
> Probably just me, but I prefer ARRAY_SIZE() instead of sizeof() for
> any kind of array sizes.

I'll fix that too

> > +		tmp = strchrnul(p + 1, '-');> +
> > *tmp = '\0';
> > +		add_uevent_var(env, "PID=%s", p);
> > +		pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> > +		if (pathbuf) {
> > +			/* sizeof counts the final '\0' */
> > +			int len = sizeof("STATS_PATH=") - 1;
> > +			const char *pvar = "STATS_PATH=";  
> 
> Can't you avoid that? See next paragraph.
> 
> 
> > +
> > +			tmp = dentry_path_raw(kvm->debugfs_dentry,
> > +					      pathbuf + len,
> > +					      PATH_MAX - len);
> > +			if (!IS_ERR(tmp)) {  
> 
> Why not
> 
> tmp = dentry_path_raw(kvm->debugfs_dentry, pathbuf, PATH_MAX);
> if (!IS_ERR(tmp)) {
> 	add_uevent_var(env, "STATS_PATH=s", tmp);
> }
> 
> and avoid len / pvar / memcpy? And keep internal env size checking
> consistent.

that would be true, but then we would copy the buffer with the path
twice, once for dentry_path_raw and once for add_uevent_var.

the env size is consistent, since that would count the bytes
effectively present in the buffer, and note that kobject_uevent_env only
wants a char**, no length, so there are no consistency issues. 

I agree that your solution looks better (and I had actually considered
implementing it like that initially), but it can potentially be slower.
I don't really have any strong preference.

Paolo: which solution do you like the most? 

[...]

Re: [PATCH v5 1/1] KVM: trigger uevents when creating or destroying a VM

[Radim Krčmář]

2017-07-17 18:27+0200, Claudio Imbrenda:
> On Mon, 17 Jul 2017 17:53:51 +0200
> David Hildenbrand <david@redhat.com> wrote:
> > > +		tmp = strchrnul(p + 1, '-');> +
> > > *tmp = '\0';
> > > +		add_uevent_var(env, "PID=%s", p);

When we're at it ... PID exists regardless of debugfs, so it would be
nice to provide it unconditionally.
(This interface looks like it was added to discourage the use of debugfs
 directory notifier in simple cases.)

I guess we cannot use task_pid_nr(current) on the KVM_EVENT_DESTROY_VM
path, so saving the PID in struct kvm would be better, IMO (it wastes
memory with larger number of VMs, but the code is harder to break).

> > > +			tmp = dentry_path_raw(kvm->debugfs_dentry,
> > > +					      pathbuf + len,
> > > +					      PATH_MAX - len);
> > > +			if (!IS_ERR(tmp)) {  
> > 
> > Why not
> > 
> > tmp = dentry_path_raw(kvm->debugfs_dentry, pathbuf, PATH_MAX);
> > if (!IS_ERR(tmp)) {
> > 	add_uevent_var(env, "STATS_PATH=s", tmp);
> > }
> > 
> > and avoid len / pvar / memcpy? And keep internal env size checking
> > consistent.
> 
> that would be true, but then we would copy the buffer with the path
> twice, once for dentry_path_raw and once for add_uevent_var.
> 
> the env size is consistent, since that would count the bytes
> effectively present in the buffer, and note that kobject_uevent_env only
> wants a char**, no length, so there are no consistency issues. 
> 
> I agree that your solution looks better (and I had actually considered
> implementing it like that initially), but it can potentially be slower.
> I don't really have any strong preference.
> 
> Paolo: which solution do you like the most? 

I slightly prefer the simpler version as performance of the path is not
that critical.

Please note that the original patch is already in 4.13 (it was ok and my
nitpicking would just make it miss the merge window :]), so the code
improvements need to be a new patch,

thanks.