Re: [libvirt] [PATCH 1/2] conf: introduce 'deflate-on-oom' attribute for memballoon device

On 12/14/2015 06:35 AM, Dmitry Andreev wrote: > Excessive memory balloon inflation can cause invocation of OOM-killer, > when Linux is under severe memory pressure. QEMU memballoon device > has a feature to release some memory at the last moment before some > process will be get killed by OOM-killer. > > Introduced attribute allows to enable or disable this feature. > --- > docs/formatdomain.html.in | 10 ++++++++++ > docs/schemas/domaincommon.rng | 5 +++++ > src/conf/domain_conf.c | 23 +++++++++++++++++++++++ > src/conf/domain_conf.h | 1 + > 4 files changed, 39 insertions(+) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index a8bd48e..fa68e7b 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -5954,6 +5954,16 @@ qemu-kvm -net nic,model=? /dev/null > <li>'xen' &mdash; default with Xen</li> > </ul> > </dd> > + <dt><code>deflate-on-oom</code></dt> > + <dd> > + <p> > + The optional <code>deflate-on-oom</code> attribute allows to > + enable/disable (values "on"/"off", respectively) the ability of the > + QEMU virtio memory balloon to release some memory at the last moment > + before a guest's process get killed by OOM-killer. > + <span class="since">Since 1.3.1, QEMU and KVM only</span> > + </p> > + </dd> > <dt><code>period</code></dt> > <dd> > <p> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 4804c69..9587849 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -3415,6 +3415,11 @@ > <value>none</value> > </choice> > </attribute> > + <optional> > + <attribute name="deflate-on-oom"> Typically don't want those "-", either use some sort of single word or other attributes to have "_"

This will of course have ramifications throughout the code. I'll only mention it here though. > + <ref name="virOnOff"/> > + </attribute> > + </optional> > <interleave> > <optional> > <ref name="alias"/> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 5200c27..b332097 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -11312,6 +11312,7 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, > unsigned int flags) > { > char *model; > + char *deflate; > virDomainMemballoonDefPtr def; > xmlNodePtr save = ctxt->node; > unsigned int period = 0; > @@ -11332,6 +11333,13 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, > goto error; > } > > + if ((deflate = virXMLPropString(node, "deflate-on-oom")) && > + (def->deflate = virTristateSwitchTypeFromString(deflate)) < 0) { Since you're not using/allowing "default", then the comparison should be <= 0 yes, there are some others that are not correct it seems. > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("invalid deflate-on-oom attribute value '%s'"), deflate); > + goto error; > + } > + > ctxt->node = node; > if (virXPathUInt("string(./stats/@period)", ctxt, &period) < -1) { > virReportError(VIR_ERR_XML_ERROR, "%s", > @@ -11350,6 +11358,7 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, > > cleanup: > VIR_FREE(model); > + VIR_FREE(deflate); > > ctxt->node = save; > return def; > @@ -17223,6 +17232,15 @@ virDomainMemballoonDefCheckABIStability(virDomainMemballoonDefPtr src, > return false; > } > > + if (src->deflate != dst->deflate) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Target balloon deflate-on-oom attribute value " > + "%s does not match source %s"), > + virTristateSwitchTypeToString(dst->deflate), > + virTristateSwitchTypeToString(src->deflate)); > + return false; > + } > + > if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) > return false; > > @@ -20390,6 +20408,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf, > unsigned int flags) > { > const char *model = virDomainMemballoonModelTypeToString(def->model); > + const char *deflate = virTristateSwitchTypeToString(def->deflate); > virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; > int indent = virBufferGetIndent(buf, false); > > @@ -20400,6 +20419,10 @@ virDomainMemballoonDefFormat(virBufferPtr buf, > } > > virBufferAsprintf(buf, "<memballoon model='%s'", model); > + > + if (def->deflate > 0 && deflate) This would be use of def->deflate != VIR_TRISTATE_SWITCH_ABSENT > + virBufferAsprintf(buf, " deflate-on-oom='%s'", deflate); The 'deflate' isn't required - see other examples. Looks OK otherwise. John > + > virBufferAdjustIndent(&childrenBuf, indent + 2); > > if (def->period) > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index cec681a..707ffb2 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1634,6 +1634,7 @@ struct _virDomainMemballoonDef { > int model; > virDomainDeviceInfo info; > int period; /* seconds between collections */ > + int deflate; /* enum virTristateSwitch */ > }; > > struct _virDomainNVRAMDef { >