Re: [libvirt] [PATCH 1/2] qemu: fix recording of vCPU pids for MTTCG

On 10/30/18 5:25 AM, Daniel P. Berrangé wrote: > On Mon, Oct 29, 2018 at 05:55:36PM -0400, John Ferlan wrote: >> >> >> On 10/17/18 10:15 AM, Daniel P. Berrangé wrote: >>> MTTCG is the new multi-threaded impl of TCG which follows >>> KVM in having one host OS thread per vCPU. Historically >>> we have discarded all PIDs reported for TCG guests, but >>> we must now selectively honour this data. >>> >>> We don't have anything in the domain XML that indicates >>> whether a guest is using TCG or MTTCG. While QEMU does >>> have an option (-accel tcg,thread=single|multi), it is >>> not desirable to expose this in libvirt. QEMU will >>> automatically use MTTCG when the host/guest architecture >>> pairing is known to be safe. Only developers of QEMU TCG >>> have a strong reason to override this logic. >>> >>> Thus we use two sanity checks to decide if the vCPU >>> PID information is usable. First we see if the PID >>> duplicates the main emulator PID, and second we see >>> if the PID duplicates any other vCPUs. >>> >>> Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; >>> --- >>> src/qemu/qemu_domain.c | 81 ++++++++++++++++++++++++++---------------- >>> 1 file changed, 51 insertions(+), 30 deletions(-) >>> >>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >>> index f00f1b3fdb..c7a0c03e3f 100644 >>> --- a/src/qemu/qemu_domain.c >>> +++ b/src/qemu/qemu_domain.c >>> @@ -10326,9 +10326,10 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, >>> qemuDomainVcpuPrivatePtr vcpupriv; >>> qemuMonitorCPUInfoPtr info = NULL; >>> size_t maxvcpus = virDomainDefGetVcpusMax(vm->def); >>> - size_t i; >>> + size_t i, j; >>> bool hotplug; >>> bool fast; >>> + bool validTIDs = true; >>> int rc; >>> int ret = -1; >>> >>> @@ -10336,6 +10337,8 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, >>> fast = virQEMUCapsGet(QEMU_DOMAIN_PRIVATE(vm)->qemuCaps, >>> QEMU_CAPS_QUERY_CPUS_FAST); >>> >>> + VIR_DEBUG("Maxvcpus %zu hotplug %d fast query %d", maxvcpus, hotplug, fast); >>> + >>> if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) >>> return -1; >>> >>> @@ -10348,39 +10351,57 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver, >>> if (rc < 0) >>> goto cleanup; >>> >>> + /* >>> + * The query-cpus[-fast] commands return information >>> + * about the vCPUs, including the OS level PID that >>> + * is executing the vCPU. >>> + * >>> + * For KVM there is always a 1-1 mapping between >>> + * vCPUs and host OS PIDs. >>> + * >>> + * For TCG things are a little more complicated. >>> + * >>> + * - In some cases the vCPUs will all have the same >>> + * PID as the main emulator thread. >>> + * - In some cases the first vCPU will have a distinct >>> + * PID, but other vCPUs will share the emulator thread >>> + * >>> + * For MTTCG, things work the same as KVM, with each >>> + * vCPU getting its own PID. >>> + * >>> + * We use the Host OS PIDs for doing vCPU pinning >>> + * and reporting. The TCG data reporting will result >>> + * in bad behaviour such as pinning the wrong PID. >>> + * We must thus detect and discard bogus PID info >>> + * from TCG, while still honouring the modern MTTCG >>> + * impl which we can support. >>> + */ >>> + for (i = 0; i < maxvcpus && validTIDs; i++) { >>> + if (info[i].tid == vm->pid) { >>> + VIR_DEBUG("vCPU[%zu] PID %llu duplicates process", >>> + i, (unsigned long long)info[i].tid); >>> + validTIDs = false; >>> + } >>> + >> >> If !validTIDs does the next loop matter? IOW: >> >> Should the above section add a "continue;" since the loop exit would >> force the exit? > > Mostly I wanted to ensure that we logged all the vCPU pids and it > won't impose an unreasonable performance impact by doing so. > Those are only VIR_DEBUG's though, so who really sees them? Perhaps using VIR_WARN would be different.