Re: [Xen-devel] [PATCH v3 2/8] libxl: implement virDomainMemorystats

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 11/17/2015 11:15 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>> Introduce support for domainMemoryStats API call, which
>> consequently enables the use of `virsh dommemstat` command to
>> query for memory statistics of a domain. We support
>> the following statistics: balloon info, available and currently
>> in use. swap-in, swap-out, major-faults, minor-faults require
>> cooperation of the guest and thus currently not supported.
>>
>> We build on the data returned from libxl_domain_info and deliver
>> it in the virDomainMemoryStat format.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>> ---
>> Changes since v1:
>>  - Cleanup properly after error fetching domain stats
>>  - Dispose libxl_dominfo after succesfull call to
>>  libxl_domain_info()
>>  - Bump version to 1.2.22
>> ---
>>  src/libxl/libxl_driver.c | 70 
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 50f6e34..f4fc7bc 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -4749,6 +4749,75 @@ libxlDomainGetCPUStats(virDomainPtr dom,
>>      return ret;
>>  }
>>  
>> +#define LIBXL_SET_MEMSTAT(TAG, VAL) \
>> +        if (i < nr_stats) { \
>> +            stats[i].tag = TAG; \
>> +            stats[i].val = VAL; \
>> +            i++; \
>> +        }
>> +
>> +static int
>> +libxlDomainMemoryStats(virDomainPtr dom,
>> +                       virDomainMemoryStatPtr stats,
>> +                       unsigned int nr_stats,
>> +                       unsigned int flags)
>> +{
>> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
>> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>> +    virDomainObjPtr vm;
>> +    libxl_dominfo d_info;
>> +    unsigned mem, maxmem;
>> +    size_t i = 0;
>> +    int ret = -1;
>> +
>> +    virCheckFlags(0, -1);
>> +
>> +    if (!(vm = libxlDomObjFromDomain(dom)))
>> +        goto cleanup;
>> +
>> +    if (virDomainMemoryStatsEnsureACL(dom->conn, vm->def) < 0)
>> +        goto cleanup;
>> +
>> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
>> +        goto cleanup;
>> +
>> +    if (!virDomainObjIsActive(vm)) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID,
>> +                       "%s", _("domain is not running"));
>> +        goto endjob;
>> +    }
>> +
>> +    if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("libxl_domain_info failed for domain '%d'"),
>> +                       vm->def->id);
>> +        goto endjob;
>> +    }
>> +    mem = d_info.current_memkb;
>> +    maxmem = d_info.max_memkb;
>> +
>> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem);
> 
> Should this just be 'mem'? On a domain with
> 
>   <memory unit='KiB'>1048576</memory>
>   <currentMemory unit='KiB'>1048576</currentMemory>
> 
> I see
> 
> # virsh dommemstat test
> actual 1024
> available 1049600
> rss 1048576
> 
> The documentation for VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON only says "Current
> balloon value (in KB)". I (perhaps incorrectly) interpret that as the amount 
> of
> memory currently assigned to the domain.
I interpreted that it based on the size of the balloon instead of based on the
memory. But trying out with plain qemu (to see what the monitor socket returns,
since qemu driver takes those values directly) and also libvirt+qemu and it gave
me what you mentioned. VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON is indeed the
amount of memory currently assigned to the domain. Apologies for the confusion.

> 
> Also, I wonder if we should provide rss for Xen domains? I'm not sure it makes
> much sense since Xen domains are more than just a process running on the host.
> AFAIK, rss would always be set to d_info.current_memkb even if a domain was 
> not
> actually using all the memory.
I agree, but I mainly included RSS stat for the lack of a "current memory in
use". But since this is cleared and ACTUAL_BALLOON is not the size of the
balloon but to the actual memory, I guess this renders RSS stat a bit useless,
and perhaps misleading like you suggest. This is the hunk to be applied on top
of this patch, having tested already, and make {syntax-check,test}-ed.

Regards,
Joao

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index db13fd2..c1563c2 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4799,9 +4799,8 @@ libxlDomainMemoryStats(virDomainPtr dom,
     mem = d_info.current_memkb;
     maxmem = d_info.max_memkb;

-    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem);
+    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, mem);
     LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
-    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);

     ret = i;
> 
> Regards,
> Jim
> 
>> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
>> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);
>> +
>> +    ret = i;
>> +
>> +    libxl_dominfo_dispose(&d_info);
>> +
>> + endjob:
>> +    if (!libxlDomainObjEndJob(driver, vm)) {
>> +        virObjectUnlock(vm);
>> +        vm = NULL;
>> +    }
>> +
>> + cleanup:
>> +    if (vm)
>> +        virObjectUnlock(vm);
>> +    return ret;
>> +}
>> +
>> +#undef LIBXL_SET_MEMSTAT
>> +
>>  static int
>>  libxlConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, 
>> int eventID,
>>                                     virConnectDomainEventGenericCallback 
>> callback,
>> @@ -5342,6 +5411,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>  #endif
>>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
>> +    .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */
>>      .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
>>      .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 
>> */
>>      .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 
>> 0.9.0 */
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel