On Mon, Jul 29, 2019 at 3:28 PM Erik Skultety <eskultet@xxxxxxxxxx> wrote:
>
> On Sun, Jul 28, 2019 at 12:02:21PM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis <stamatis.iliass@xxxxxxxxx>
> > ---
> > src/test/test_driver.c | 132 +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 132 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index ab0f8b06d6..56f08fc3d2 100755
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -3629,6 +3629,137 @@ static int testDomainSetMetadata(virDomainPtr dom,
> > return ret;
> > }
> >
> > +#define TEST_TOTAL_CPUTIME 48772617035
>
> Let's be explicit with ullong ^here by adding LL
Oops. I had seen Daniel's comment but forgot to apply.
>
> > +
> > +static int
> > +testDomainGetDomainTotalCpuStats(virTypedParameterPtr params,
> > + int nparams)
>
> indent is off
>
> > +{
> > + if (nparams == 0) /* return supported number of params */
> > + return 3;
> > +
> > + if (virTypedParameterAssign(¶ms[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
> > + VIR_TYPED_PARAM_ULLONG, TEST_TOTAL_CPUTIME) < 0)
> > + return -1;
> > +
> > + if (nparams > 1 &&
> > + virTypedParameterAssign(¶ms[1],
> > + VIR_DOMAIN_CPU_STATS_USERTIME,
> > + VIR_TYPED_PARAM_ULLONG, 5540000000) < 0)
> > + return -1;
> > +
> > + if (nparams > 2 &&
> > + virTypedParameterAssign(¶ms[2],
> > + VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
> > + VIR_TYPED_PARAM_ULLONG, 6460000000) < 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;
>
> @ret is unnecessary, see below
>
> > +
> > + /* 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;
>
> return total_cpus;
>
> > + }
> > +
> > + 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 -1;
>
> > + }
> > +
> > + /* 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;
>
> How about initializing i = start_cpu straight away instead?
>
> > + int idx = (i - start_cpu) * nparams + param_idx;
> > + if (virTypedParameterAssign(¶ms[idx],
> > + VIR_DOMAIN_CPU_STATS_CPUTIME,
> > + VIR_TYPED_PARAM_ULLONG,
> > + (TEST_TOTAL_CPUTIME / total_cpus) + i) < 0)
>
> I'd strongly prefer if we didn't perform the division in each iteration,
I think the compiler will be smart enough to optimize this? But ok
sure, let's not make assumptions.
> the "+ i" also seems unnecessary.
I just added it in order for different CPUs to return different values.
+1, +2 etc. are trivial quantities so the results still make sense imo
>
> > + goto cleanup;
>
> return -1;
>
> > + }
> > +
> > + /* 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(¶ms[idx],
> > + VIR_DOMAIN_CPU_STATS_VCPUTIME,
> > + VIR_TYPED_PARAM_ULLONG,
> > + (TEST_TOTAL_CPUTIME / total_cpus) - 1234567890 + i) < 0)
>
> Same as above...
>
> > + goto cleanup;
>
> return -1;
>
> > + }
> > + param_idx++;
> > + }
> > +
> > + ret = param_idx;
>
> return param_idx;
>
> > + cleanup:
> > + return ret;
>
> Drop the cleanup label.
Yeah, totally. That was a leftover from previous code and I didn't
realize it after adjusting.
>
> > +}
> > +
> > +
> > +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,
> > @@ -8552,6 +8683,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 */
> > --
>
> I'll adjust the code before merging.
Sure, thanks for the review!
Ilias
> Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
