Store a virCgroupPtr instance in qemuDomainObjPrivatePtr

Instead of calling virCgroupForDomain every time we need
the virCgrouPtr instance, just do it once at Vm startup
and cache a reference to the object in qemuDomainObjPrivatePtr
until shutdown of the VM. Removing the virCgroupPtr from
the QEMU driver state also means we don't have stale mount
info, if someone mounts the cgroups filesystem after libvirtd
has been started
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

src/qemu/qemu_cgroup.h

@@ -25,26 +25,19 @@
 # define __QEMU_CGROUP_H__
 
 # include "virusb.h"
+# include "vircgroup.h"
 # include "domain_conf.h"
 # include "qemu_conf.h"
 
-struct _qemuCgroupData {
-    virDomainObjPtr vm;
-    virCgroupPtr cgroup;
-};
-typedef struct _qemuCgroupData qemuCgroupData;
-
-bool qemuCgroupControllerActive(virQEMUDriverPtr driver,
-                                int controller);
 int qemuSetupDiskCgroup(virDomainObjPtr vm,
-                        virCgroupPtr cgroup,
                         virDomainDiskDefPtr disk);
 int qemuTeardownDiskCgroup(virDomainObjPtr vm,
-                           virCgroupPtr cgroup,
                            virDomainDiskDefPtr disk);
 int qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev,
                                  const char *path,
                                  void *opaque);
+int qemuInitCgroup(virQEMUDriverPtr driver,
+                   virDomainObjPtr vm);
 int qemuSetupCgroup(virQEMUDriverPtr driver,
                     virDomainObjPtr vm,
                     virBitmapPtr nodemask);
@@ -56,14 +49,11 @@ int qemuSetupCgroupVcpuPin(virCgroupPtr cgroup,
                            int nvcpupin,
                            int vcpuid);
 int qemuSetupCgroupEmulatorPin(virCgroupPtr cgroup, virBitmapPtr cpumask);
-int qemuSetupCgroupForVcpu(virQEMUDriverPtr driver, virDomainObjPtr vm);
+int qemuSetupCgroupForVcpu(virDomainObjPtr vm);
 int qemuSetupCgroupForEmulator(virQEMUDriverPtr driver,
                                virDomainObjPtr vm,
                                virBitmapPtr nodemask);
-int qemuRemoveCgroup(virQEMUDriverPtr driver,
-                     virDomainObjPtr vm,
-                     int quiet);
-int qemuAddToCgroup(virQEMUDriverPtr driver,
-                    virDomainDefPtr def);
+int qemuRemoveCgroup(virDomainObjPtr vm);
+int qemuAddToCgroup(virDomainObjPtr vm);
 
 #endif /* __QEMU_CGROUP_H__ */

src/qemu/qemu_conf.h

@@ -34,7 +34,6 @@
 # include "domain_event.h"
 # include "virthread.h"
 # include "security/security_manager.h"
-# include "vircgroup.h"
 # include "virpci.h"
 # include "virusb.h"
 # include "cpu_conf.h"
@@ -164,9 +163,6 @@ struct _virQEMUDriver {
     /* Atomic increment only */
     int nextvmid;
 
-    /* Immutable pointer. Immutable object */
-    virCgroupPtr cgroup;
-
     /* Atomic inc/dec only */
     unsigned int nactive;
 

src/qemu/qemu_domain.c

@@ -235,6 +235,7 @@ qemuDomainObjPrivateFree(void *data)
 
     virObjectUnref(priv->qemuCaps);
 
+    virCgroupFree(&priv->cgroup);
     qemuDomainPCIAddressSetFree(priv->pciaddrs);
     qemuDomainCCWAddressSetFree(priv->ccwaddrs);
     virDomainChrSourceDefFree(priv->monConfig);

src/qemu/qemu_domain.h

@@ -25,6 +25,7 @@
 # define __QEMU_DOMAIN_H__
 
 # include "virthread.h"
+# include "vircgroup.h"
 # include "domain_conf.h"
 # include "snapshot_conf.h"
 # include "qemu_monitor.h"
@@ -165,6 +166,8 @@ struct _qemuDomainObjPrivate {
     qemuDomainCleanupCallback *cleanupCallbacks;
     size_t ncleanupCallbacks;
     size_t ncleanupCallbacks_max;
+
+    virCgroupPtr cgroup;
 };
 
 struct qemuDomainWatchdogEvent

src/qemu/qemu_hotplug.c

@@ -1136,27 +1136,16 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver,
         goto error;
     }
 
-    if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
-        virCgroupPtr cgroup = NULL;
+    if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) {
         virUSBDevicePtr usb;
-        qemuCgroupData data;
-
-        if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("Unable to find cgroup for %s"),
-                           vm->def->name);
-            goto error;
-        }
 
         if ((usb = virUSBDeviceNew(hostdev->source.subsys.u.usb.bus,
                                 hostdev->source.subsys.u.usb.device,
                                 NULL)) == NULL)
             goto error;
 
-        data.vm = vm;
-        data.cgroup = cgroup;
         if (virUSBDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup,
-                                    &data) < 0) {
+                                    vm) < 0) {
             virUSBDeviceFree(usb);
             goto error;
         }
@@ -2032,7 +2021,6 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
     int i, ret = -1;
     virDomainDiskDefPtr detach = NULL;
     qemuDomainObjPrivatePtr priv = vm->privateData;
-    virCgroupPtr cgroup = NULL;
     char *drivestr = NULL;
 
     i = qemuFindDisk(vm->def, dev->data.disk->dst);
@@ -2052,15 +2040,6 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
-        if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("Unable to find cgroup for %s"),
-                           vm->def->name);
-            goto cleanup;
-        }
-    }
-
     if (STREQLEN(vm->def->os.machine, "s390-ccw", 8) &&
         virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
         if (!virDomainDeviceAddressIsValid(&detach->info,
@@ -2130,11 +2109,9 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
                                             vm->def, dev->data.disk) < 0)
         VIR_WARN("Unable to restore security label on %s", dev->data.disk->src);
 
-    if (cgroup != NULL) {
-        if (qemuTeardownDiskCgroup(vm, cgroup, dev->data.disk) < 0)
-            VIR_WARN("Failed to teardown cgroup for disk path %s",
-                     NULLSTR(dev->data.disk->src));
-    }
+    if (qemuTeardownDiskCgroup(vm, dev->data.disk) < 0)
+        VIR_WARN("Failed to teardown cgroup for disk path %s",
+                 NULLSTR(dev->data.disk->src));
 
     if (virDomainLockDiskDetach(driver->lockManager, vm, dev->data.disk) < 0)
         VIR_WARN("Unable to release lock on %s", dev->data.disk->src);
@@ -2142,7 +2119,6 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
     ret = 0;
 
 cleanup:
-    virCgroupFree(&cgroup);
     VIR_FREE(drivestr);
     return ret;
 }
@@ -2154,7 +2130,6 @@ int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
     int i, ret = -1;
     virDomainDiskDefPtr detach = NULL;
     qemuDomainObjPrivatePtr priv = vm->privateData;
-    virCgroupPtr cgroup = NULL;
     char *drivestr = NULL;
 
     i = qemuFindDisk(vm->def, dev->data.disk->dst);
@@ -2181,15 +2156,6 @@ int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
-        if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("Unable to find cgroup for %s"),
-                           vm->def->name);
-            goto cleanup;
-        }
-    }
-
     /* build the actual drive id string as the disk->info.alias doesn't
      * contain the QEMU_DRIVE_HOST_PREFIX that is passed to qemu */
     if (virAsprintf(&drivestr, "%s%s",
@@ -2222,11 +2188,9 @@ int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
                                             vm->def, dev->data.disk) < 0)
         VIR_WARN("Unable to restore security label on %s", dev->data.disk->src);
 
-    if (cgroup != NULL) {
-        if (qemuTeardownDiskCgroup(vm, cgroup, dev->data.disk) < 0)
-            VIR_WARN("Failed to teardown cgroup for disk path %s",
-                     NULLSTR(dev->data.disk->src));
-    }
+    if (qemuTeardownDiskCgroup(vm, dev->data.disk) < 0)
+        VIR_WARN("Failed to teardown cgroup for disk path %s",
+                 NULLSTR(dev->data.disk->src));
 
     if (virDomainLockDiskDetach(driver->lockManager, vm, dev->data.disk) < 0)
         VIR_WARN("Unable to release lock on disk %s", dev->data.disk->src);
@@ -2235,7 +2199,6 @@ int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
 
 cleanup:
     VIR_FREE(drivestr);
-    virCgroupFree(&cgroup);
     return ret;
 }
 

src/qemu/qemu_migration.c

@@ -4186,7 +4186,6 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
                     enum qemuDomainAsyncJob asyncJob)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
-    virCgroupPtr cgroup = NULL;
     int ret = -1;
     int rc;
     bool restoreLabel = false;
@@ -4220,21 +4219,13 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
          * given cgroup ACL permission.  We might also stumble on
          * a race present in some qemu versions where it does a wait()
          * that botches pclose.  */
-        if (qemuCgroupControllerActive(driver,
-                                       VIR_CGROUP_CONTROLLER_DEVICES)) {
-            if (virCgroupForDomain(driver->cgroup, vm->def->name,
-                                   &cgroup, 0) != 0) {
-                virReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("Unable to find cgroup for %s"),
-                               vm->def->name);
-                goto cleanup;
-            }
-            rc = virCgroupAllowDevicePath(cgroup, path,
+        if (virCgroupHasController(priv->cgroup,
+                                   VIR_CGROUP_CONTROLLER_DEVICES)) {
+            rc = virCgroupAllowDevicePath(priv->cgroup, path,
                                           VIR_CGROUP_DEVICE_RW);
-            virDomainAuditCgroupPath(vm, cgroup, "allow", path, "rw", rc);
+            virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", rc);
             if (rc == 1) {
                 /* path was not a device, no further need for cgroup */
-                virCgroupFree(&cgroup);
             } else if (rc < 0) {
                 virReportSystemError(-rc,
                                      _("Unable to allow device %s for %s"),
@@ -4335,14 +4326,14 @@ cleanup:
                                                  vm->def, path) < 0)
         VIR_WARN("failed to restore save state label on %s", path);
 
-    if (cgroup != NULL) {
-        rc = virCgroupDenyDevicePath(cgroup, path,
+    if (virCgroupHasController(priv->cgroup,
+                               VIR_CGROUP_CONTROLLER_DEVICES)) {
+        rc = virCgroupDenyDevicePath(priv->cgroup, path,
                                      VIR_CGROUP_DEVICE_RWM);
-        virDomainAuditCgroupPath(vm, cgroup, "deny", path, "rwm", rc);
+        virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", rc);
         if (rc < 0)
             VIR_WARN("Unable to deny device %s for %s %d",
                      path, vm->def->name, rc);
-        virCgroupFree(&cgroup);
     }
     return ret;
 }

src/qemu/qemu_process.c

@@ -1395,6 +1395,7 @@ qemuProcessReadLogOutput(virDomainObjPtr vm,
         /* Filter out debug messages from intermediate libvirt process */
         while ((eol = strchr(filter_next, '\n'))) {
             *eol = '\0';
+            VIR_ERROR("<<<<<<<<<<<<%s>>>>>>>>>>", filter_next);
             if (virLogProbablyLogMessage(filter_next)) {
                 memmove(filter_next, eol + 1, got - (eol - buf));
                 got -= eol + 1 - filter_next;
@@ -2529,7 +2530,7 @@ static int qemuProcessHook(void *data)
      * memory allocation is on the correct NUMA node
      */
     VIR_DEBUG("Moving process to cgroup");
-    if (qemuAddToCgroup(h->driver, h->vm->def) < 0)
+    if (qemuAddToCgroup(h->vm) < 0)
         goto cleanup;
 
     /* This must be done after cgroup placement to avoid resetting CPU
@@ -3004,6 +3005,9 @@ qemuProcessReconnect(void *opaque)
     if (qemuUpdateActiveUsbHostdevs(driver, obj->def) < 0)
         goto error;
 
+    if (qemuInitCgroup(driver, obj) < 0)
+        goto error;
+
     /* XXX: Need to change as long as lock is introduced for
      * qemu_driver->sharedDisks.
      */
@@ -3384,7 +3388,7 @@ int qemuProcessStart(virConnectPtr conn,
     /* Ensure no historical cgroup for this VM is lying around bogus
      * settings */
     VIR_DEBUG("Ensuring no historical cgroup is lying around");
-    qemuRemoveCgroup(driver, vm, 1);
+    qemuRemoveCgroup(vm);
 
     for (i = 0 ; i < vm->def->ngraphics; ++i) {
         virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
@@ -3750,7 +3754,7 @@ int qemuProcessStart(virConnectPtr conn,
         goto cleanup;
 
     VIR_DEBUG("Setting cgroup for each VCPU (if required)");
-    if (qemuSetupCgroupForVcpu(driver, vm) < 0)
+    if (qemuSetupCgroupForVcpu(vm) < 0)
         goto cleanup;
 
     VIR_DEBUG("Setting cgroup for emulator (if required)");
@@ -4085,7 +4089,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
     }
 
 retry:
-    if ((ret = qemuRemoveCgroup(driver, vm, 0)) < 0) {
+    if ((ret = qemuRemoveCgroup(vm)) < 0) {
         if (ret == -EBUSY && (retries++ < 5)) {
             usleep(200*1000);
             goto retry;
@@ -4093,6 +4097,7 @@ retry:
         VIR_WARN("Failed to remove cgroup for %s",
                  vm->def->name);
     }
+    virCgroupFree(&priv->cgroup);
 
     qemuProcessRemoveDomainStatus(driver, vm);