8:15 a.m.
The patch set fixes two problems of lxc that happen when ns
subsystem is enabled with memory subsystem at the same time.
The fist problem is that cgroup subdirectories that are
automatically created by ns subsystem remain after domain
shutdown. The second problem is that memory usage is not
properly accounted.
Ryota Ozaki (2):
cgroup: Change virCgroupRemove to remove all child groups at first
cgroup: Enable memory.use_hierarchy of cgroup for domain
8:15 a.m.
New subject: [libvirt] [PATCH 1/2] cgroup: Change virCgroupRemove to remove all child groups at first
As same as normal directories, a cgroup cannot be removed if it
contains sub groups. This patch changes virCgroupRemove to remove
all child groups (subdirectories) of a target group before removing
the target group.
The handling is required when we run lxc with ns subsystem of cgroup.
Ns subsystem automatically creates child cgroups on every process
forks, but unfortunately the groups are not removed on process exits,
so we have to remove them by ourselves.
With this patch, such child groups are surely removed at lxc
shutdown, i.e., lxcVmCleanup which calls virCgroupRemove.
---
src/util/cgroup.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index b8b2eb5..531c131 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -23,6 +23,7 @@
#include <sys/stat.h>
#include <sys/types.h>
#include <libgen.h>
+#include <dirent.h>
#include "internal.h"
#include "util.h"
@@ -561,11 +562,52 @@ cleanup:
}
#endif
+static int virCgroupRemoveRecursively(char *grppath)
+{
+ DIR *grpdir;
+ struct dirent *ent;
+ int rc = 0;
+
+ grpdir = opendir(grppath);
+ if (grpdir == NULL) {
+ VIR_ERROR("Unable to open %s (%d)", grppath, errno);
+ rc = -errno;
+ return rc;
+ }
+
+ while ((ent = readdir(grpdir)) != NULL) {
+ char path[PATH_MAX];
+ int ret;
+
+ if (ent->d_name[0] == '.') continue;
+ if (ent->d_type != DT_DIR) continue;
+
+ ret = snprintf(path, sizeof(path), "%s/%s", grppath, ent->d_name);
+ if (ret < 0)
+ break;
+ rc = virCgroupRemoveRecursively(path);
+ if (rc != 0)
+ break;
+ }
+ DEBUG("Removing cgroup %s", grppath);
+ if (rmdir(grppath) != 0 && errno != ENOENT) {
+ rc = -errno;
+ VIR_ERROR("Unable to remove %s (%d)", grppath, errno);
+ }
+
+ return rc;
+}
+
/**
* virCgroupRemove:
*
* @group: The group to be removed
*
+ * It first removes all child groups recursively
+ * in depth first order and then removes @group
+ * because the presence of the child groups
+ * prevents removing @group.
+ *
* Returns: 0 on success
*/
int virCgroupRemove(virCgroupPtr group)
@@ -585,10 +627,8 @@ int virCgroupRemove(virCgroupPtr group)
&grppath) != 0)
continue;
- DEBUG("Removing cgroup %s", grppath);
- if (rmdir(grppath) != 0 && errno != ENOENT) {
- rc = -errno;
- }
+ DEBUG("Removing cgroup %s and all child cgroups", grppath);
+ rc = virCgroupRemoveRecursively(grppath);
VIR_FREE(grppath);
}
--
1.6.5.2
4:42 p.m.
New subject: [libvirt] [PATCH 1/2] cgroup: Change virCgroupRemove to remove all child groups at first
On 05/23/2010 07:15 AM, Ryota Ozaki wrote:
As same as normal directories, a cgroup cannot be removed if it
contains sub groups. This patch changes virCgroupRemove to remove
all child groups (subdirectories) of a target group before removing
the target group.
The handling is required when we run lxc with ns subsystem of cgroup.
Ns subsystem automatically creates child cgroups on every process
forks, but unfortunately the groups are not removed on process exits,
so we have to remove them by ourselves.
With this patch, such child groups are surely removed at lxc
shutdown, i.e., lxcVmCleanup which calls virCgroupRemove.
---
src/util/cgroup.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index b8b2eb5..531c131 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -23,6 +23,7 @@
#include <sys/stat.h>
#include <sys/types.h>
#include <libgen.h>
+#include <dirent.h>
#include "internal.h"
#include "util.h"
@@ -561,11 +562,52 @@ cleanup:
}
#endif
+static int virCgroupRemoveRecursively(char *grppath)
+{
+ DIR *grpdir;
+ struct dirent *ent;
+ int rc = 0;
+
+ grpdir = opendir(grppath);
+ if (grpdir == NULL) {
+ VIR_ERROR("Unable to open %s (%d)", grppath, errno);
+ rc = -errno;
+ return rc;
+ }
+
+ while ((ent = readdir(grpdir)) != NULL) {
Technically, any loop over readdir must first set errno to 0, then call
readdir, and if it is NULL, check if errno is still 0. Otherwise, you
can miss subtle readdir failures.
+ char path[PATH_MAX];
I'd rather get out of the habit of stack-allocating PATH_MAX bytes,
since it is not portably guaranteed to fit in a page of memory, and
especially since you are using recursion and will chew through lots of
stack in a deep hierarchy.
+ int ret;
+
+ if (ent->d_name[0] == '.') continue;
+ if (ent->d_type != DT_DIR) continue;
+
+ ret = snprintf(path, sizeof(path), "%s/%s", grppath, ent->d_name);
Rather, we can construct path via virAsprintf(), and VIR_FREE() it later.
+ if (ret < 0)
+ break;
+ rc = virCgroupRemoveRecursively(path);
+ if (rc != 0)
+ break;
+ }
+ DEBUG("Removing cgroup %s", grppath);
+ if (rmdir(grppath) != 0 && errno != ENOENT) {
+ rc = -errno;
+ VIR_ERROR("Unable to remove %s (%d)", grppath, errno);
+ }
+
+ return rc;
+}
+
/**
* virCgroupRemove:
*
* @group: The group to be removed
*
+ * It first removes all child groups recursively
+ * in depth first order and then removes @group
+ * because the presence of the child groups
+ * prevents removing @group.
+ *
* Returns: 0 on success
*/
int virCgroupRemove(virCgroupPtr group)
@@ -585,10 +627,8 @@ int virCgroupRemove(virCgroupPtr group)
&grppath) != 0)
continue;
- DEBUG("Removing cgroup %s", grppath);
- if (rmdir(grppath) != 0 && errno != ENOENT) {
- rc = -errno;
- }
+ DEBUG("Removing cgroup %s and all child cgroups", grppath);
+ rc = virCgroupRemoveRecursively(grppath);
But other than the missed error handling, this patch looks like the
right technical approach for the problem.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:51 a.m.
New subject: [libvirt] [PATCH 1/2] cgroup: Change virCgroupRemove to remove all child groups at first
Hi Eric.
Thanks for your kind review.
On Wed, Jun 23, 2010 at 6:42 AM, Eric Blake <eblake(a)redhat.com> wrote:
On 05/23/2010 07:15 AM, Ryota Ozaki wrote:
> As same as normal directories, a cgroup cannot be removed if it
> contains sub groups. This patch changes virCgroupRemove to remove
> all child groups (subdirectories) of a target group before removing
> the target group.
>
> The handling is required when we run lxc with ns subsystem of cgroup.
> Ns subsystem automatically creates child cgroups on every process
> forks, but unfortunately the groups are not removed on process exits,
> so we have to remove them by ourselves.
>
> With this patch, such child groups are surely removed at lxc
> shutdown, i.e., lxcVmCleanup which calls virCgroupRemove.
> ---
> src/util/cgroup.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
> 1 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/cgroup.c b/src/util/cgroup.c
> index b8b2eb5..531c131 100644
> --- a/src/util/cgroup.c
> +++ b/src/util/cgroup.c
> @@ -23,6 +23,7 @@
> #include <sys/stat.h>
> #include <sys/types.h>
> #include <libgen.h>
> +#include <dirent.h>
>
> #include "internal.h"
> #include "util.h"
> @@ -561,11 +562,52 @@ cleanup:
> }
> #endif
>
> +static int virCgroupRemoveRecursively(char *grppath)
> +{
> + DIR *grpdir;
> + struct dirent *ent;
> + int rc = 0;
> +
> + grpdir = opendir(grppath);
> + if (grpdir == NULL) {
> + VIR_ERROR("Unable to open %s (%d)", grppath, errno);
> + rc = -errno;
> + return rc;
> + }
> +
> + while ((ent = readdir(grpdir)) != NULL) {
Technically, any loop over readdir must first set errno to 0, then call
readdir, and if it is NULL, check if errno is still 0. Otherwise, you
can miss subtle readdir failures.
Oh, right. So revised version will be like this:
for (;;) {
errno = 0;
ent = readdir(grpdir);
if (ent == NULL) {
if (errno)
VIR_ERROR(_("Failed to readdir for %s (%d)")_, grppath, errno);
break;
}
...
}
If I miss something, please let me know.
> + char path[PATH_MAX];
I'd rather get out of the habit of stack-allocating PATH_MAX bytes,
since it is not portably guaranteed to fit in a page of memory, and
especially since you are using recursion and will chew through lots of
stack in a deep hierarchy.
I just followed other codes that use PATH_MAX, but indeed, the codes
are not recursive unlike mine.
OK. I'll fix.
> + int ret;
> +
> + if (ent->d_name[0] == '.') continue;
> + if (ent->d_type != DT_DIR) continue;
> +
> + ret = snprintf(path, sizeof(path), "%s/%s", grppath,
ent->d_name);
Rather, we can construct path via virAsprintf(), and VIR_FREE() it later.
> + if (ret < 0)
> + break;
> + rc = virCgroupRemoveRecursively(path);
> + if (rc != 0)
> + break;
> + }
> + DEBUG("Removing cgroup %s", grppath);
> + if (rmdir(grppath) != 0 && errno != ENOENT) {
> + rc = -errno;
> + VIR_ERROR("Unable to remove %s (%d)", grppath, errno);
> + }
> +
> + return rc;
> +}
> +
> /**
> * virCgroupRemove:
> *
> * @group: The group to be removed
> *
> + * It first removes all child groups recursively
> + * in depth first order and then removes @group
> + * because the presence of the child groups
> + * prevents removing @group.
> + *
> * Returns: 0 on success
> */
> int virCgroupRemove(virCgroupPtr group)
> @@ -585,10 +627,8 @@ int virCgroupRemove(virCgroupPtr group)
> &grppath) != 0)
> continue;
>
> - DEBUG("Removing cgroup %s", grppath);
> - if (rmdir(grppath) != 0 && errno != ENOENT) {
> - rc = -errno;
> - }
> + DEBUG("Removing cgroup %s and all child cgroups", grppath);
> + rc = virCgroupRemoveRecursively(grppath);
But other than the missed error handling, this patch looks like the
right technical approach for the problem.
Thanks again.
ozaki-r
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:37 a.m.
New subject: [libvirt] [PATCH 1/2] cgroup: Change virCgroupRemove to remove all child groups at first
On 06/23/2010 05:51 AM, Ryota Ozaki wrote:
> Technically, any loop over readdir must first set errno to 0,
then call
> readdir, and if it is NULL, check if errno is still 0. Otherwise, you
> can miss subtle readdir failures.
Oh, right. So revised version will be like this:
for (;;) {
errno = 0;
ent = readdir(grpdir);
if (ent == NULL) {
if (errno)
VIR_ERROR(_("Failed to readdir for %s (%d)")_, grppath, errno);
break;
}
...
}
If I miss something, please let me know.
Yes, that is a correct usage pattern for readdir.
Also, I missed it the first time, but you are leaking fd's - you called
grpdir=opendir(grppath), but never called closedir(grpdir).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:48 a.m.
New subject: [libvirt] [PATCH 1/2] cgroup: Change virCgroupRemove to remove all child groups at first
On Wed, Jun 23, 2010 at 10:37 PM, Eric Blake <eblake(a)redhat.com> wrote:
On 06/23/2010 05:51 AM, Ryota Ozaki wrote:
>> Technically, any loop over readdir must first set errno to 0, then call
>> readdir, and if it is NULL, check if errno is still 0. Otherwise, you
>> can miss subtle readdir failures.
>
> Oh, right. So revised version will be like this:
>
> for (;;) {
> errno = 0;
> ent = readdir(grpdir);
> if (ent == NULL) {
> if (errno)
> VIR_ERROR(_("Failed to readdir for %s (%d)")_, grppath, errno);
> break;
> }
> ...
> }
>
> If I miss something, please let me know.
Yes, that is a correct usage pattern for readdir.
Thank you for the confirmation.
Also, I missed it the first time, but you are leaking fd's - you called
grpdir=opendir(grppath), but never called closedir(grpdir).
Oops! I'll fix it as well.
Thanks!
ozaki-r
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:15 a.m.
New subject: [libvirt] [PATCH 2/2] cgroup: Enable memory.use_hierarchy of cgroup for domain
Through conversation with Kumar L Srikanth-B22348, I found
that the function of getting memory usage (e.g., virsh dominfo)
doesn't work for lxc with ns subsystem of cgroup enabled.
This is because of features of ns and memory subsystems.
Ns creates child cgroup on every process fork and as a result
processes in a container are not assigned in a cgroup for
domain (e.g., libvirt/lxc/test1/). For example, libvirt_lxc
and init (or somewhat specified in XML) are assigned into
libvirt/lxc/test1/8839/ and libvirt/lxc/test1/8839/8849/,
respectively. On the other hand, memory subsystem accounts
memory usage within a group of processes by default, i.e.,
it does not take any child (and descendent) groups into
account. With the two features, virsh dominfo which just
checks memory usage of a cgroup for domain always returns
zero because the cgroup has no process.
Setting memory.use_hierarchy of a group allows to account
(and limit) memory usage of every descendent groups of the group.
By setting it of a cgroup for domain, we can get proper memory
usage of lxc with ns subsystem enabled. (To be exact, the
setting is required only when memory and ns subsystems are
enabled at the same time, e.g., mount -t cgroup none /cgroup.)
---
src/util/cgroup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index b8b2eb5..f7d6b41 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -443,7 +443,38 @@ static int virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr
group)
return rc;
}
-static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group, int create)
+static int virCgroupSetMemoryUseHierarchy(virCgroupPtr group)
+{
+ int rc = 0;
+ unsigned long long value;
+ const char *filename = "memory.use_hierarchy";
+
+ rc = virCgroupGetValueU64(group,
+ VIR_CGROUP_CONTROLLER_MEMORY,
+ filename, &value);
+ if (rc != 0) {
+ VIR_ERROR("Failed to read %s/%s (%d)", group->path, filename, rc);
+ return rc;
+ }
+
+ /* Setting twice causes error, so if already enabled, skip setting */
+ if (value == 1)
+ return 0;
+
+ VIR_DEBUG("Setting up %s/%s", group->path, filename);
+ rc = virCgroupSetValueU64(group,
+ VIR_CGROUP_CONTROLLER_MEMORY,
+ filename, 1);
+
+ if (rc != 0) {
+ VIR_ERROR("Failed to set %s/%s (%d)", group->path, filename, rc);
+ }
+
+ return rc;
+}
+
+static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group,
+ int create, int memory_hierarchy)
{
int i;
int rc = 0;
@@ -477,6 +508,20 @@ static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr
group, int creat
break;
}
}
+ /*
+ * Note that virCgroupSetMemoryUseHierarchy should always be
+ * called prior to creating subcgroups and attaching tasks.
+ */
+ if (memory_hierarchy &&
+ group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint != NULL
&&
+ (i == VIR_CGROUP_CONTROLLER_MEMORY ||
+ STREQ(group->controllers[i].mountPoint,
group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint))) {
+ rc = virCgroupSetMemoryUseHierarchy(group);
+ if (rc != 0) {
+ VIR_FREE(path);
+ break;
+ }
+ }
}
VIR_FREE(path);
@@ -553,7 +598,7 @@ static int virCgroupAppRoot(int privileged,
if (rc != 0)
goto cleanup;
- rc = virCgroupMakeGroup(rootgrp, *group, create);
+ rc = virCgroupMakeGroup(rootgrp, *group, create, 0);
cleanup:
virCgroupFree(&rootgrp);
@@ -653,7 +698,7 @@ int virCgroupForDriver(const char *name,
VIR_FREE(path);
if (rc == 0) {
- rc = virCgroupMakeGroup(rootgrp, *group, create);
+ rc = virCgroupMakeGroup(rootgrp, *group, create, 0);
if (rc != 0)
virCgroupFree(group);
}
@@ -703,7 +748,17 @@ int virCgroupForDomain(virCgroupPtr driver,
VIR_FREE(path);
if (rc == 0) {
- rc = virCgroupMakeGroup(driver, *group, create);
+ /*
+ * Create a cgroup with memory.use_hierarchy enabled to
+ * surely account memory usage of lxc with ns subsystem
+ * enabled. (To be exact, memory and ns subsystems are
+ * enabled at the same time.)
+ *
+ * The reason why doing it here, not a upper group, say
+ * a group for driver, is to avoid overhead to track
+ * cumulative usage that we don't need.
+ */
+ rc = virCgroupMakeGroup(driver, *group, create, 1);
if (rc != 0)
virCgroupFree(group);
}
--
1.6.5.2
4:50 p.m.
New subject: [libvirt] [PATCH 2/2] cgroup: Enable memory.use_hierarchy of cgroup for domain
On 05/23/2010 07:15 AM, Ryota Ozaki wrote:
[Apologies for the delayed review, and for the fact that you had to ping
us to remind us this had not been visited]
Through conversation with Kumar L Srikanth-B22348, I found
that the function of getting memory usage (e.g., virsh dominfo)
doesn't work for lxc with ns subsystem of cgroup enabled.
This is because of features of ns and memory subsystems.
Ns creates child cgroup on every process fork and as a result
processes in a container are not assigned in a cgroup for
domain (e.g., libvirt/lxc/test1/). For example, libvirt_lxc
and init (or somewhat specified in XML) are assigned into
libvirt/lxc/test1/8839/ and libvirt/lxc/test1/8839/8849/,
respectively. On the other hand, memory subsystem accounts
memory usage within a group of processes by default, i.e.,
it does not take any child (and descendent) groups into
account. With the two features, virsh dominfo which just
checks memory usage of a cgroup for domain always returns
zero because the cgroup has no process.
Setting memory.use_hierarchy of a group allows to account
(and limit) memory usage of every descendent groups of the group.
s/descendent/descendant/
By setting it of a cgroup for domain, we can get proper memory
usage of lxc with ns subsystem enabled. (To be exact, the
setting is required only when memory and ns subsystems are
enabled at the same time, e.g., mount -t cgroup none /cgroup.)
---
src/util/cgroup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index b8b2eb5..f7d6b41 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -443,7 +443,38 @@ static int virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr
group)
return rc;
}
-static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group, int create)
+static int virCgroupSetMemoryUseHierarchy(virCgroupPtr group)
+{
+ int rc = 0;
+ unsigned long long value;
+ const char *filename = "memory.use_hierarchy";
+
+ rc = virCgroupGetValueU64(group,
+ VIR_CGROUP_CONTROLLER_MEMORY,
+ filename, &value);
+ if (rc != 0) {
+ VIR_ERROR("Failed to read %s/%s (%d)", group->path, filename, rc);
Needs _() translation these days; 'make syntax-check' should have caught
this.
+ return rc;
+ }
+
+ /* Setting twice causes error, so if already enabled, skip setting */
+ if (value == 1)
+ return 0;
+
+ VIR_DEBUG("Setting up %s/%s", group->path, filename);
Debug remains untranslated...
+ rc = virCgroupSetValueU64(group,
+ VIR_CGROUP_CONTROLLER_MEMORY,
+ filename, 1);
+
+ if (rc != 0) {
+ VIR_ERROR("Failed to set %s/%s (%d)", group->path, filename, rc);
...but another VIR_ERROR that needs translation.
+ }
+
+ return rc;
+}
+
+static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group,
+ int create, int memory_hierarchy)
Based on how this is being used, it seems like you should use
bool/true/false rather than int/0/1 for memory_hierarchy.
{
int i;
int rc = 0;
@@ -477,6 +508,20 @@ static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr
group, int creat
break;
}
}
+ /*
+ * Note that virCgroupSetMemoryUseHierarchy should always be
+ * called prior to creating subcgroups and attaching tasks.
+ */
+ if (memory_hierarchy &&
+ group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint != NULL
&&
+ (i == VIR_CGROUP_CONTROLLER_MEMORY ||
+ STREQ(group->controllers[i].mountPoint,
group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint))) {
+ rc = virCgroupSetMemoryUseHierarchy(group);
+ if (rc != 0) {
+ VIR_FREE(path);
+ break;
+ }
+ }
}
VIR_FREE(path);
@@ -553,7 +598,7 @@ static int virCgroupAppRoot(int privileged,
if (rc != 0)
goto cleanup;
- rc = virCgroupMakeGroup(rootgrp, *group, create);
+ rc = virCgroupMakeGroup(rootgrp, *group, create, 0);
ACK with those nits addressed. Hopefully a v2 submission won't take as
much time on the review side of things.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
7:03 a.m.
New subject: [libvirt] [PATCH 2/2] cgroup: Enable memory.use_hierarchy of cgroup for domain
On Wed, Jun 23, 2010 at 6:50 AM, Eric Blake <eblake(a)redhat.com> wrote:
On 05/23/2010 07:15 AM, Ryota Ozaki wrote:
[Apologies for the delayed review, and for the fact that you had to ping
us to remind us this had not been visited]
Nevermind. I'm also sometimes late ;-)
> Through conversation with Kumar L Srikanth-B22348, I found
> that the function of getting memory usage (e.g., virsh dominfo)
> doesn't work for lxc with ns subsystem of cgroup enabled.
>
> This is because of features of ns and memory subsystems.
> Ns creates child cgroup on every process fork and as a result
> processes in a container are not assigned in a cgroup for
> domain (e.g., libvirt/lxc/test1/). For example, libvirt_lxc
> and init (or somewhat specified in XML) are assigned into
> libvirt/lxc/test1/8839/ and libvirt/lxc/test1/8839/8849/,
> respectively. On the other hand, memory subsystem accounts
> memory usage within a group of processes by default, i.e.,
> it does not take any child (and descendent) groups into
> account. With the two features, virsh dominfo which just
> checks memory usage of a cgroup for domain always returns
> zero because the cgroup has no process.
>
> Setting memory.use_hierarchy of a group allows to account
> (and limit) memory usage of every descendent groups of the group.
s/descendent/descendant/
ouch.
> By setting it of a cgroup for domain, we can get proper memory
> usage of lxc with ns subsystem enabled. (To be exact, the
> setting is required only when memory and ns subsystems are
> enabled at the same time, e.g., mount -t cgroup none /cgroup.)
> ---
> src/util/cgroup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/cgroup.c b/src/util/cgroup.c
> index b8b2eb5..f7d6b41 100644
> --- a/src/util/cgroup.c
> +++ b/src/util/cgroup.c
> @@ -443,7 +443,38 @@ static int virCgroupCpuSetInherit(virCgroupPtr parent,
virCgroupPtr group)
> return rc;
> }
>
> -static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group, int create)
> +static int virCgroupSetMemoryUseHierarchy(virCgroupPtr group)
> +{
> + int rc = 0;
> + unsigned long long value;
> + const char *filename = "memory.use_hierarchy";
> +
> + rc = virCgroupGetValueU64(group,
> + VIR_CGROUP_CONTROLLER_MEMORY,
> + filename, &value);
> + if (rc != 0) {
> + VIR_ERROR("Failed to read %s/%s (%d)", group->path, filename,
rc);
Needs _() translation these days; 'make syntax-check' should have caught
this.
Oh, sorry. I'll fix all you mention in this patch as well as missing ones in
the previous patch.
> + return rc;
> + }
> +
> + /* Setting twice causes error, so if already enabled, skip setting */
> + if (value == 1)
> + return 0;
> +
> + VIR_DEBUG("Setting up %s/%s", group->path, filename);
Debug remains untranslated...
> + rc = virCgroupSetValueU64(group,
> + VIR_CGROUP_CONTROLLER_MEMORY,
> + filename, 1);
> +
> + if (rc != 0) {
> + VIR_ERROR("Failed to set %s/%s (%d)", group->path, filename,
rc);
...but another VIR_ERROR that needs translation.
> + }
> +
> + return rc;
> +}
> +
> +static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr group,
> + int create, int memory_hierarchy)
Based on how this is being used, it seems like you should use
bool/true/false rather than int/0/1 for memory_hierarchy.
Indeed. Will fix.
> {
> int i;
> int rc = 0;
> @@ -477,6 +508,20 @@ static int virCgroupMakeGroup(virCgroupPtr parent, virCgroupPtr
group, int creat
> break;
> }
> }
> + /*
> + * Note that virCgroupSetMemoryUseHierarchy should always be
> + * called prior to creating subcgroups and attaching tasks.
> + */
> + if (memory_hierarchy &&
> + group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint !=
NULL &&
> + (i == VIR_CGROUP_CONTROLLER_MEMORY ||
> + STREQ(group->controllers[i].mountPoint,
group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint))) {
> + rc = virCgroupSetMemoryUseHierarchy(group);
> + if (rc != 0) {
> + VIR_FREE(path);
> + break;
> + }
> + }
> }
>
> VIR_FREE(path);
> @@ -553,7 +598,7 @@ static int virCgroupAppRoot(int privileged,
> if (rc != 0)
> goto cleanup;
>
> - rc = virCgroupMakeGroup(rootgrp, *group, create);
> + rc = virCgroupMakeGroup(rootgrp, *group, create, 0);
>
ACK with those nits addressed. Hopefully a v2 submission won't take as
much time on the review side of things.
Thanks.
ozaki-r
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5300
days inactive
5331
days old
8 comments
2 participants
participants (2)
-
Eric Blake -
Ryota Ozaki