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

On 17.12.2015 23:25, John Ferlan wrote: > > > 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 "_" Is it OK to use 'deflation' or 'deflate' word? Can't choose the name by myself.

> > 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 { >>