[libvirt PATCH v2 1/1] qemu: fix vcpu clearing when multiple vcpu hotunplugs timeout

[Shaleen Bathla]

Problem:
libvirt has a 5 second timeout (generally) for hotplug/unplug
operations which can time out due to heavy load in guest.

vcpu hotunplug occurs one vcpu at a time.
But, if we perform hotplug-unplug repeatedly,
Case 1: qemu sends multiple timedout vcpu unplug notification before
libvirt processed first one.
Case 2: when attempting a hotplug, qemu finishes unplug of another cpu

libvirt waits for an async event notification from qemu regarding
successful vcpu delete.
qemu deletes its vcpu, vcpuinfo and sends notification to libvirt.
libvirt handles vcpu delete notification, and updates vcpuinfo
received from qemu in qemuDomainRefreshVcpuInfo().

qemu's vcpuinfo during refresh will not contain other deleted, sent vcpu
qemu's vcpuinfo will overwrite libvirt's vcpuinfo of the other pending
timedout vcpu(s) which qemu has deleted and notified libvirt.
The overwrite resets other timedout vcpu's alias to NULL and tid to 0.

The error is then seen when validating tid of vcpus.
Example error log:
"internal error: qemu didn't report thread id for vcpu 'XX'"

Solution:
Clear vcpu alias of only the vcpu that hotplug API calls.
Other vcpu-alias won't get reset when vcpuinfo refresh occurs.
Validation is also done for corresponding vcpus only, not all.

Co-authored-by: Partha Satapathy <partha.satapathy at oracle.com>
Signed-off-by: Shaleen Bathla <shaleen.bathla at oracle.com>
---
 src/qemu/qemu_domain.c  |  6 ++++--
 src/qemu/qemu_hotplug.c | 39 +++++++++++++++++++++++++++++++--------
 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3435da5bdc4c..6ae842d0e32f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9773,8 +9773,10 @@ qemuDomainRefreshVcpuInfo(virDomainObj *vm,
         vcpupriv->vcpus = info[i].vcpus;
         VIR_FREE(vcpupriv->type);
         vcpupriv->type = g_steal_pointer(&info[i].type);
-        VIR_FREE(vcpupriv->alias);
-        vcpupriv->alias = g_steal_pointer(&info[i].alias);
+        if (info[i].alias) {
+            VIR_FREE(vcpupriv->alias);
+            vcpupriv->alias = g_steal_pointer(&info[i].alias);
+        }
         virJSONValueFree(vcpupriv->props);
         vcpupriv->props = g_steal_pointer(&info[i].props);
         vcpupriv->enable_id = info[i].id;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index da92ced2f444..f11b90a220ac 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6090,6 +6090,8 @@ qemuDomainRemoveVcpu(virDomainObj *vm,
     unsigned int nvcpus = vcpupriv->vcpus;
     virErrorPtr save_error = NULL;
     size_t i;
+    bool hasVcpuPids = qemuDomainHasVcpuPids(vm);
+    bool rollback = false;

     if (qemuDomainRefreshVcpuInfo(vm, VIR_ASYNC_JOB_NONE, false) < 0)
         return -1;
@@ -6097,14 +6099,26 @@ qemuDomainRemoveVcpu(virDomainObj *vm,
     /* 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 = false;
+        VIR_FREE(vcpupriv->alias); /* clear vcpu alias of only this vcpu */
+
+        if (vcpupriv->tid != 0) {
+            if (hasVcpuPids)
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("qemu reported thread id for inactive vcpu '%zu'"), i);
+
+            rollback = true;
+            break;
+        }
     }

-    if (qemuDomainValidateVcpuInfo(vm) < 0) {
+    if (rollback) {
         /* rollback vcpu count if the setting has failed */
         virDomainAuditVcpu(vm, oldvcpus, oldvcpus - nvcpus, "update", false);

-        for (i = vcpu; i < vcpu + nvcpus; i++) {
+        for (; i >= vcpu; i--) {
             vcpuinfo = virDomainDefGetVcpu(vm->def, i);
             vcpuinfo->online = true;
         }
@@ -6141,6 +6155,9 @@ qemuDomainRemoveVcpuAlias(virDomainObj *vm,
             return;
         }
     }
+
+    VIR_DEBUG("%s not found in vcpulist of domain %s ",
+              alias, vm->def->name);
 }


@@ -6209,6 +6226,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriver *driver,
     int rc;
     int oldvcpus = virDomainDefGetVcpus(vm->def);
     size_t i;
+    bool hasVcpuPids = qemuDomainHasVcpuPids(vm);

     if (!qemuDomainSupportsNewVcpuHotplug(vm)) {
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
@@ -6245,14 +6263,19 @@ qemuDomainHotplugAddVcpu(virQEMUDriver *driver,

         vcpuinfo->online = true;

-        if (vcpupriv->tid > 0 &&
-            qemuProcessSetupVcpu(vm, i, true) < 0)
-            return -1;
+        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;
+            }
+        }
     }

-    if (qemuDomainValidateVcpuInfo(vm) < 0)
-        return -1;
-
     qemuDomainVcpuPersistOrder(vm->def);

     if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0)
--
2.31.1

V1 Patch :
https://listman.redhat.com/archives/libvir-list/2022-November/235470.html

V1 Patch Review comments :
https://listman.redhat.com/archives/libvir-list/2022-November/235534.html

Changes in V2 since V1:
- Patch addresses review comments from Peter Krempa.
- Validation is done for per cpu hotplug entity instead of all