[libvirt] [PATCH v2 0/2] test_driver: implement virDomainGetCPUStats
Changes since v1: * Removed failing test cases * Total CPUTIME now is the sum of the individual CPUs Ilias Stamatis (2): tests: virsh-optparse: remove no longer valid cpu-stats test cases test_driver: implement virDomainGetCPUStats src/test/test_driver.c | 132 +++++++++++++++++++++++++++++++++++++++++ tests/virsh-optparse | 20 ------- 2 files changed, 132 insertions(+), 20 deletions(-) -- 2.22.0
These test cases are no longer valid since this series provides an implementation of the virDomainGetCPUStats API for the test driver. Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- tests/virsh-optparse | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 090d6c205c..d9c8f3c731 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -179,26 +179,6 @@ virsh -q -c $test_url cpu-stats test --start -1 >out 2>err && fail=1 test -s out && fail=1 compare exp-err err || fail=1 -# Zero. The test driver doesn't support the operation so the command -# fails, but the value has been parsed correctly -cat <<\EOF > exp-err || framework_failure -error: Failed to retrieve CPU statistics for domain 'test' -error: this function is not supported by the connection driver: virDomainGetCPUStats -EOF -virsh -q -c $test_url cpu-stats test --start 0 >out 2>err && fail=1 -test -s out && fail=1 -compare exp-err err || fail=1 - -# Valid numeric value. The test driver doesn't support the operation -# so the command fails, but the value has been parsed correctly -cat <<\EOF > exp-err || framework_failure -error: Failed to retrieve CPU statistics for domain 'test' -error: this function is not supported by the connection driver: virDomainGetCPUStats -EOF -virsh -q -c $test_url cpu-stats test --start 42 >out 2>err && fail=1 -test -s out && fail=1 -compare exp-err err || fail=1 - ### Test a scaled numeric option # Non-numeric value -- 2.22.0
On Sun, Jul 28, 2019 at 12:02:20PM +0200, Ilias Stamatis wrote:
These test cases are no longer valid since this series provides an implementation of the virDomainGetCPUStats API for the test driver.
"this series" is relevant only in context of the patches sent to the ML. Once it's in the git history, it's essentially irrelevant information. I'll change it before pushing. Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- 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 + +static int +testDomainGetDomainTotalCpuStats(virTypedParameterPtr params, + int nparams) +{ + 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; + + /* 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(¶ms[idx], + VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, + (TEST_TOTAL_CPUTIME / total_cpus) + 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(¶ms[idx], + VIR_DOMAIN_CPU_STATS_VCPUTIME, + VIR_TYPED_PARAM_ULLONG, + (TEST_TOTAL_CPUTIME / total_cpus) - 1234567890 + 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, @@ -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 */ -- 2.22.0
On Sun, Jul 28, 2019 at 12:02:21PM +0200, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- 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
+ +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, the "+ i" also seems unnecessary.
+ 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.
+} + + +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. Reviewed-by: Erik Skultety <eskultet@redhat.com>
On Mon, Jul 29, 2019 at 3:28 PM Erik Skultety <eskultet@redhat.com> wrote:
On Sun, Jul 28, 2019 at 12:02:21PM +0200, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- 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@redhat.com>
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
Well, I didn't see a use case in that, but someone may sort the returned stats by the most utilized CPU, so it kinda makes sense, I'll leave it in then. Erik
participants (2)
-
Erik Skultety -
Ilias Stamatis