<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, May 26, 2020 at 4:27 PM Jan Stancek <<a href="mailto:jstancek@redhat.com" target="_blank">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>
<br>
> +<br>
> +/* cgroup v1 */<br>
> +#define PATH_TMP_CG1_MEM     "/tmp/cgroup1_mem"<br>
> +#define PATH_TMP_CG1_CST     "/tmp/cgroup1_cpuset"<br>
<br>
It's only used for mount, so not sure if making it relative to TMPDIR<br>
buys us anything.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">I choose to use /tmp dir just because the cgroup is</div><div class="gmail_default" style="font-size:small">mounted temporarily for LTP test and destroyed</div><div class="gmail_default" style="font-size:small">after using. And I have no preference to use other</div><div class="gmail_default" style="font-size:small">places(i.e. /mnt or /dev).</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>
> +<br>
> +/* cgroup v1 */<br>
> +void tst_mount_cgroup1(const char *name, const char *option,<br>
> +                     const char *mnt_path, const char *new_path)<br>
<br>
I'd make all cgroup API start with tst_cgroup*.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Good point. I agree to make the function start with that prefix.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">The logic in this patch looks like:</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">tst_cgroup1_mount(...)</div><div class="gmail_default" style="font-size:small">tst_cgroup2_mount(...)</div><div class="gmail_default" style="font-size:small">// these two API as the basic/internal function for a different version of cgroup mounting</div><br></div><div><div class="gmail_default" style="font-size:small">tst_cgroup1_mem(){</div><div class="gmail_default" style="font-size:small">    tst_cgroup1_mount("with mem para")</div><div class="gmail_default" style="font-size:small">}</div><div class="gmail_default" style="font-size:small">tst_cgroup2_mem(){</div><div class="gmail_default" style="font-size:small">    tst_cgroup2_mem("no need set mem, becase v2 has only one heierarchy")</div><div class="gmail_default" style="font-size:small">}</div><div class="gmail_default" style="font-size:small">// these two API mount different cgroup hierarchy via tst_cgroup*_mount()</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">tst_cgroup_mem()</div><div class="gmail_default" style="font-size:small">{</div><div class="gmail_default" style="font-size:small">    //if v1</div><div class="gmail_default" style="font-size:small">    tst_cgroup1_mem()</div><div class="gmail_default" style="font-size:small">    // if v2</div><div class="gmail_default" style="font-size:small">    tst_cgroup2_mem()</div><div class="gmail_default" style="font-size:small">}</div><div class="gmail_default" style="font-size:small">tst_cgroup_cpu()</div><div class="gmail_default" style="font-size:small">{</div><div class="gmail_default" style="font-size:small">    // if v1</div><div class="gmail_default" style="font-size:small">    tst_cgroup1_cpuset();</div><div class="gmail_default" style="font-size:small">    // if v2</div><div class="gmail_default" style="font-size:small">    tst_cgroup2_cpuset("no cpuset, skip this invoke");</div><div class="gmail_default" style="font-size:small">}</div><div class="gmail_default" style="font-size:small">// actully, we only use tst_cgroup_*() in testcase, they as the finial API to export to</div><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>
Is the intent to provide API for both v1 and v2, or just higher level API<br>
that hides the details?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">The former. My original purpose is to provide unified APIs</div><div class="gmail_default" style="font-size:small">for both v1 and v2. Testcase author does not need to care</div><div class="gmail_default" style="font-size:small">about the difference of two versions, if he/she want to set </div><div class="gmail_default" style="font-size:small">max bytes in tests, just invoke:</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">//in setup()</div><div class="gmail_default" style="font-size:small">    tst_cgroup_mem()</div><div class="gmail_default" style="font-size:small">//in main()</div><div class="gmail_default" style="font-size:small">    <span class="gmail_default"></span>tst_cgroup_mem_set_maxbytes()</div><div class="gmail_default" style="font-size:small">//in cleanup()</div><div class="gmail_default" style="font-size:small">    tst_cgroup_umount()</div><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>
> +{<br>
> +     if (tst_is_mounted(mnt_path))<br>
> +             goto out;<br>
> +<br>
> +     SAFE_MKDIR(mnt_path, 0777);<br>
> +     if (mount(name, mnt_path, "cgroup", 0, option) == -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 v1 is not configured in kernel");<br>
> +             }<br>
<br>
We should probably handle also EBUSY, for cases when controller is already part<br>
of existing hierarchy. E.g. cpu,cpuacct is mounted together, and someone<br>
tries to mount just cpu:<br>
  mount("none", "/mnt/cgroup", "cgroup", MS_MGC_VAL, "cpu") = -1 EBUSY (Device or resource busy)<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">That's true.</div><br></div><div><div class="gmail_default" style="font-size:small">But in general, people are not permitted to use tst_cgroup*_mount() directly, it is only as the low-level/internal function to hide details we mount cgroup. My previous thought is that, in v1, cpu,cpuacct are bound together(as system way) in tst_croup_cpu().</div><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>
> +<br>
> +void tst_write_memcg1(long memsz)<br>
<br>
This should at least say memmax or something similar, if we add<br>
functions for more knobs later.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Good suggestion.</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>
I'm thinking if it would be worth having API more parametrized,<br>
something like:<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Sounds neat, we could have a try and then there</div><div class="gmail_default" style="font-size:small">is no need to export too many functions with _mem()</div><div class="gmail_default" style="font-size:small">or _cpu() suffixes.</div><br></div><div><div class="gmail_default" style="font-size:small">I'd like to merge your method with my original basic</div><div class="gmail_default" style="font-size:small">functions, it seems not very hard to achieve patch v2:</div><br></div><div><div class="gmail_default" style="font-size:small">tst_cgroup_create(enum tst_cgroup_ctrl ctrl)</div><div class="gmail_default" style="font-size:small">{</div><div class="gmail_default" style="font-size:small">    tst_cgroup_version();</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">    switch(ctrl) {</div><div class="gmail_default" style="font-size:small">    case <span class="gmail_default"></span>TST_CGROUP_MEMCG:</div><div class="gmail_default" style="font-size:small">        // if v1</div><div class="gmail_default" style="font-size:small">        tst_cgroup1_mount(TST_CGROUP_MEMCG);</div><div class="gmail_default" style="font-size:small">        // if v2</div><div class="gmail_default" style="font-size:small">        tst_cgroup2_mount()</div><div class="gmail_default" style="font-size:small">    break;</div><div class="gmail_default" style="font-size:small">    case <span class="gmail_default"></span>TST_CGROUP_CPUSET:</div><div class="gmail_default" style="font-size:small">          //if v1</div><div class="gmail_default" style="font-size:small">          tst_cgroup1_mount(TST_CGROUP_CPUSET);</div><div class="gmail_default" style="font-size:small">         // if v2</div><div class="gmail_default" style="font-size:small">         TCONF here...</div><div class="gmail_default" style="font-size:small">    break;</div><div class="gmail_default" style="font-size:small">    }</div><div class="gmail_default" style="font-size:small">}</div><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>
enum tst_cgroup_ctrl {<br>
        <span class="gmail_default" style="font-size:small"></span>TST_CGROUP_MEMCG = 1,<br>
        <span class="gmail_default" style="font-size:small"></span>TST_CGROUP_CPUSET = 2,<br>
};<br>
<br>
tst_cgroup_mount(enum tst_cgroup_ctrl)<br>
tst_cgroup_umount(enum tst_cgroup_ctrl)<br>
  // tests wouldn't need to use these ones directly<br>
  // would be probably internal functions<br>
<br>
tst_cgroup_version()<br>
  // to get/check cgroup support in setup()<br>
<br>
tst_cgroup_create(enum tst_cgroup_ctrl, const char *dir)<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Maybe we can drop the second parameter "dir", the mount</div><div class="gmail_default" style="font-size:small">functions are internal and we just use path macros in sub-function</div><div class="gmail_default" style="font-size:small">which like what I did.</div><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">
  // mounts cgroup if not already mounted<br>
  // creates "dir", sets up subtree_control, etc.<br>
<br>
tst_cgroup_cleanup()<br>
  // cleans up everything, removes dirs, umounts what was mounted<br>
<br>
tst_cgroup_move_current(enum tst_cgroup_ctrl, const char *dir)<br>
  // writes getpid() to dir/"tasks"<br>
<br>
<span class="gmail_default" style="font-size:small"></span>tst_cgroup_mem_set_maxbytes(const char *dir, long memsz)<br>
  // memcg specific function<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>