<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small">Hi Richard,</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 25, 2020 at 8:20 PM Richard Palethorpe <<a href="mailto:rpalethorpe@suse.com" target="_blank">rpalethorpe@suse.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The best way to find out if we can do something is to try it, we don't<br>
check if the system has enough RAM and PIDs available before calling<br>
fork() in safe_fork(). Currently we try to guess what cgroups are<br>
currently in use then try to use the same version. We guess by<br>
grepping the mounts and filesystem files, these files need to be<br>
parsed in a structured way and even then, it is not the job of all<br>
tests which *use* cgroups to test that if cgroup(2) is present in<br>
mounts or filesystem that it can then be used.<br>
<br>
The cpuset group is only available on V1 and we can mount and use V1<br>
even if V2 is active. Just because the system has V2 active does not<br>
mean we cannot execute tests which require V1. This is one example<br>
where trying to figure out ahead of time what can or can't be used<br>
results in less testing.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Good point.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">My previous thought on the Cgroup test-lib design was only to respect one version,</div><div class="gmail_default" style="font-size:small">because the upstream developer hopes to use V2 replaces the V1 step by step.</div><div class="gmail_default" style="font-size:small">So the key point of work mainly focused on the supported&mounted version, </div><div class="gmail_default" style="font-size:small">which sounds like Cgroup itself is the leading actor, and to easily extend Cgroup</div><div class="gmail_default" style="font-size:small">itself testing according to different versions.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default">But I didn't realize that there will be a mixed using situation we have to take care of.</div><div class="gmail_default">At that moment, the Cgroup test-lib actor as an assistant in the general test case.</div><div class="gmail_default"><br></div><div class="gmail_default">Anyway, this patch looks practicable and fitted for the period of transition of Cgroup.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Furthermore removing these checks results in a ~50% reduction in code<br>
and this is without correct parsing and checking of mounts and<br>
filesystems. We also have to consider that just because one V1<br>
controller is mounted, this does not prevent all V2 controllers from<br>
being used. So someone may mount V1 cpuset for legacy reasons, while<br>
using V2 for other controllers. To account for this we would need to<br>
check each controller individually.<br>
<br>
Note that the above may be a valid thing to do in a test checking<br>
specific cgroup behavior, but this library code is meant for use by<br>
all tests which need cgroups for some reason.<br>
---<br>
include/tst_cgroup.h | 2 -<br>
lib/tst_cgroup.c | 118 ++++++++++++++-----------------------------<br>
2 files changed, 39 insertions(+), 81 deletions(-)<br>
<br>
diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h<br>
index 77780e0d6..62d381ce9 100644<br>
--- a/include/tst_cgroup.h<br>
+++ b/include/tst_cgroup.h<br>
@@ -21,8 +21,6 @@ enum tst_cgroup_ctrl {<br>
/* add cgroup controller */<br>
};<br>
<br>
-enum tst_cgroup_ver tst_cgroup_version(void);<br>
-<br>
/* To mount/umount specified cgroup controller on 'cgroup_dir' path */<br>
void tst_cgroup_mount(enum tst_cgroup_ctrl ctrl, const char *cgroup_dir);<br>
void tst_cgroup_umount(const char *cgroup_dir);<br>
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c<br>
index ba413d874..887423bc6 100644<br>
--- a/lib/tst_cgroup.c<br>
+++ b/lib/tst_cgroup.c<br>
@@ -21,47 +21,6 @@<br>
static enum tst_cgroup_ver tst_cg_ver;<br>
static int clone_children;<br>
<br>
-static int tst_cgroup_check(const char *cgroup)<br>
-{<br>
- char line[PATH_MAX];<br>
- FILE *file;<br>
- int cg_check = 0;<br>
-<br>
- file = SAFE_FOPEN("/proc/filesystems", "r");<br>
- while (fgets(line, sizeof(line), file)) {<br>
- if (strstr(line, cgroup) != NULL) {<br>
- cg_check = 1;<br>
- break;<br>
- }<br>
- }<br>
- SAFE_FCLOSE(file);<br>
-<br>
- return cg_check;<br>
-}<br>
-<br>
-enum tst_cgroup_ver tst_cgroup_version(void)<br>
-{<br>
- enum tst_cgroup_ver cg_ver;<br>
-<br>
- if (tst_cgroup_check("cgroup2")) {<br>
- if (!tst_is_mounted("cgroup2") && tst_is_mounted("cgroup"))<br>
- cg_ver = TST_CGROUP_V1;<br>
- else<br>
- cg_ver = TST_CGROUP_V2;<br>
-<br>
- goto out;<br>
- }<br>
-<br>
- if (tst_cgroup_check("cgroup"))<br>
- cg_ver = TST_CGROUP_V1;<br>
-<br>
- if (!cg_ver)<br>
- tst_brk(TCONF, "Cgroup is not configured");<br>
-<br>
-out:<br>
- return cg_ver;<br>
-}<br>
-<br>
static void tst_cgroup1_mount(const char *name, const char *option,<br>
const char *mnt_path, const char *new_path)<br>
{<br>
@@ -100,26 +59,18 @@ out:<br>
tst_res(TINFO, "Cgroup(%s) v1 mount at %s success", option, mnt_path);<br>
}<br>
<br>
-static void tst_cgroup2_mount(const char *mnt_path, const char *new_path)<br>
+static int cgroup2_mount(const char *mnt_path, const char *new_path)<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">We'd like to make the series function name starts with tst_*.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
{<br>
- if (tst_is_mounted(mnt_path))<br>
- goto out;<br>
+ if (!tst_is_mounted(mnt_path)) {<br>
+ SAFE_MKDIR(mnt_path, 0777);<br>
<br>
- SAFE_MKDIR(mnt_path, 0777);<br>
- if (mount("cgroup2", mnt_path, "cgroup2", 0, NULL) == -1) {<br>
- if (errno == ENODEV) {<br>
- if (rmdir(mnt_path) == -1)<br>
- tst_res(TWARN | TERRNO, "rmdir %s failed", mnt_path);<br>
- tst_brk(TCONF,<br>
- "Cgroup v2 is not configured in kernel");<br>
- }<br>
- tst_brk(TBROK | TERRNO, "mount %s", mnt_path);<br>
+ if (mount("cgroup2", mnt_path, "cgroup2", 0, NULL) == -1)<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Here we have to remove mnt_path if the mount fails since it also tries to create</div><div class="gmail_default" style="font-size:small">the same path in V1 then.</div><div class="gmail_default" style="font-size:small"><br></div>+ if (rmdir(mnt_path) == -1)<br>+ tst_res(TWARN | TERRNO, "rmdir %s", mnt_path);<br><div class="gmail_default" style="font-size:small"></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ return -1;<br>
}<br>
<br>
-out:<br>
SAFE_MKDIR(new_path, 0777);<br>
<br>
- tst_res(TINFO, "Cgroup v2 mount at %s success", mnt_path);<br>
+ return 0;<br>
}<br>
<br>
static void tst_cgroupN_umount(const char *mnt_path, const char *new_path)<br>
@@ -274,39 +225,48 @@ void tst_cgroup_mount(enum tst_cgroup_ctrl ctrl, const char *cgroup_dir)<br>
char *cgroup_new_dir;<br>
char knob_path[PATH_MAX];<br>
<br>
- tst_cg_ver = tst_cgroup_version();<br>
-<br>
tst_cgroup_set_path(cgroup_dir);<br>
cgroup_new_dir = tst_cgroup_get_path(cgroup_dir);<br>
<br>
- if (tst_cg_ver & TST_CGROUP_V1) {<br>
- switch(ctrl) {<br>
- case TST_CGROUP_MEMCG:<br>
- tst_cgroup1_mount("memcg", "memory", cgroup_dir, cgroup_new_dir);<br>
- break;<br>
- case TST_CGROUP_CPUSET:<br>
- tst_cgroup1_mount("cpusetcg", "cpuset", cgroup_dir, cgroup_new_dir);<br>
- break;<br>
- default:<br>
- tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);<br>
- }<br>
+ if (ctrl == TST_CGROUP_CPUSET) {<br>
+ tst_res(TINFO, "CGroup V2 lacks cpuset subsystem, trying V1");<br>
+ goto cgroup_v1;<br>
}<br>
<br>
- if (tst_cg_ver & TST_CGROUP_V2) {<br>
- tst_cgroup2_mount(cgroup_dir, cgroup_new_dir);<br>
+ if (cgroup2_mount(cgroup_dir, cgroup_new_dir)) {<br>
+ tst_res(TINFO | TERRNO, "Mounting CGroup V2 failed, trying V1");<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Can we add the diagnostic check when mounting Cgroup V2 failed?</div></div><div class="gmail_default" style="font-size:small">(i.e reserve the tst_cgroup_check() function and used in tst_cgroup2_mount())</div><div class="gmail_default" style="font-size:small"><br></div><span class="gmail_default" style="font-size:small"> </span>if (tst_cgroup_check("cgroup2"))</div><div class="gmail_quote"> <span class="gmail_default" style="font-size:small"> </span>warning for mount<span class="gmail_default" style="font-size:small">ing</span> failed, else ignore th<span class="gmail_default" style="font-size:small">is failure</span></div><div class="gmail_quote"><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ goto cgroup_v1;<br>
+ }<br>
+<br>
+ tst_res(TINFO, "Mounted CGroup V2");<br>
+<br>
+ switch(ctrl) {<br>
+ case TST_CGROUP_MEMCG:<br>
+ sprintf(knob_path, "%s/cgroup.subtree_control", cgroup_dir);<br>
+ if (FILE_PRINTF(knob_path, "%s", "+memory")) {<br>
+ tst_res(TINFO,<br>
+ "Can't add V2 memory controller, this might be because it is mounted as V1");<br></blockquote><div> </div><div><span class="gmail_default" style="font-size:small">Seems we have to </span>umount <span class="gmail_default" style="font-size:small">Cgroup_V</span>2 and <span class="gmail_default" style="font-size:small">do the </span>cleanup<span class="gmail_default" style="font-size:small"> for the shift to Cgroup_V1.</span></div><div><br></div>if (rmdir(cgroup_new_dir) == -1)<br><span class="gmail_default" style="font-size:small"> </span>tst_res(TWARN | TERRNO, "rmdir %s", cgroup_new_dir);<br>if (umount(cgroup_dir) == -1)<br><span class="gmail_default" style="font-size:small"> </span>tst_res(TWARN | TERRNO, "umount %s", cgroup_dir);<br>if (rmdir(cgroup_dir) == -1)<br><span class="gmail_default" style="font-size:small"> </span>tst_res(TWARN | TERRNO, "rmdir %s", cgroup_dir);<br><div><span class="gmail_default"></span> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ break;<br>
+ }<br>
+ tst_cg_ver = TST_CGROUP_V2;<br>
+ return;<br>
+ default:<br>
+ tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);<br>
+ }<br>
<br>
- switch(ctrl) {<br>
- case TST_CGROUP_MEMCG:<br>
- sprintf(knob_path, "%s/cgroup.subtree_control", cgroup_dir);<br>
- SAFE_FILE_PRINTF(knob_path, "%s", "+memory");<br>
+cgroup_v1:<br>
+ switch(ctrl) {<br>
+ case TST_CGROUP_MEMCG:<br>
+ tst_cgroup1_mount("memcg", "memory", cgroup_dir, cgroup_new_dir);<br>
break;<br>
- case TST_CGROUP_CPUSET:<br>
- tst_brk(TCONF, "Cgroup v2 hasn't achieve cpuset subsystem");<br>
+ case TST_CGROUP_CPUSET:<br>
+ tst_cgroup1_mount("cpusetcg", "cpuset", cgroup_dir, cgroup_new_dir);<br>
break;<br>
- default:<br>
- tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);<br>
- }<br>
+ default:<br>
+ tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);<br>
}<br>
+<br>
+ tst_cg_ver = TST_CGROUP_V1;<br>
}<br>
<br>
void tst_cgroup_umount(const char *cgroup_dir)<br>
-- <br>
2.28.0<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>