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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list