Re: [libvirt] [PATCH v3 14/14] qemu: Add swtpm to emulator cgroup

On 05/04/2018 04:21 PM, Stefan Berger wrote: > Add the external swtpm to the emulator cgroup so that upper limits of CPU > usage can be enforced on the emulated TPM. > > To enable this we need to have the swtpm write its process id (pid) into a > file. We then read it from the file to configure the emulator cgroup. > > The PID file is created in /var/run/libvirt/qemu/swtpm: > > [root@localhost swtpm]# ls -lZ /var/run/libvirt/qemu/swtpm/ > total 4 > -rw-r--r--. 1 tss tss system_u:object_r:qemu_var_run_t:s0 5 Apr 10 12:26 1-testvm-swtpm.pid > srw-rw----. 1 qemu qemu system_u:object_r:svirt_image_t:s0:c597,c632 0 Apr 10 12:26 1-testvm-swtpm.sock > > The swtpm command line now looks as follows: > > root@localhost testvm]# ps auxZ | grep swtpm | grep socket | grep -v grep > system_u:system_r:virtd_t:s0:c597,c632 tss 18697 0.0 0.0 28172 3892 ? Ss 16:46 0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/1-testvm-swtpm.sock,mode=0600 --tpmstate dir=/var/lib/libvirt/swtpm/485d0004-a48f-436a-8457-8a3b73e28568/tpm1.2/ --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log --pid file=/var/run/libvirt/qemu/swtpm/1-testvm-swtpm.pid > > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > --- > src/conf/domain_conf.c | 1 + > src/conf/domain_conf.h | 1 + > src/qemu/qemu_cgroup.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_cgroup.h | 1 + > src/qemu/qemu_extdevice.c | 19 +++++++++++++++++ > src/qemu/qemu_process.c | 4 ++++ > src/util/virtpm.c | 33 +++++++++++++++++++++++++++++ > 7 files changed, 112 insertions(+) > So that I don't forget ;-) - there should be a "next" patch as well that adds some text to docs/news.xml in order to describe "at a high level" the new feature or improvement being done. Perhaps something that should be done for the existing series I pushed, but neglected to remember to ask about while reviewing. > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index c98d26a..f542c8e 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -2624,6 +2624,7 @@ void virDomainTPMDefFree(virDomainTPMDefPtr def) > VIR_FREE(def->data.emulator.source.data.nix.path); > VIR_FREE(def->data.emulator.storagepath); > VIR_FREE(def->data.emulator.logfile); > + VIR_FREE(def->data.emulator.pidfile); > break; > case VIR_DOMAIN_TPM_TYPE_LAST: > break; > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 826ff26..49c77f7 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1311,6 +1311,7 @@ struct _virDomainTPMDef { > virDomainChrSourceDef source; > char *storagepath; > char *logfile; > + char *pidfile; > } emulator; > } data; > }; There are varying "arguments" about using/saving a pidfile or pid of a thread in recent posts vis-a-vis the pid stored in some file doesn't match what's running for various reasons. How does one know that that pid in the pidfile matches the pid of the running process without checking. You may want to look at src/util/virpidfile.c and various virPidFile* API's to see if they're useful. Somehow I have a feeling this will be similar to other consumers. Also, see the discussion during pr_helper review v3, patch 9: https://www.redhat.com/archives/libvir-list/2018-March/msg00749.html The pr_helper in the end is a process/thread being started to manage something - much like swtpm is a process/thread managing something (blackbox view). > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c > index 1a5adca..f86ac3f 100644 > --- a/src/qemu/qemu_cgroup.c > +++ b/src/qemu/qemu_cgroup.c > @@ -38,6 +38,7 @@ > #include "virnuma.h" > #include "virsystemd.h" > #include "virdevmapper.h" > +#include "virpidfile.h" > > #define VIR_FROM_THIS VIR_FROM_QEMU > > @@ -1146,6 +1147,58 @@ qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, > > > int > +qemuSetupCgroupForExtDevices(virDomainObjPtr vm) > +{ > + qemuDomainObjPrivatePtr priv = vm->privateData; > + virDomainTPMDefPtr tpm = vm->def->tpm; > + virCgroupPtr cgroup_temp = NULL; > + pid_t pid; > + int ret = -1; > + > + if (priv->cgroup == NULL) > + return 0; /* Not supported, so claim success */ > + Since this only matters "if (tpm && tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR)", then why not check that here and simplify the rest a bit more.

> + /* > + * If CPU cgroup controller is not initialized here, then we need > + * neither period nor quota settings. And if CPUSET controller is > + * not initialized either, then there's nothing to do anyway. > + */ > + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && > + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) > + return 0; > + > + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, > + false, &cgroup_temp) < 0) Currently, the VIR_CGROUP_THREAD_EMULATOR is used for the qemu-kvm emulator (e.g. qemu-kvm process/thread). There's also VCPU and IOTHREAD cgroup enum's. So that makes me wonder if there should be a new "group" for "any" similar thread that's created based on domain configuration.

This type of conversation is a bit out of my comfort zone though. Usually I defer to danpb on this although IIRC mkletzan also has some expertise here. Since this "concept" of having to limit the thread via cgroups has an impact on another series I've recently reviewed (the pr_helper), you should see that I responded today to that series with a question of mprivozn about whether it would be needed there as well, see: https://www.redhat.com/archives/libvir-list/2018-May/msg00579.html

> + goto cleanup; > + > + if (tpm) { > + switch (tpm->type) { > + case VIR_DOMAIN_TPM_TYPE_EMULATOR: > + if (virPidFileReadPath(tpm->data.emulator.pidfile, &pid) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Could not read swtpm's pidfile %s"), > + tpm->data.emulator.pidfile); > + goto cleanup; > + }

> + if (virCgroupAddTask(cgroup_temp, pid) < 0) > + goto cleanup; > + break; > + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: > + case VIR_DOMAIN_TPM_TYPE_LAST: > + break; > + } > + } > + > + ret = 0; > + > +cleanup: > + virCgroupFree(&cgroup_temp); > + > + return ret; > +} > + > + > +int > qemuSetupGlobalCpuCgroup(virDomainObjPtr vm) > { > qemuDomainObjPrivatePtr priv = vm->privateData; > diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h > index 3b8ff60..478bf7e 100644 > --- a/src/qemu/qemu_cgroup.h > +++ b/src/qemu/qemu_cgroup.h > @@ -69,6 +69,7 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, > long long quota); > int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); > int qemuSetupGlobalCpuCgroup(virDomainObjPtr vm); > +int qemuSetupCgroupForExtDevices(virDomainObjPtr vm); > int qemuRemoveCgroup(virDomainObjPtr vm); > > typedef struct _qemuCgroupEmulatorAllNodesData qemuCgroupEmulatorAllNodesData; > diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c > index eb7220d..779e759 100644 > --- a/src/qemu/qemu_extdevice.c > +++ b/src/qemu/qemu_extdevice.c > @@ -148,6 +148,9 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, > virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); > virDomainTPMDefPtr tpm = def->tpm; > char *shortName = virDomainDefGetShortName(def); > + char *pidfiledata = NULL; > + int timeout; > + int len; > > if (!shortName) > return -1; > @@ -195,6 +198,22 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, > goto error; > } > > + /* check that the swtpm has written its pid into the file */ > + timeout = 1000; /* ms */ > + while ((len = virFileReadHeaderQuiet(tpm->data.emulator.pidfile, > + 10, &pidfiledata)) <= 0) { > + if (len == 0 && timeout > 0) { > + timeout -= 50; > + usleep(50 * 1000); > + continue; > + } > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("swtpm did not write pidfile '%s'"), > + tpm->data.emulator.pidfile); > + goto error; > + } > + VIR_FREE(pidfiledata); > + I think the virPidFile definitely helps here although I haven't dug too deep into those API's. > ret = 0; > > cleanup: > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 2b07530..bf0e4da 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -6076,6 +6076,10 @@ qemuProcessLaunch(virConnectPtr conn, > if (qemuProcessSetupEmulator(vm) < 0) > goto cleanup; > > + VIR_DEBUG("Setting cgroup for external devices (if required)"); > + if (qemuSetupCgroupForExtDevices(vm) < 0) > + goto cleanup; > + > VIR_DEBUG("Setting up resctrl"); > if (qemuProcessResctrlCreate(driver, vm) < 0) > goto cleanup; > diff --git a/src/util/virtpm.c b/src/util/virtpm.c > index 0617326..3d8a195 100644 > --- a/src/util/virtpm.c > +++ b/src/util/virtpm.c > @@ -39,6 +39,7 @@ > #include "virlog.h" > #include "virtpm.h" > #include "virutil.h" > +#include "virpidfile.h" > #include "configmake.h" > > #define VIR_FROM_THIS VIR_FROM_NONE > @@ -376,6 +377,25 @@ int virTPMEmulatorInitPaths(virDomainTPMDefPtr tpm, > } > > /* > + * virTPMCreatePidFilename > + */ > +static char *virTPMCreatePidFilename(const char *swtpmStateDir, > + const char *shortName) > +{ > + char *pidfile = NULL; > + char *devname = NULL; > + > + if (virAsprintf(&devname, "%s-swtpm", shortName) < 0) > + return NULL; > + > + pidfile = virPidFileBuildPath(swtpmStateDir, devname); > + > + VIR_FREE(devname); > + > + return pidfile; > +} > + > +/* > * virTPMEmulatorPrepareHost: > * > * @tpm: tpm definition > @@ -442,6 +462,10 @@ int virTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, > goto cleanup; > tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_UNIX; > > + if (!(tpm->data.emulator.pidfile = > + virTPMCreatePidFilename(swtpmStateDir, shortName))) > + goto cleanup; > + Not quite sure why we need to save this especially since during stop processing we don't use it and we can recreate it fairly easily. > ret = 0; > > cleanup: > @@ -619,6 +643,9 @@ virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, const char *vmname, > break; > } > > + virCommandAddArg(cmd, "--pid"); > + virCommandAddArgFormat(cmd, "file=%s", tpm->data.emulator.pidfile); > + This code could just have a local @pidfile = virTPMCreatePidFilename, then once added to the arglist, VIR_FREE(pidfile). Especially since we can build it up on the fly using information we have.

> return cmd; > > error: > @@ -646,6 +673,7 @@ virTPMEmulatorStop(const char *swtpmStateDir, const char *shortName) > virCommandPtr cmd; > char *pathname; > char *errbuf = NULL; > + char *pidfile; > > if (virTPMEmulatorInit() < 0) > return; > @@ -674,6 +702,11 @@ virTPMEmulatorStop(const char *swtpmStateDir, const char *shortName) > unlink(pathname); > > cleanup: > + /* clean up the PID file */ > + if ((pidfile = virTPMCreatePidFilename(swtpmStateDir, shortName))) { > + unlink(pidfile); > + VIR_FREE(pidfile); > + } So why not use tpm->data.emulator.pidfile or pass it and only clean up "if (pidfile) {...}"... IOW: Pass as an argument @pidfile; otherwise, what's the purpose of keeping it around? Curious why is it libvirt's responsibility for this cleanup? If adding "--pid $path" to the swtpm startup has the result of creating a pid file that a consumer can read once started/running, then shouldn't it be the responsibility of that same image/code to remove the pidfile as part of it's clean up processing indicating that the process no longer exists? OK so yeah, swtpm could die, not clean things up, and then what? Well for sure the domain isn't going to last, right? So perhaps on cleanup I could see reason for ensuring the pidfile doesn't exist, but I also assume that swtpm would write a "clean" pid if it finds --pid on the command line. So does it really matter beyond needing to somehow know that swtpm succeeded in starting "this" time.

John > VIR_FREE(pathname); > VIR_FREE(errbuf); > } >