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

[Peter Krempa]

On Thu, Nov 17, 2022 at 21:39:58 +0530, Shaleen Bathla wrote:
> ping

Sorry I was sick so didn't get to this until now.

> On Fri, Nov 11, 2022 at 02:54:38PM +0530, Shaleen Bathla wrote:
> > 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 */

This comment no longer makes sense after this commit.

> >      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);

Actually we can report this without checking hasVcpuPids as if qemu
doesn't report thread ids the vcpu will not have any.

> > +
> > +            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;

We don't need to roll back when we don't set it in the first place.

> >          }
> > @@ -6141,6 +6155,9 @@ qemuDomainRemoveVcpuAlias(virDomainObj *vm,
> >              return;
> >          }
> >      }
> > +
> > +    VIR_DEBUG("%s not found in vcpulist of domain %s ",
> > +              alias, vm->def->name);

We prefer quotes around substitutions to make it clear what's the added
part.

> >  }
> > 
> > 
> > @@ -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;

Well this way the code doesn't setup cgroups for any other cpus. This
has to be checked at the end of the loop.

> > +            }
> > +        }
> >      }
> > 
> > -    if (qemuDomainValidateVcpuInfo(vm) < 0)
> > -        return -1;
> > -
> >      qemuDomainVcpuPersistOrder(vm->def);
> > 
> >      if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0)

I'll be posting a fixed version with all my suggestions addressed.
Please make sure to give it a test.