[libvirt] [PATCH] test_driver: implement virDomainGetCPUStats

Ilias Stamatis posted 1 patch 4 years, 8 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190718100243.1953-1-stamatis.iliass@gmail.com
There is a newer version of this series
src/test/test_driver.c | 131 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 131 insertions(+)
[libvirt] [PATCH] test_driver: implement virDomainGetCPUStats
Posted by Ilias Stamatis 4 years, 8 months ago
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
---
 src/test/test_driver.c | 131 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 131 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index fcb80c9e47..2907c043cb 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3258,6 +3258,136 @@ static int testDomainSetMetadata(virDomainPtr dom,
     return ret;
 }

+
+static int
+testDomainGetDomainTotalCpuStats(virTypedParameterPtr params,
+                                int nparams)
+{
+    if (nparams == 0) /* return supported number of params */
+        return 3;
+
+    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
+                                VIR_TYPED_PARAM_ULLONG, 77102913900) < 0)
+        return -1;
+
+    if (nparams > 1 &&
+        virTypedParameterAssign(&params[1],
+                                VIR_DOMAIN_CPU_STATS_USERTIME,
+                                VIR_TYPED_PARAM_ULLONG, 45650000000) < 0)
+        return -1;
+
+    if (nparams > 2 &&
+        virTypedParameterAssign(&params[2],
+                                VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
+                                VIR_TYPED_PARAM_ULLONG, 11390000000) < 0)
+        return -1;
+
+    if (nparams > 3)
+        nparams = 3;
+
+    return nparams;
+}
+
+
+static int
+testDomainGetPercpuStats(virTypedParameterPtr params,
+                         unsigned int nparams,
+                         int start_cpu,
+                         unsigned int ncpus,
+                         int total_cpus)
+{
+    size_t i;
+    int need_cpus;
+    int param_idx;
+    int ret = -1;
+
+    /* return the number of supported params */
+    if (nparams == 0 && ncpus != 0)
+        return 2;
+
+    /* return total number of cpus */
+    if (ncpus == 0) {
+        ret = total_cpus;
+        goto cleanup;
+    }
+
+    if (start_cpu >= total_cpus) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("start_cpu %d larger than maximum of %d"),
+                       start_cpu, total_cpus - 1);
+        goto cleanup;
+    }
+
+    /* return percpu cputime in index 0 */
+    param_idx = 0;
+
+    /* number of cpus to compute */
+    need_cpus = MIN(total_cpus, start_cpu + ncpus);
+
+    for (i = 0; i < need_cpus; i++) {
+        if (i < start_cpu)
+            continue;
+        int idx = (i - start_cpu) * nparams + param_idx;
+        if (virTypedParameterAssign(&params[idx],
+                                    VIR_DOMAIN_CPU_STATS_CPUTIME,
+                                    VIR_TYPED_PARAM_ULLONG,
+                                    202542145062 + 10 * i) < 0)
+            goto cleanup;
+    }
+
+    /* return percpu vcputime in index 1 */
+    param_idx = 1;
+
+    if (param_idx < nparams) {
+        for (i = start_cpu; i < need_cpus; i++) {
+            int idx = (i - start_cpu) * nparams + param_idx;
+            if (virTypedParameterAssign(&params[idx],
+                                        VIR_DOMAIN_CPU_STATS_VCPUTIME,
+                                        VIR_TYPED_PARAM_ULLONG,
+                                        26254023765 + 10 * i) < 0)
+                goto cleanup;
+        }
+        param_idx++;
+    }
+
+    ret = param_idx;
+ cleanup:
+    return ret;
+}
+
+
+static int
+testDomainGetCPUStats(virDomainPtr dom,
+                      virTypedParameterPtr params,
+                      unsigned int nparams,
+                      int start_cpu,
+                      unsigned int ncpus,
+                      unsigned int flags)
+{
+    virDomainObjPtr vm = NULL;
+    testDriverPtr privconn = dom->conn->privateData;
+    int ret = -1;
+
+    virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
+
+    if (!(vm = testDomObjFromDomain(dom)))
+        return -1;
+
+    if (virDomainObjCheckActive(vm) < 0)
+        goto cleanup;
+
+    if (start_cpu == -1)
+        ret = testDomainGetDomainTotalCpuStats(params, nparams);
+    else
+        ret = testDomainGetPercpuStats(params, nparams, start_cpu, ncpus,
+                                       privconn->nodeInfo.cores);
+
+ cleanup:
+    virDomainObjEndAPI(&vm);
+    return ret;
+}
+
+
 static int
 testDomainSendProcessSignal(virDomainPtr dom,
                             long long pid_value,
@@ -7794,6 +7924,7 @@ static virHypervisorDriver testHypervisorDriver = {
     .domainSendKey = testDomainSendKey, /* 5.5.0 */
     .domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */
     .domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */
+    .domainGetCPUStats = testDomainGetCPUStats, /* 5.6.0 */
     .domainSendProcessSignal = testDomainSendProcessSignal, /* 5.5.0 */
     .connectGetCPUModelNames = testConnectGetCPUModelNames, /* 1.1.3 */
     .domainManagedSave = testDomainManagedSave, /* 1.1.4 */
--
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainGetCPUStats
Posted by Erik Skultety 4 years, 8 months ago
On Thu, Jul 18, 2019 at 12:02:43PM +0200, Ilias Stamatis wrote:
> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> ---
>  src/test/test_driver.c | 131 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 131 insertions(+)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index fcb80c9e47..2907c043cb 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -3258,6 +3258,136 @@ static int testDomainSetMetadata(virDomainPtr dom,
>      return ret;
>  }
>
> +
> +static int
> +testDomainGetDomainTotalCpuStats(virTypedParameterPtr params,
> +                                int nparams)
> +{
> +    if (nparams == 0) /* return supported number of params */
> +        return 3;
> +
> +    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
> +                                VIR_TYPED_PARAM_ULLONG, 77102913900) < 0)
> +        return -1;
> +
> +    if (nparams > 1 &&
> +        virTypedParameterAssign(&params[1],
> +                                VIR_DOMAIN_CPU_STATS_USERTIME,
> +                                VIR_TYPED_PARAM_ULLONG, 45650000000) < 0)
> +        return -1;
> +
> +    if (nparams > 2 &&
> +        virTypedParameterAssign(&params[2],
> +                                VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
> +                                VIR_TYPED_PARAM_ULLONG, 11390000000) < 0)
> +        return -1;
> +
> +    if (nparams > 3)
> +        nparams = 3;
> +
> +    return nparams;
> +}
> +
> +
> +static int
> +testDomainGetPercpuStats(virTypedParameterPtr params,
> +                         unsigned int nparams,
> +                         int start_cpu,
> +                         unsigned int ncpus,
> +                         int total_cpus)
> +{
> +    size_t i;
> +    int need_cpus;
> +    int param_idx;
> +    int ret = -1;
> +
> +    /* return the number of supported params */
> +    if (nparams == 0 && ncpus != 0)
> +        return 2;
> +
> +    /* return total number of cpus */
> +    if (ncpus == 0) {
> +        ret = total_cpus;
> +        goto cleanup;
> +    }
> +
> +    if (start_cpu >= total_cpus) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("start_cpu %d larger than maximum of %d"),
> +                       start_cpu, total_cpus - 1);
> +        goto cleanup;
> +    }
> +
> +    /* return percpu cputime in index 0 */
> +    param_idx = 0;
> +
> +    /* number of cpus to compute */
> +    need_cpus = MIN(total_cpus, start_cpu + ncpus);
> +
> +    for (i = 0; i < need_cpus; i++) {
> +        if (i < start_cpu)
> +            continue;
> +        int idx = (i - start_cpu) * nparams + param_idx;
> +        if (virTypedParameterAssign(&params[idx],
> +                                    VIR_DOMAIN_CPU_STATS_CPUTIME,
> +                                    VIR_TYPED_PARAM_ULLONG,
> +                                    202542145062 + 10 * i) < 0)

What's the reasoning behind the formula? I'm curious, wouldn't have
202542145062 + i been enough? Anyhow, the CPUTIME should be a portion of the
total cputime of all CPUs, your per-CPU time is much bigger by the total,
that doesn't sound right. I don't care about the exact numbers, but I'd like to
see them to make sense.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainGetCPUStats
Posted by Ilias Stamatis 4 years, 8 months ago
On Thu, Jul 25, 2019 at 2:01 PM Erik Skultety <eskultet@redhat.com> wrote:
>
> On Thu, Jul 18, 2019 at 12:02:43PM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > ---
> >  src/test/test_driver.c | 131 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 131 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index fcb80c9e47..2907c043cb 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -3258,6 +3258,136 @@ static int testDomainSetMetadata(virDomainPtr dom,
> >      return ret;
> >  }
> >
> > +
> > +static int
> > +testDomainGetDomainTotalCpuStats(virTypedParameterPtr params,
> > +                                int nparams)
> > +{
> > +    if (nparams == 0) /* return supported number of params */
> > +        return 3;
> > +
> > +    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
> > +                                VIR_TYPED_PARAM_ULLONG, 77102913900) < 0)
> > +        return -1;
> > +
> > +    if (nparams > 1 &&
> > +        virTypedParameterAssign(&params[1],
> > +                                VIR_DOMAIN_CPU_STATS_USERTIME,
> > +                                VIR_TYPED_PARAM_ULLONG, 45650000000) < 0)
> > +        return -1;
> > +
> > +    if (nparams > 2 &&
> > +        virTypedParameterAssign(&params[2],
> > +                                VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
> > +                                VIR_TYPED_PARAM_ULLONG, 11390000000) < 0)
> > +        return -1;
> > +
> > +    if (nparams > 3)
> > +        nparams = 3;
> > +
> > +    return nparams;
> > +}
> > +
> > +
> > +static int
> > +testDomainGetPercpuStats(virTypedParameterPtr params,
> > +                         unsigned int nparams,
> > +                         int start_cpu,
> > +                         unsigned int ncpus,
> > +                         int total_cpus)
> > +{
> > +    size_t i;
> > +    int need_cpus;
> > +    int param_idx;
> > +    int ret = -1;
> > +
> > +    /* return the number of supported params */
> > +    if (nparams == 0 && ncpus != 0)
> > +        return 2;
> > +
> > +    /* return total number of cpus */
> > +    if (ncpus == 0) {
> > +        ret = total_cpus;
> > +        goto cleanup;
> > +    }
> > +
> > +    if (start_cpu >= total_cpus) {
> > +        virReportError(VIR_ERR_INVALID_ARG,
> > +                       _("start_cpu %d larger than maximum of %d"),
> > +                       start_cpu, total_cpus - 1);
> > +        goto cleanup;
> > +    }
> > +
> > +    /* return percpu cputime in index 0 */
> > +    param_idx = 0;
> > +
> > +    /* number of cpus to compute */
> > +    need_cpus = MIN(total_cpus, start_cpu + ncpus);
> > +
> > +    for (i = 0; i < need_cpus; i++) {
> > +        if (i < start_cpu)
> > +            continue;
> > +        int idx = (i - start_cpu) * nparams + param_idx;
> > +        if (virTypedParameterAssign(&params[idx],
> > +                                    VIR_DOMAIN_CPU_STATS_CPUTIME,
> > +                                    VIR_TYPED_PARAM_ULLONG,
> > +                                    202542145062 + 10 * i) < 0)
>
> What's the reasoning behind the formula? I'm curious, wouldn't have
> 202542145062 + i been enough? Anyhow, the CPUTIME should be a portion of the
> total cputime of all CPUs, your per-CPU time is much bigger by the total,
> that doesn't sound right. I don't care about the exact numbers, but I'd like to
> see them to make sense.
>
> Erik

The 10 * i was totally random. You are right, let's make them make sense.

We can have a total CPUTIME and then divide that equally (?) by the
number of CPUs.

The "problem" is that the number of CPUs can vary. So depending on the
number of CPUs the test VM has been configured with, this API will
return different numbers. Is that ok?

Because I thought we would prefer to return the same fixed numbers
every time. However then they won't make sense when the number of CPUs
changes.


Ilias

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainGetCPUStats
Posted by Erik Skultety 4 years, 8 months ago
On Fri, Jul 26, 2019 at 01:24:39PM +0200, Ilias Stamatis wrote:
> On Thu, Jul 25, 2019 at 2:01 PM Erik Skultety <eskultet@redhat.com> wrote:
> >
> > On Thu, Jul 18, 2019 at 12:02:43PM +0200, Ilias Stamatis wrote:
> > > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > > ---
> > >  src/test/test_driver.c | 131 +++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 131 insertions(+)
> > >
> > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > > index fcb80c9e47..2907c043cb 100644
> > > --- a/src/test/test_driver.c
> > > +++ b/src/test/test_driver.c
> > > @@ -3258,6 +3258,136 @@ static int testDomainSetMetadata(virDomainPtr dom,
> > >      return ret;
> > >  }
> > >
> > > +
> > > +static int
> > > +testDomainGetDomainTotalCpuStats(virTypedParameterPtr params,
> > > +                                int nparams)
> > > +{
> > > +    if (nparams == 0) /* return supported number of params */
> > > +        return 3;
> > > +
> > > +    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
> > > +                                VIR_TYPED_PARAM_ULLONG, 77102913900) < 0)
> > > +        return -1;
> > > +
> > > +    if (nparams > 1 &&
> > > +        virTypedParameterAssign(&params[1],
> > > +                                VIR_DOMAIN_CPU_STATS_USERTIME,
> > > +                                VIR_TYPED_PARAM_ULLONG, 45650000000) < 0)
> > > +        return -1;
> > > +
> > > +    if (nparams > 2 &&
> > > +        virTypedParameterAssign(&params[2],
> > > +                                VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
> > > +                                VIR_TYPED_PARAM_ULLONG, 11390000000) < 0)
> > > +        return -1;
> > > +
> > > +    if (nparams > 3)
> > > +        nparams = 3;
> > > +
> > > +    return nparams;
> > > +}
> > > +
> > > +
> > > +static int
> > > +testDomainGetPercpuStats(virTypedParameterPtr params,
> > > +                         unsigned int nparams,
> > > +                         int start_cpu,
> > > +                         unsigned int ncpus,
> > > +                         int total_cpus)
> > > +{
> > > +    size_t i;
> > > +    int need_cpus;
> > > +    int param_idx;
> > > +    int ret = -1;
> > > +
> > > +    /* return the number of supported params */
> > > +    if (nparams == 0 && ncpus != 0)
> > > +        return 2;
> > > +
> > > +    /* return total number of cpus */
> > > +    if (ncpus == 0) {
> > > +        ret = total_cpus;
> > > +        goto cleanup;
> > > +    }
> > > +
> > > +    if (start_cpu >= total_cpus) {
> > > +        virReportError(VIR_ERR_INVALID_ARG,
> > > +                       _("start_cpu %d larger than maximum of %d"),
> > > +                       start_cpu, total_cpus - 1);
> > > +        goto cleanup;
> > > +    }
> > > +
> > > +    /* return percpu cputime in index 0 */
> > > +    param_idx = 0;
> > > +
> > > +    /* number of cpus to compute */
> > > +    need_cpus = MIN(total_cpus, start_cpu + ncpus);
> > > +
> > > +    for (i = 0; i < need_cpus; i++) {
> > > +        if (i < start_cpu)
> > > +            continue;
> > > +        int idx = (i - start_cpu) * nparams + param_idx;
> > > +        if (virTypedParameterAssign(&params[idx],
> > > +                                    VIR_DOMAIN_CPU_STATS_CPUTIME,
> > > +                                    VIR_TYPED_PARAM_ULLONG,
> > > +                                    202542145062 + 10 * i) < 0)
> >
> > What's the reasoning behind the formula? I'm curious, wouldn't have
> > 202542145062 + i been enough? Anyhow, the CPUTIME should be a portion of the
> > total cputime of all CPUs, your per-CPU time is much bigger by the total,
> > that doesn't sound right. I don't care about the exact numbers, but I'd like to
> > see them to make sense.
> >
> > Erik
>
> The 10 * i was totally random. You are right, let's make them make sense.
>
> We can have a total CPUTIME and then divide that equally (?) by the
> number of CPUs.

I didn't want to do division really (in an ideal world we'd be okay with bit
shifts, but that's not the case unfortunately), but it's test driver, so
doesn't matter at all.

>
> The "problem" is that the number of CPUs can vary. So depending on the
> number of CPUs the test VM has been configured with, this API will
> return different numbers. Is that ok?

Yes it is, that is expected, it would be weird if a configuration with 4 vCPUs
returns data for 2. CPUTIME needs to be static though.

Erik

>
> Because I thought we would prefer to return the same fixed numbers
> every time. However then they won't make sense when the number of CPUs
> changes.
>
>
> Ilias
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainGetCPUStats
Posted by Daniel P. Berrangé 4 years, 8 months ago
On Thu, Jul 25, 2019 at 02:00:56PM +0200, Erik Skultety wrote:
> On Thu, Jul 18, 2019 at 12:02:43PM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > ---
> >  src/test/test_driver.c | 131 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 131 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index fcb80c9e47..2907c043cb 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -3258,6 +3258,136 @@ static int testDomainSetMetadata(virDomainPtr dom,
> >      return ret;
> >  }
> >
> > +
> > +static int
> > +testDomainGetDomainTotalCpuStats(virTypedParameterPtr params,
> > +                                int nparams)
> > +{
> > +    if (nparams == 0) /* return supported number of params */
> > +        return 3;
> > +
> > +    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
> > +                                VIR_TYPED_PARAM_ULLONG, 77102913900) < 0)
> > +        return -1;
> > +
> > +    if (nparams > 1 &&
> > +        virTypedParameterAssign(&params[1],
> > +                                VIR_DOMAIN_CPU_STATS_USERTIME,
> > +                                VIR_TYPED_PARAM_ULLONG, 45650000000) < 0)
> > +        return -1;
> > +
> > +    if (nparams > 2 &&
> > +        virTypedParameterAssign(&params[2],
> > +                                VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
> > +                                VIR_TYPED_PARAM_ULLONG, 11390000000) < 0)
> > +        return -1;
> > +
> > +    if (nparams > 3)
> > +        nparams = 3;
> > +
> > +    return nparams;
> > +}
> > +
> > +
> > +static int
> > +testDomainGetPercpuStats(virTypedParameterPtr params,
> > +                         unsigned int nparams,
> > +                         int start_cpu,
> > +                         unsigned int ncpus,
> > +                         int total_cpus)
> > +{
> > +    size_t i;
> > +    int need_cpus;
> > +    int param_idx;
> > +    int ret = -1;
> > +
> > +    /* return the number of supported params */
> > +    if (nparams == 0 && ncpus != 0)
> > +        return 2;
> > +
> > +    /* return total number of cpus */
> > +    if (ncpus == 0) {
> > +        ret = total_cpus;
> > +        goto cleanup;
> > +    }
> > +
> > +    if (start_cpu >= total_cpus) {
> > +        virReportError(VIR_ERR_INVALID_ARG,
> > +                       _("start_cpu %d larger than maximum of %d"),
> > +                       start_cpu, total_cpus - 1);
> > +        goto cleanup;
> > +    }
> > +
> > +    /* return percpu cputime in index 0 */
> > +    param_idx = 0;
> > +
> > +    /* number of cpus to compute */
> > +    need_cpus = MIN(total_cpus, start_cpu + ncpus);
> > +
> > +    for (i = 0; i < need_cpus; i++) {
> > +        if (i < start_cpu)
> > +            continue;
> > +        int idx = (i - start_cpu) * nparams + param_idx;
> > +        if (virTypedParameterAssign(&params[idx],
> > +                                    VIR_DOMAIN_CPU_STATS_CPUTIME,
> > +                                    VIR_TYPED_PARAM_ULLONG,
> > +                                    202542145062 + 10 * i) < 0)
> 
> What's the reasoning behind the formula? I'm curious, wouldn't have
> 202542145062 + i been enough? Anyhow, the CPUTIME should be a portion of the
> total cputime of all CPUs, your per-CPU time is much bigger by the total,
> that doesn't sound right. I don't care about the exact numbers, but I'd like to
> see them to make sense.

Note that this calculation  is going to be overflowing 32-bit ints,
so the 202542145062 constant needs a LL suffix to force 64-bit
arthimetic too, so we don't break on 32-bit hosts.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list