<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 9, 2020 at 6:42 PM Jan Stancek <<a href="mailto:jstancek@redhat.com">jstancek@redhat.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"><br>
----- Original Message -----<br>
> Many of our LTP tests need Control Group in the configuration,<br>
> this patch makes cgroup unified mounting at setup phase to be<br>
> possible. The method is extracted from mem.h with the purpose<br>
> of extendible for further developing, and trying to compatible<br>
> the current two versions of cgroup,<br>
> <br>
> It's hard to make all APIs be strictly consistent because there<br>
> are many differences between v1 and v2. But it capsulate the detail<br>
> of cgroup mounting in high-level functions, which will be easier<br>
> to use cgroup without considering too much technical thing.<br>
> <br>
> Btw, test get passed on RHEL7(x86_64), RHEL8(ppc64le), Fedora32(x86_64).<br>
> <br>
<br>
No strong objections to v4, couple comments below (if you spin v5 because<br>
of other reviews).<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Thanks for review!</div><div class="gmail_default" style="font-size:small">Ok, I agree to keep patchv4 posts for more days in case other people have comments.</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>
> +2.2.36 Using Control Group<br>
> +^^^^^^^^^^^^^^^^^^^^^^^^^^<br>
<br>
Would be nice if there was short description of each function.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">I'd add some code comments in the tst_cgroup.h header file. Which something maybe like:</div><div class="gmail_default" style="font-size:small"><br></div>/* 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><br>/* To move current process PID to the mounted cgroup tasks */<br>void tst_cgroup_move_current(const char *cgroup_dir);<br><br>/* To set cgroup controller knob with new value */<br>void tst_cgroup_set_knob(const char *cgroup_dir, const char *knob, long value);<br><br>/* Set of functions to set knobs under the memory controller */<br>void tst_cgroup_mem_set_maxbytes(const char *cgroup_dir, long memsz);<br>int tst_cgroup_mem_swapacct_enabled(const char *cgroup_dir);<br>void tst_cgroup_mem_set_maxswap(const char *cgroup_dir, long memsz);<br><br>/* Set of functions to read/write cpuset controller files content */<br>void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char *filename, char *retbuf);<br>void tst_cgroup_cpuset_write_files(const char *cgroup_dir, const char *filename, const char *buf);<br><br></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>
> +static void tst_cgroup_set_path(const char *cgroup_dir)<br>
> +{<br>
> + char cgroup_new_dir[PATH_MAX];<br>
> + struct tst_cgroup_path *tst_cgroup_path, *a;<br>
> +<br>
> + if (!cgroup_dir)<br>
> + tst_brk(TBROK, "Invalid cgroup dir, plese check cgroup_dir");<br>
> +<br>
> + sprintf(cgroup_new_dir, "%s/ltp_%d", cgroup_dir, rand());<br>
> +<br>
> + /* To store cgroup path in the 'path' list */<br>
> + tst_cgroup_path = SAFE_MMAP(NULL, (sizeof(struct tst_cgroup_path)),<br>
> + PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);<br>
<br>
This looks like it could use just SAFE_MALLOC/SAFE_FREE.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Agree. </div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">And btw, seems we have to set cgroup.clone_children as 1, otherwise, we can't write anything to the cpuset subsystem files.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small"> BROK: Failed to close FILE '/tmp/cgroup_cst/ltp_1804289383/tasks' at tst_cgroup.c:296: ENOSPC (28)<br></div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">So these lines will be add in to library as neccesary:</div><div class="gmail_default" style="font-size:small"><br></div>--- a/lib/tst_cgroup.c<br>+++ b/lib/tst_cgroup.c<br>@@ -251,6 +251,16 @@ void tst_cgroup_mount(enum tst_cgroup_ctrl ctrl, const char *cgroup_dir)<br> break;<br> case TST_CGROUP_CPUSET:<br> tst_cgroup1_mount("cpusetcg", "cpuset", cgroup_dir, cgroup_new_dir);<br>+ /*<br>+ * we should assign one or more memory nodes to cpuset.mems<br>+ * and cpuset.cpus, otherwise, echo $$ > tasks gives “no space<br>+ * left on device: ENOSPC” when trying to use cpuset.<br>+ *<br>+ * Or, setting cgroup.clone_children to 1 can help in automatically<br>+ * inheriting memory and node setting from parent cgroup when a<br>+ * child cgroup is created.<br>+ */<br>+ tst_cgroup_set_knob(cgroup_dir, "../cgroup.clone_children", 1);<br> break;<br><br></div><div><br></div></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>