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

[Dmitry Andreev]

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