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

[John Ferlan]

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