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@xxxxxxxxxx>
> ---
> 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?
Beyond that the logic and comments look reasonable. I assume since
domain XML doesn't care whether MTTCG or TCG is used and things are
handled under the covers by QEMU that means there's no migration or
save/restore issues. Of course you have a much deeper understanding of
the QEMU code than I do!
The one other question I'd have is should validTIDs setting be done just
once and saved perhaps in the domain private block? There is more than 1
caller (and *Launch can call twice). It's not like it's going to change,
right? So doing the same loop from a hotplug path won't matter nor would
subsequent reconnects or attaches. So perhaps the validTIDs should be a
tristate that only needs to be checked when the value is UNKNOWN. It's
not like the loop is that expensive since it's only numeric comparisons,
so it doesn't matter. I suppose I can be easily convinced taking this
route would be fine, but figured I'd ask.
John
> + for (j = 0; j < i; j++) {
> + if (info[i].tid == info[j].tid) {
> + VIR_DEBUG("vCPU[%zu] PID %llu duplicates vCPU[%zu]",
> + i, (unsigned long long)info[i].tid, j);
> + validTIDs = false;
> + }
> + }
> +
> + if (validTIDs)
> + VIR_DEBUG("vCPU[%zu] PID %llu is valid",
> + i, (unsigned long long)info[i].tid);
> + }
> +
> + VIR_DEBUG("Extracting vCPU information validTIDs=%d", validTIDs);
> for (i = 0; i < maxvcpus; i++) {
> vcpu = virDomainDefGetVcpu(vm->def, i);
> vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);
>
> - /*
> - * Current QEMU *can* report info about host threads mapped
> - * to vCPUs, but it is not in a manner we can correctly
> - * deal with. The TCG CPU emulation does have a separate vCPU
> - * thread, but it runs every vCPU in that same thread. So it
> - * is impossible to setup different affinity per thread.
> - *
> - * What's more the 'query-cpus[-fast]' command returns bizarre
> - * data for the threads. It gives the TCG thread for the
> - * vCPU 0, but for vCPUs 1-> N, it actually replies with
> - * the main process thread ID.
> - *
> - * The result is that when we try to set affinity for
> - * vCPU 1, it will actually change the affinity of the
> - * emulator thread :-( When you try to set affinity for
> - * vCPUs 2, 3.... it will fail if the affinity was
> - * different from vCPU 1.
> - *
> - * We *could* allow vcpu pinning with TCG, if we made the
> - * restriction that all vCPUs had the same mask. This would
> - * at least let us separate emulator from vCPUs threads, as
> - * we do for KVM. It would need some changes to our cgroups
> - * CPU layout though, and error reporting for the config
> - * restrictions.
> - *
> - * Just disable CPU pinning with TCG until someone wants
> - * to try to do this hard work.
> - */
> - if (vm->def->virtType != VIR_DOMAIN_VIRT_QEMU)
> + if (validTIDs)
> vcpupriv->tid = info[i].tid;
>
> vcpupriv->socket_id = info[i].socket_id;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
