Add a test to fetch the GetMemoryStat output. This only gets
data for v1 only right now since the v2 data from commit 61ff6021
is rather useless returning all 0's.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
tests/vircgrouptest.c | 61 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
index 310e1fb6a2..06c4a8ef5c 100644
--- a/tests/vircgrouptest.c
+++ b/tests/vircgrouptest.c
@@ -802,6 +802,64 @@ static int testCgroupGetMemoryUsage(const void *args ATTRIBUTE_UNUSED)
return ret;
}
+
+static int
+testCgroupGetMemoryStat(const void *args ATTRIBUTE_UNUSED)
+{
+ virCgroupPtr cgroup = NULL;
+ int rv, ret = -1;
+ size_t i;
+
+ const unsigned long long expected_values[] = {
+ 1336619008ULL,
+ 67100672ULL,
+ 145887232ULL,
+ 661872640ULL,
+ 627400704UL,
+ 3690496ULL
+ };
+ const char* names[] = {
+ "cache",
+ "active_anon",
+ "inactive_anon",
+ "active_file",
+ "inactive_file",
+ "unevictable"
+ };
+ unsigned long long values[ARRAY_CARDINALITY(expected_values)];
+
+ if ((rv = virCgroupNewPartition("/virtualmachines", true,
+ (1 << VIR_CGROUP_CONTROLLER_MEMORY),
+ &cgroup)) < 0) {
+ fprintf(stderr, "Could not create /virtualmachines cgroup: %d\n", -rv);
+ goto cleanup;
+ }
+
+ if ((rv = virCgroupGetMemoryStat(cgroup, &values[0],
+ &values[1], &values[2],
+ &values[3], &values[4],
+ &values[5])) < 0) {
+ fprintf(stderr, "Could not retrieve GetMemoryStat for /virtualmachines cgroup: %d\n", -rv);
+ goto cleanup;
+ }
+
+ for (i = 0; i < ARRAY_CARDINALITY(expected_values); i++) {
+ if (expected_values[i] != (values[i] << 10)) {
+ fprintf(stderr,
+ "Wrong value (%llu) for %s from virCgroupGetMemoryStat (expected %llu)\n",
+ values[i], names[i], expected_values[i]);
+ goto cleanup;
+ }
+ }
+
+ ret = 0;
+
+ cleanup:
+ virCgroupFree(&cgroup);
+ return ret;
+}
+
+
static int testCgroupGetBlkioIoServiced(const void *args ATTRIBUTE_UNUSED)
{
virCgroupPtr cgroup = NULL;
@@ -1035,6 +1093,9 @@ mymain(void)
if (virTestRun("virCgroupGetMemoryUsage works", testCgroupGetMemoryUsage, NULL) < 0)
ret = -1;
+ if (virTestRun("virCgroupGetMemoryStat works", testCgroupGetMemoryStat, NULL) < 0)
+ ret = -1;
+
if (virTestRun("virCgroupGetPercpuStats works", testCgroupGetPercpuStats, NULL) < 0)
ret = -1;
cleanupFakeFS(fakerootdir);
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 07, 2018 at 06:57:40PM -0500, John Ferlan wrote:
> Add a test to fetch the GetMemoryStat output. This only gets
> data for v1 only right now since the v2 data from commit 61ff6021
> is rather useless returning all 0's.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> tests/vircgrouptest.c | 61 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
> index 310e1fb6a2..06c4a8ef5c 100644
> --- a/tests/vircgrouptest.c
> +++ b/tests/vircgrouptest.c
> @@ -802,6 +802,64 @@ static int testCgroupGetMemoryUsage(const void *args ATTRIBUTE_UNUSED)
> return ret;
> }
>
> +
> +static int
> +testCgroupGetMemoryStat(const void *args ATTRIBUTE_UNUSED)
> +{
> + virCgroupPtr cgroup = NULL;
> + int rv, ret = -1;
Please each variable on separate line. Once you need to change/remove
any of the variable the diff is way better.
> + size_t i;
> +
> + const unsigned long long expected_values[] = {
> + 1336619008ULL,
> + 67100672ULL,
> + 145887232ULL,
> + 661872640ULL,
> + 627400704UL,
> + 3690496ULL
> + };
> + const char* names[] = {
> + "cache",
> + "active_anon",
> + "inactive_anon",
> + "active_file",
> + "inactive_file",
> + "unevictable"
> + };
> + unsigned long long values[ARRAY_CARDINALITY(expected_values)];
> +
> + if ((rv = virCgroupNewPartition("/virtualmachines", true,
> + (1 << VIR_CGROUP_CONTROLLER_MEMORY),
> + &cgroup)) < 0) {
> + fprintf(stderr, "Could not create /virtualmachines cgroup: %d\n", -rv);
> + goto cleanup;
> + }
> +
> + if ((rv = virCgroupGetMemoryStat(cgroup, &values[0],
> + &values[1], &values[2],
> + &values[3], &values[4],
> + &values[5])) < 0) {
> + fprintf(stderr, "Could not retrieve GetMemoryStat for /virtualmachines cgroup: %d\n", -rv);
> + goto cleanup;
> + }
> +
> + for (i = 0; i < ARRAY_CARDINALITY(expected_values); i++) {
> + if (expected_values[i] != (values[i] << 10)) {
This feels wrong and it's just a lucky coincidence that it works with
these values. It's basically the same operation as 'x * 1024'.
I would rather change it into this:
if ((expected_values[i] >> 10) != values[i]) {
because we know that we do the same operation after reading these values
from memory.stat file.
> + fprintf(stderr,
> + "Wrong value (%llu) for %s from virCgroupGetMemoryStat (expected %llu)\n",
> + values[i], names[i], expected_values[i]);
This would print wrong values, we need to print shifted values.
Probably the best solution would be to have "expected_values" with the
correct number from the start.
Note: please keep the lines under 80 characters.
Because it's a test I'm OK with both solutions, modifying
"expected_values" in place or to have them correct from the start and
I'll leave it up to you. There is no need to resend it.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/13/18 8:01 AM, Pavel Hrdina wrote:
> On Wed, Nov 07, 2018 at 06:57:40PM -0500, John Ferlan wrote:
>> Add a test to fetch the GetMemoryStat output. This only gets
>> data for v1 only right now since the v2 data from commit 61ff6021
>> is rather useless returning all 0's.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> tests/vircgrouptest.c | 61 +++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 61 insertions(+)
>>
>> diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
>> index 310e1fb6a2..06c4a8ef5c 100644
>> --- a/tests/vircgrouptest.c
>> +++ b/tests/vircgrouptest.c
>> @@ -802,6 +802,64 @@ static int testCgroupGetMemoryUsage(const void *args ATTRIBUTE_UNUSED)
>> return ret;
>> }
>>
>> +
>> +static int
>> +testCgroupGetMemoryStat(const void *args ATTRIBUTE_UNUSED)
>> +{
>> + virCgroupPtr cgroup = NULL;
>> + int rv, ret = -1;
>
> Please each variable on separate line. Once you need to change/remove
> any of the variable the diff is way better.
>
Right - just some copy-pasta here.
>> + size_t i;
>> +
>> + const unsigned long long expected_values[] = {
>> + 1336619008ULL,
>> + 67100672ULL,
>> + 145887232ULL,
>> + 661872640ULL,
>> + 627400704UL,
>> + 3690496ULL
>> + };
>> + const char* names[] = {
>> + "cache",
>> + "active_anon",
>> + "inactive_anon",
>> + "active_file",
>> + "inactive_file",
>> + "unevictable"
>> + };
>> + unsigned long long values[ARRAY_CARDINALITY(expected_values)];
>> +
>> + if ((rv = virCgroupNewPartition("/virtualmachines", true,
>> + (1 << VIR_CGROUP_CONTROLLER_MEMORY),
>> + &cgroup)) < 0) {
>> + fprintf(stderr, "Could not create /virtualmachines cgroup: %d\n", -rv);
>> + goto cleanup;
>> + }
>> +
>> + if ((rv = virCgroupGetMemoryStat(cgroup, &values[0],
>> + &values[1], &values[2],
>> + &values[3], &values[4],
>> + &values[5])) < 0) {
>> + fprintf(stderr, "Could not retrieve GetMemoryStat for /virtualmachines cgroup: %d\n", -rv);
>> + goto cleanup;
>> + }
>> +
>> + for (i = 0; i < ARRAY_CARDINALITY(expected_values); i++) {
>> + if (expected_values[i] != (values[i] << 10)) {
>
> This feels wrong and it's just a lucky coincidence that it works with
> these values. It's basically the same operation as 'x * 1024'.
>
> I would rather change it into this:
>
> if ((expected_values[i] >> 10) != values[i]) {
>
> because we know that we do the same operation after reading these values
> from memory.stat file.
>
That's fine - either/or. I forgot to note the values were "sourced
from" the original commit d14524701 MAKE_FILE mocking logic and the
fetch/store logic in virCgroupGetMemoryStat which does the >> 10.
>> + fprintf(stderr,
>> + "Wrong value (%llu) for %s from virCgroupGetMemoryStat (expected %llu)\n",
>> + values[i], names[i], expected_values[i]);
>
> This would print wrong values, we need to print shifted values.
> Probably the best solution would be to have "expected_values" with the
> correct number from the start
Oh yeah - forgot to do that after I realized the >> was necessary... Off
by a magnitude of 1024 is always easy to figure out though. Still the
"correct number" could be a matter of opinion, too. Do you view the
expected number as seen in the array without the shift or with it? e.g.
for 'cache' is 1336619008 (expected w/o shift) or 1305292 (value w/
shift) the correct value?
>
> Note: please keep the lines under 80 characters.
Hey, that's my line ;-)
>
> Because it's a test I'm OK with both solutions, modifying
> "expected_values" in place or to have them correct from the start and
> I'll leave it up to you. There is no need to resend it.
>
> Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
>
Thanks I went with displaying the shifted value:
fprintf(stderr,
"Wrong value (%llu) for %s from virCgroupGetMemoryStat "
"(expected %llu)\n",
values[i], names[i], (expected_values[i] >> 10));
But I won't push right away just in case someone prefers the unshifted
from the expected array.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.