[PATCH v2 3/3] qemu: Provide virDomainGetCPUStats() implementation for session connection

[Martin Kletzander]

On Mon, Jan 23, 2023 at 02:12:06PM +0100, Michal Prívozník wrote:
>On 1/19/23 15:18, Martin Kletzander wrote:
>> On Wed, Jan 18, 2023 at 10:58:19AM +0100, Michal Privoznik wrote:
>>> We have virDomainGetCPUStats() API which offers querying
>>> statistics on host CPU usage by given guest. And it works in two
>>> modes: getting overall stats (@start_cpu == -1, @ncpus == 1) or
>>> getting per host CPU usage.
>>>
>>> For the QEMU driver it is implemented by looking into values
>>> stored in corresponding cpuacct CGroup controller. Well, this
>>> works for system instances, where libvirt has permissions to
>>> create CGroups and place QEMU process into them. But it does not
>>> fly for session connection, where no CGroups are set up.
>>>
>>> Fortunately, we can do something similar to v8.8.0-rc1~95 and use
>>> virProcessGetStatInfo() to fill the overall stats. Unfortunately,
>>> I haven't found any source of per host CPU usage, so we just
>>> continue throwing an error in that case.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>> src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 50 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 25c681bfd2..cc10dd87e2 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -16006,6 +16006,50 @@ qemuDomainGetMetadata(virDomainPtr dom,
>>>     return ret;
>>> }
>>>
>>> +#define QEMU_CPU_STATS_PROC_TOTAL 3
>>> +
>>> +static int
>>> +qemuDomainGetCPUStatsProc(virDomainObj *vm,
>>> +                          virTypedParameterPtr params,
>>> +                          unsigned int nparams)
>>
>> The only thing I do not like about this patch is that we would now have
>> both:
>>
>>     static int
>>     qemuDomainGetCPUStatsProc(virDomainObj *vm,
>>                               virTypedParameterPtr params,
>>                               unsigned int nparams)
>>
>> and:
>>
>>     static int
>>     qemuDomainGetStatsCpuProc(virDomainObj *vm,
>>                               virTypedParamList *params)
>>
>> doing almost, but not completely, the same thing.  And there is no nice
>> way to avoid it.  I just haven't noticed in v1, sorry.  Or is there a
>> way to converge at least some of these?
>
>Yeah, I realized this too. But, there's a difference between
>virTypedParameter and virTypedParamList. The former is allocated by user
>and we fill it out, while the latter is allocated by us. For instance,
>users interested in just the very first stat, might allocate space for
>it and call us like:
>
>  qemuDomainGetCPUStatsProc(vm, &params, 1);
>
>(obviously, they would call the public API, but in the end this is how
>the function would be called). That is not possible with virTypedParamList.
>
>Another difference is in the semantics: when the function is called with
>nparams == 0, then it's supposed to return the number of supported stats.
>
>We could invent some glue function that would cover these differences,
>but I worry that it would end up being longer than this new function I'm
>inventing (and less readable too).
>

As I said I think there is no nice way to avoid it.  Even if we figure
out something there is no need to hold this back just for a few lines.

Reviewed-by: Martin Kletzander <mkletzan at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20230123/920ffa99/attachment.sig>