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

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 11/18/2015 11:05 AM, Joao Martins wrote:
>
> 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;

Looks good, ACK with that squashed in. I've pushed it now.

Regards,
Jim


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