5:57 p.m.
Originally reported here:
https://www.redhat.com/archives/libvir-list/2018-November/msg00252.html
John Ferlan (1):
tests: Augment vcgrouptest to add virCgroupGetMemoryStat
Peter Chubb (1):
util: Fix virCgroupGetMemoryStat
src/util/vircgroupv1.c | 7 ++++-
src/util/vircgroupv2.c | 7 ++++-
tests/vircgrouptest.c | 61 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 73 insertions(+), 2 deletions(-)
--
2.17.2
5:57 p.m.
New subject: [libvirt] [PATCH 1/2] util: Fix virCgroupGetMemoryStat
From: Peter Chubb <Peter.Chubb(a)data61.csiro.au>
Commit 901d2b9c introduced virCgroupGetMemoryStat and replaced
the LXC virLXCCgroupGetMemStat logic in commit e634c7cd0. However,
in doing so the replacement wasn't exact as the LXC logic used
getline() to process the cgroup controller data, while the new
virCgroupGetMemoryStat used "memory.stat" manual buffer read/
processing which neglected to forward through @line in order
to read each line in the output.
To fix that, we should be sure to carry forward the @line value
for each line read updating it beyond that current @newLine value
once we've calculated the values that we want.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/vircgroupv1.c | 7 ++++++-
src/util/vircgroupv2.c | 7 ++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 28a74474ee..678ffda35c 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1476,7 +1476,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
line = stat;
- while (line) {
+ while (line && *line) {
char *newLine = strchr(line, '\n');
char *valueStr = strchr(line, ' ');
unsigned long long value;
@@ -1506,6 +1506,11 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
inactiveFileVal = value >> 10;
else if (STREQ(line, "unevictable"))
unevictableVal = value >> 10;
+
+ if (newLine)
+ line = newLine + 1;
+ else
+ break;
}
*cache = cacheVal;
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 32adb06784..541e8e790e 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -1068,7 +1068,7 @@ virCgroupV2GetMemoryStat(virCgroupPtr group,
line = stat;
- while (line) {
+ while (line && *line) {
char *newLine = strchr(line, '\n');
char *valueStr = strchr(line, ' ');
unsigned long long value;
@@ -1102,6 +1102,11 @@ virCgroupV2GetMemoryStat(virCgroupPtr group,
inactiveFileVal = value >> 10;
else if (STREQ(line, "unevictable"))
unevictableVal = value >> 10;
+
+ if (newLine)
+ line = newLine + 1;
+ else
+ break;
}
*cache = cacheVal;
--
2.17.2
7:07 p.m.
New subject: [libvirt] [PATCH 1/2] util: Fix virCgroupGetMemoryStat
>>>> "John" == John Ferlan
<jferlan(a)redhat.com> writes:
John> Signed-off-by: John Ferlan <jferlan(a)redhat.com> ---
Signed-off-by: Peter Chubb <peter.chubb(a)data61.csiro.au>
--
Dr Peter Chubb Tel: +61 2 9490 5852 http://ts.data61.csiro.au/
Trustworthy Systems Group Data61, CSIRO (formerly NICTA)
6:43 a.m.
New subject: [libvirt] [PATCH 1/2] util: Fix virCgroupGetMemoryStat
On Wed, Nov 07, 2018 at 06:57:39PM -0500, John Ferlan wrote:
From: Peter Chubb <Peter.Chubb(a)data61.csiro.au>
Commit 901d2b9c introduced virCgroupGetMemoryStat and replaced
the LXC virLXCCgroupGetMemStat logic in commit e634c7cd0. However,
in doing so the replacement wasn't exact as the LXC logic used
getline() to process the cgroup controller data, while the new
virCgroupGetMemoryStat used "memory.stat" manual buffer read/
processing which neglected to forward through @line in order
to read each line in the output.
To fix that, we should be sure to carry forward the @line value
for each line read updating it beyond that current @newLine value
once we've calculated the values that we want.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/vircgroupv1.c | 7 ++++++-
src/util/vircgroupv2.c | 7 ++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 28a74474ee..678ffda35c 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1476,7 +1476,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
line = stat;
- while (line) {
+ while (line && *line) {
There is no need to check line, it cannot be NULL.
char *newLine = strchr(line, '\n');
char *valueStr = strchr(line, ' ');
unsigned long long value;
@@ -1506,6 +1506,11 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
inactiveFileVal = value >> 10;
else if (STREQ(line, "unevictable"))
unevictableVal = value >> 10;
+
+ if (newLine)
+ line = newLine + 1;
+ else
+ break;
}
*cache = cacheVal;
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 32adb06784..541e8e790e 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -1068,7 +1068,7 @@ virCgroupV2GetMemoryStat(virCgroupPtr group,
line = stat;
- while (line) {
+ while (line && *line) {
Same here.
char *newLine = strchr(line, '\n');
char *valueStr = strchr(line, ' ');
unsigned long long value;
@@ -1102,6 +1102,11 @@ virCgroupV2GetMemoryStat(virCgroupPtr group,
inactiveFileVal = value >> 10;
else if (STREQ(line, "unevictable"))
unevictableVal = value >> 10;
+
+ if (newLine)
+ line = newLine + 1;
+ else
+ break;
}
*cache = cacheVal;
--
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
5:57 p.m.
New subject: [libvirt] [PATCH 2/2] tests: Augment vcgrouptest to add virCgroupGetMemoryStat
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(a)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
7:01 a.m.
New subject: [libvirt] [PATCH 2/2] tests: Augment vcgrouptest to add virCgroupGetMemoryStat
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(a)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(a)redhat.com>
1:28 p.m.
New subject: [libvirt] [PATCH 2/2] tests: Augment vcgrouptest to add virCgroupGetMemoryStat
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(a)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(a)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
2201
days inactive
2207
days old
6 comments
3 participants
participants (3)
-
John Ferlan -
Pavel Hrdina -
Peter.Chubb＠data61.csiro.au