Re: [libvirt PATCH v3 4/5] qemu: allow use of async teardown in domain

On 7/5/23 08:20, Boris Fiuczynski wrote: > Asynchronous teardown can be specified if the QEMU binary supports it by > adding in the domain XML > > <features> > ... > <async-teardown enabled='yes|no'/> > ... > </features> > > By default this new feature is disabled. > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > Reviewed-by: Thomas Huth <thuth(a)redhat.com&gt; > --- > docs/formatdomain.rst | 6 +++ > src/conf/domain_conf.c | 22 ++++++++++ > src/conf/domain_conf.h | 1 + > src/conf/schemas/domaincommon.rng | 9 ++++ > src/qemu/qemu_command.c | 20 +++++++++ > src/qemu/qemu_validate.c | 9 ++++ > .../async-teardown.x86_64-latest.args | 37 ++++++++++++++++ > tests/qemuxml2argvdata/async-teardown.xml | 31 +++++++++++++ > ...0-async-teardown-disabled.s390x-6.0.0.args | 35 +++++++++++++++ > ...-async-teardown-disabled.s390x-latest.args | 36 +++++++++++++++ > .../s390-async-teardown-disabled.xml | 24 ++++++++++ > ...async-teardown-no-attrib.s390x-latest.args | 36 +++++++++++++++ > .../s390-async-teardown-no-attrib.xml | 24 ++++++++++ > .../s390-async-teardown.s390x-6.0.0.err | 1 + > .../s390-async-teardown.s390x-latest.args | 36 +++++++++++++++ > .../qemuxml2argvdata/s390-async-teardown.xml | 24 ++++++++++ > tests/qemuxml2argvtest.c | 7 +++ > .../async-teardown.x86_64-latest.xml | 44 +++++++++++++++++++ > ...90-async-teardown-disabled.s390x-6.0.0.xml | 36 +++++++++++++++ > ...0-async-teardown-disabled.s390x-latest.xml | 36 +++++++++++++++ > ...-async-teardown-no-attrib.s390x-latest.xml | 36 +++++++++++++++ > .../s390-async-teardown.s390x-latest.xml | 36 +++++++++++++++ > tests/qemuxml2xmltest.c | 6 +++ > 23 files changed, 552 insertions(+) > create mode 100644 tests/qemuxml2argvdata/async-teardown.x86_64-latest.args > create mode 100644 tests/qemuxml2argvdata/async-teardown.xml > create mode 100644 tests/qemuxml2argvdata/s390-async-teardown-disabled.s390x-6.0.0.args > create mode 100644 tests/qemuxml2argvdata/s390-async-teardown-disabled.s390x-latest.args > create mode 100644 tests/qemuxml2argvdata/s390-async-teardown-disabled.xml > create mode 100644 tests/qemuxml2argvdata/s390-async-teardown-no-attrib.s390x-latest.args > create mode 100644 tests/qemuxml2argvdata/s390-async-teardown-no-attrib.xml > create mode 100644 tests/qemuxml2argvdata/s390-async-teardown.s390x-6.0.0.err > create mode 100644 tests/qemuxml2argvdata/s390-async-teardown.s390x-latest.args > create mode 100644 tests/qemuxml2argvdata/s390-async-teardown.xml > create mode 100644 tests/qemuxml2xmloutdata/async-teardown.x86_64-latest.xml > create mode 100644 tests/qemuxml2xmloutdata/s390-async-teardown-disabled.s390x-6.0.0.xml > create mode 100644 tests/qemuxml2xmloutdata/s390-async-teardown-disabled.s390x-latest.xml > create mode 100644 tests/qemuxml2xmloutdata/s390-async-teardown-no-attrib.s390x-latest.xml > create mode 100644 tests/qemuxml2xmloutdata/s390-async-teardown.s390x-latest.xml > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > index f29449f749..98273c87ad 100644 > --- a/docs/formatdomain.rst > +++ b/docs/formatdomain.rst > @@ -2000,6 +2000,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off. > <tcg> > <tb-cache unit='MiB'>128</tb-cache> > </tcg> > + <async-teardown enabled='yes'/> > </features> > ... > > @@ -2230,6 +2231,11 @@ are: > tb-cache The size of translation block cache size an integer (a multiple of MiB) :since:`8.0.0` > =========== ============================================== =================================================== ============== > > +``async-teardown`` > + Depending on the ``enabled`` attribute (values ``yes``, ``no``) enable or > + disable QEMU asynchronous teardown to improve memory reclaiming on a guest. > + :since:`Since 9.5.0` (QEMU only) Unfortunately, this has missed 9.5.0 timeframe.

> + > Time keeping > ------------ > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 4121b6a054..5ac5c0b771 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -181,6 +181,7 @@ VIR_ENUM_IMPL(virDomainFeature, > "sbbc", > "ibs", > "tcg", > + "async-teardown", > ); > > VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, > @@ -16689,6 +16690,20 @@ virDomainFeaturesDefParse(virDomainDef *def, > return -1; > break; > > + case VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN: { > + virTristateBool enabled; > + > + if (virXMLPropTristateBool(nodes[i], "enabled", > + VIR_XML_PROP_NONE, &enabled) < 0) > + return -1; > + > + if (enabled == VIR_TRISTATE_BOOL_ABSENT) > + enabled = VIR_TRISTATE_BOOL_YES; > + > + def->features[val] = enabled; > + break; > + } > + > case VIR_DOMAIN_FEATURE_LAST: > break; > } > @@ -20628,6 +20643,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src, > > case VIR_DOMAIN_FEATURE_MSRS: > case VIR_DOMAIN_FEATURE_TCG: > + case VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN: > case VIR_DOMAIN_FEATURE_LAST: > break; > } > @@ -27340,6 +27356,12 @@ virDomainDefFormatFeatures(virBuffer *buf, > virDomainFeatureTCGFormat(&childBuf, def); > break; > > + case VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN: > + if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT) > + virBufferAsprintf(&childBuf, "<async-teardown enabled='%s'/>\n", > + virTristateBoolTypeToString(def->features[i])); > + break; > + > case VIR_DOMAIN_FEATURE_LAST: > break; > } > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index cddaa3824d..c857ba556f 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -2170,6 +2170,7 @@ typedef enum { > VIR_DOMAIN_FEATURE_SBBC, > VIR_DOMAIN_FEATURE_IBS, > VIR_DOMAIN_FEATURE_TCG, > + VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN, > > VIR_DOMAIN_FEATURE_LAST > } virDomainFeature; > diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng > index fcf9e00600..c2f56b0490 100644 > --- a/src/conf/schemas/domaincommon.rng > +++ b/src/conf/schemas/domaincommon.rng > @@ -6660,6 +6660,15 @@ > <optional> > <ref name="tcgfeatures"/> > </optional> > + <optional> > + <element name="async-teardown"> > + <optional> > + <attribute name="enabled"> > + <ref name="virYesNo"/> > + </attribute> > + </optional> > + </element> > + </optional> > </interleave> > </element> > </optional> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index cde6ab4dde..3d386e1738 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -10175,6 +10175,23 @@ qemuBuildCryptoCommandLine(virCommand *cmd, > } > > > +static int > +qemuBuildAsyncTeardownCommandLine(virCommand *cmd, > + const virDomainDef *def, > + virQEMUCaps *qemuCaps) > +{ > + g_autofree char *async = NULL; > + > + if (def->features[VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN] != VIR_TRISTATE_BOOL_ABSENT && For this ^^ > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN)) { > + async = g_strdup_printf("async-teardown=%s", > + virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_ASYNC_TEARDOWN])); and this ^^ let me use a variable. It's going to be more readable that way.

> + virCommandAddArgList(cmd, "-run-with", async, NULL); > + } > + return 0; > +} Michal