On 5/21/20 9:44 AM, Michal Koutný wrote:
Hello.
Just some questions and readability suggestion.
On Mon, May 11, 2020 at 09:30:13AM -0600, Tom Hromatka
<tom.hroma...@oracle.com> wrote:
This commit adds cgroup v2 support to cgroup_init(). With these
changes, cgget and cgset now work on a cgroup v1 mount, a cgroup
v2 mount, or a cgroup v2 unified mount hierarchy.
What exactly is the difference between the last two mounts?
Would they be treated differently?
Good question. There are a couple differences that could affect
users:
1. The unified hierarchy is automatically mounted by systemd at
startup. Often (but definitely not always), a cgroup v2
mount is done by a user using the mount command after the
system has booted.
2. Systemd mounts the unified hierarchy at /sys/fs/cgroup/ whereas
a user is free to mount a cgroup v2 mount wherever they want.
There may be other differences as well. Ultimately neither difference
outlined above is a big deal, but I feel they are worth mentioning
as they have tripped up some of my users of cgroups.
Would the cgset really work on v2 hierarchy if controller
cgroup.subtree_control isn't modified? (I don't mean extending the patch
but rather better describe it in its message.)
That's an interesting thought. As you alluded to, both cgget and cgset
will fail if the requested controller isn't enabled in subtree_control.
And I think that's okay for the moment, as enabling/disabling controllers
should be done via a different mechanism. But I do think all of libcgroup's
error messages need to be reviewed and possibly updated. (We're currently
going through this in libseccomp, and it's a major undertaking.)
Here's what this patchset's cgset and cgget report when a v2 controller
isn't enabled. I haven't really thought about it yet if these errors
are appropriate or even helpful.
$ sudo ./cgget -r cpuset.cpus foo
foo:
variable file read failed No such file or directory
cpuset.cpus:
$ sudo ./cgset -r cpuset.cpus=1 foo
lt-cgset: cgroup modify error: Cgroup, requested group parameter does
not exist
+ stok_buff = strtok(ret_c, " ");
+ while (stok_buff) {
while ((stok_buff = strtok(ret_c, " "))
+ if (duplicate) {
+ cgroup_dbg("controller %s is already mounted on %s\n",
+ stok_buff, cg_mount_table[i].mount.path);
+
+ ret = cg_add_duplicate_mount(&cg_mount_table[i],
+ ent->mnt_dir);
+ if (ret)
+ goto out;
goto unnecessary, use break.
Sure.
+
+ /* advance to the next controller */
+ stok_buff = strtok(NULL, " ");
+ continue;
Same strtok at three places is asking for bugs. See the suggested
while() condition above.
Also why not use strtok_r for consistency with cgroup_process_v1_mount?
Good call. During an early prototype, I copied the logic from another
area in libcgroup with the intention of updating it to use strtok_r
but never did it. Thank you.
+ }
+
[...]
+
+ /* advance to the next controller */
+ stok_buff = strtok(NULL, " ");
The third.
Michal
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
