[libvirt] [PATCH v2 3/3] vz: add memory statistics

[Nikolay Shirokovskiy]

On 25.06.2015 20:36, Dmitry Guryanov wrote:
> On 06/18/2015 12:28 PM, Nikolay Shirokovskiy wrote:
>> From: Nikolay Shirokovskiy <nshirokovskiy at parallels.com>
>>
>> Implemented counters:
>>   VIR_DOMAIN_MEMORY_STAT_SWAP_IN
>>   VIR_DOMAIN_MEMORY_STAT_SWAP_OUT
>>   VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT
>>   VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT
>>   VIR_DOMAIN_MEMORY_STAT_AVAILABLE
>>   VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON
>>   VIR_DOMAIN_MEMORY_STAT_UNUSED
>>
>> Comments.
>>
>> 1. Use vzDomObjFromDomainRef/virDomainObjEndAPI pair to get domain
>> object as we use prlsdkGetStatsParam. See previous statistics
>> comments.
>>
>> 2. Balloon statistics is not applicable to containers. Fault
>> statistics for containers not provided in PCS6 yet.
> 
> At some reason it returns -1 for all counters on my server, need to check, why is it...
You need an recent build of PCS6 update10 and update guest tools for VMs too.
> 
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>> ---
>>   src/vz/vz_driver.c |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 73 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
>> index 4197569..8dae7c4 100644
>> --- a/src/vz/vz_driver.c
>> +++ b/src/vz/vz_driver.c
>> @@ -1377,6 +1377,78 @@ vzDomainInterfaceStats(virDomainPtr domain,
>>       return ret;
>>   }
>>   +static int
>> +vzDomainMemoryStats(virDomainPtr domain,
>> +                      virDomainMemoryStatPtr stats,
>> +                      unsigned int nr_stats,
>> +                      unsigned int flags)
>> +{
>> +    virDomainObjPtr dom = NULL;
>> +    int ret = -1;
>> +    long long v = 0, t = 0, u = 0;
>> +    size_t i = 0;
>> +
>> +    virCheckFlags(0, -1);
>> +    if (!(dom = vzDomObjFromDomainRef(domain)))
>> +        return -1;
>> +
>> +#define PARALLELS_GET_COUNTER(NAME, VALUE)                          \
>> +    if (prlsdkGetStatsParam(dom, NAME, &VALUE) < 0)                 \
>> +        goto cleanup;                                               \
>> +
>> +#define PARALLELS_MEMORY_STAT_SET(TAG, VALUE)                       \
>> +    if (i < nr_stats) {                                             \
>> +        stats[i].tag = (TAG);                                       \
>> +        stats[i].val = (VALUE);                                     \
>> +        i++;                                                        \
>> +    }
>> +
>> +#define PARALLELS_COUNTER_PROTECT(VALUE) VALUE == -1 : ?
>> +
>> +    i = 0;
>> +
>> +    // count to kb
>> +    PARALLELS_GET_COUNTER("guest.ram.swap_in", v)
>> +    if (v != -1)
>> +        PARALLELS_MEMORY_STAT_SET(VIR_DOMAIN_MEMORY_STAT_SWAP_IN, v << 12)
>> +
>> +    PARALLELS_GET_COUNTER("guest.ram.swap_out", v)
>> +    if (v != -1)
>> +        PARALLELS_MEMORY_STAT_SET(VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, v << 12)
>> +
>> +    PARALLELS_GET_COUNTER("guest.ram.minor_fault", v)
>> +    if (v != -1)
>> +        PARALLELS_MEMORY_STAT_SET(VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT, v)
>> +
>> +    PARALLELS_GET_COUNTER("guest.ram.major_fault", v)
>> +    if (v != -1)
>> +        PARALLELS_MEMORY_STAT_SET(VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT, v)
> 
> To be honest, I don't think macros here improve code readability, look, how it would be without them:
> 
> if (prlsdkGetStatsParam(dom, "guest.ram.major_fault", &v) < 0)
>     goto cleanup;
> 
> if (v != -1 && i < nr_stats) {
>     stats[i].tag = VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT;
>     stats[i].val = v;
>     i++;
> }
> 
> 
> Only "goto cleanup" and "i++" are repeating information.
> 

But with macros it is easy to see actual structure:
1. you get some counter
2. check if it is present (!= -1) 
3. save to libvirt structure optionally converting to different units

Without conversion this could be just 1 line. May be introduce conversion macros.

Then we could get just:

PARALLELS_GET_COUNTER("guest.ram.swap_out", VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, BYTES_TO_PAGES)
PARALLELS_GET_COUNTER("guest.ram.minor_fault", VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT, NO_CONVERSION)

Without macros it is entangled with counters vector capacity checks( i < nr_stats),
incrementing counters. It looks like low-level but there is no wish to
factor it out to type.

>> +
>> +    PARALLELS_GET_COUNTER("guest.ram.total", v)
>> +    if (v != -1)
>> +        PARALLELS_MEMORY_STAT_SET(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, v << 10)
>> +
>> +    PARALLELS_GET_COUNTER("guest.ram.balloon_actual", v)
>> +    if (v != -1)
>> +        PARALLELS_MEMORY_STAT_SET(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, v << 10)
>> +
>> +    PARALLELS_GET_COUNTER("guest.ram.usage", u)
>> +    PARALLELS_GET_COUNTER("guest.ram.total", t)
>> +    if (u != -1 && t != -1)
>> +        PARALLELS_MEMORY_STAT_SET(VIR_DOMAIN_MEMORY_STAT_UNUSED, (t - u) << 10)
>> +
>> +
>> +#undef PARALLELS_GET_COUNTER
>> +#undef PARALLELS_MEMORY_STAT_SET
>> +
>> +    ret = i;
>> + cleanup:
>> +    if (dom)
>> +        virDomainObjEndAPI(&dom);
>> +
>> +    return ret;
>> +}
>> +
>>   static virHypervisorDriver vzDriver = {
>>       .name = "vz",
>>       .connectOpen = vzConnectOpen,            /* 0.10.0 */
>> @@ -1429,6 +1501,7 @@ static virHypervisorDriver vzDriver = {
>>       .domainBlockStats = vzDomainBlockStats, /* 1.3.0 */
>>       .domainBlockStatsFlags = vzDomainBlockStatsFlags, /* 1.3.0 */
>>       .domainInterfaceStats = vzDomainInterfaceStats, /* 1.3.0 */
>> +    .domainMemoryStats = vzDomainMemoryStats, /* 1.3.0 */
>>   };
>>     static virConnectDriver vzConnectDriver = {
> 
>