Re: [PATCH] qemu: fix vcpu clearing when multiple vcpu hotunplugs timeout

On Fri, Nov 04, 2022 at 17:52:26 +0530, Shaleen Bathla wrote: So the problem you are describing happens when qemu either: 1) bunches up two unplug confirmations before libvirt processed the first one In this case the call to 'qemuDomainRemoveVcpu' to remove the first cpu already gets updated CPU data where both cpus are removed. Thus qemuDomainValidateVcpuInfo fails. 2) when attempting a hotplug at which point qemu finishes unplug of another cpu After the new cpu is successfully plugged in the updated data doesn't contain the unplugged cpu's data. To correct the hotplug/unplug code paths (qemuDomainHotplugAddVcpu/qemuDomainRemoveVcpu) I think a better solution will be to validate only the single CPU entity which is handled by the corresponding hotplug API. This means replacing the qemuDomainValidateVcpuInfo by modifying the following block (qemuDomainHotplugAddVcpu) to do the validation: /* validation requires us to set the expected state prior to calling it */ for (i = vcpu; i < vcpu + nvcpus; i++) { vcpuinfo = virDomainDefGetVcpu(vm->def, i); vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo); vcpuinfo->online = true; if (vcpupriv->tid > 0 && qemuProcessSetupVcpu(vm, i, true) < 0) return -1; } and modify it such that it does the validation internally: bool hasVcpuPids = qemuDomainHasVcpuPids(vm)); [...] /* Validate that the new vcpus have thread IDs if needed */ for (i = vcpu; i < vcpu + nvcpus; i++) { vcpuinfo = virDomainDefGetVcpu(vm->def, i); vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo); vcpuinfo->online = true; if (vcpupriv->tid > 0) { if (qemuProcessSetupVcpu(vm, i, true) < 0) return -1; } else { if (hasVcpuPids) { virReportError(VIR_ERR_INTERNAL_ERROR, _("qemu didn't report thread id for vcpu '%zu'"), i); return -1; } } } Similarly for the unplug code path. The code in qemuDomainRefreshVcpuInfo which refreshes the alias should also do so only when the data reported from qemu do report it: if (info[i].alias) { VIR_FREE(vcpupriv->alias); vcpupriv->alias = g_steal_pointer(&info[i].alias); } That way you will not need the additional variable. The alias will then be cleared in qemuDomainRemoveVcpu after handling the unplug finalizing steps. Note that the sub-sequent review comments were written before the above paragraph. Add extra newline here to separate the blocks. Ideally also add a comment explaining that if 'hp_timeout_alias' is set there's a pending device removal so the thread ID might not be present any more. This debug statement doesn't seem to make too much sense here. Missing corresponding freeing of this new field in qemuDomainVcpuPrivateDispose. That is required regardless of if you clear it in other ways. Checking the pointer before a free is not needed. g_free can handle NULL pointers. This can be deferred to qemuDomainVcpuPrivateDispose. Drop this debug statement. It doesn't matter how we found it. Same as above. This VIR_INFO call is not needed as the above ee Declare these helper variables in the appropriate loop block/scope. The VIR_INFO call purely logs the message but does not set the 'virError' object which is returned to the caller. Since this function returns failure (it's called directly from qemuDomainSetVcpu, and no other error is reported) in this case the error must be set properly. If this is an error always use 'virReportError'. If it is something that should be ignored, in most cases VIR_DEBUG is the proper logging statement. Additionally this code makes no sense here at all looking back at how live hotplug is handled. In cases when the CPU is still considered online by libvirt (your code specifically handles this case), then the control flow never reaches this point, but rather the caller 'qemuDomainSetVcpuInternal' calls 'qemuDomainFilterHotplugVcpuEntities' which doesn't allow onlining a cpu which is already online. Same as above. Similarly to the additional note, this code is here not wanted either, as it prevents re-sending of the device deletion request which might be needed in some cases.