<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 2, 2020 at 8:12 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">Hi Li,<br>
<br>
>Why we need this? Because, if a testcase(i.e oom05.c) needs more than one<br>
>cgroup<br>
>subsystem(memory, cpuset) on RHEL7(cgroup-v1), it should mount two<br>
>different<br>
>directories and do some knob setting.<br>
<br>
Mounting with different controllers is fine, I meant do we have a case for mounting<br>
same controller multiple times? We might have, because current design allows<br>
only for single directory (tst_cgroup_new_path), that's automatically created on mount.<br>
(This is your example 4)<br>
<br>
><br>
><br>
>><br>
>> > +<br>
>> > +static void tst_cgroup_set_path(const char *cgroup_dir)<br>
>> > +{<br>
>> > + struct tst_cgroup_path *tst_cgroup_path, *a;<br>
>> > +<br>
>> > + if (!cgroup_dir)<br>
>> > + tst_brk(TBROK, "Invalid cgroup dir, plese check<br>
>> cgroup_dir");<br>
>> > +<br>
>> > + sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);<br>
>> > + sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());<br>
>> > +<br>
>> > + /* To store cgroup path in the shared 'path' list */<br>
>> > + tst_cgroup_path = SAFE_MMAP(NULL, (sizeof(struct tst_cgroup_path)),<br>
>> > + PROT_READ | PROT_WRITE, MAP_ANONYMOUS |<br>
>> MAP_SHARED, -1, 0);<br>
>><br>
>> I'm not sure I understand what is the reason to have tst_cgroup_path. Is<br>
>> it expected,<br>
>> that mount and umount are called by different processes? It might be easier<br>
>><br>
><br>
>The shared 'tst_cgroup_path' is necessary especially for mounting<br>
>different cgoups in setup(). Otherwise, it would be easy to get lost<br>
>which directory for kind of cgroup type.<br>
<br>
But why is it shared? Is cleanup going to run in different process context?<br>
Which one of your examples needs shared memory?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">My original thought was that if children want to remove some paths</div><div class="gmail_default" style="font-size:small">from the list, so this shared memory was made sense. But in the</div><div class="gmail_default" style="font-size:small">signed-off patch v2, it seems we only do the deletion in cleanup(),</div><div class="gmail_default" style="font-size:small">so private memory is enough.</div><br></div><div><div class="gmail_default" style="font-size:small">Another slight reason to keep using share memory is to avoid the</div><div class="gmail_default" style="font-size:small">children have a new copy of the list, that will allocate more memory</div><div class="gmail_default" style="font-size:small">in testing, but that's fine, I have no objection to using private memory.</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>
>And the worth to say, the random directory name for same cgroup<br>
>mounting is also on purpose, though we mount same(i.e memory)<br>
>cgroup in two places it still belongs to the one hierarchy, and create<br>
>the same name of the new directory will be hit an error in EEXIST.<br>
><br>
>static void tst_cgroup_set_path(const char *cgroup_dir)<br>
>{<br>
> ...<br>
> sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);<br>
> sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());<br>
<br>
I see why you are tracking this in list, but this exchange of state through<br>
global variables does seem bit unclear.<br></blockquote><div><span class="gmail_default" style="font-size:small"><br></span></div><div><span class="gmail_default" style="font-size:small">Yes, that's a compromise to support more usage of the APIs.</span></div><div><span class="gmail_default" style="font-size:small"></span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Could we leave "new_path" creation to testcase itself? It would give<br>
us more flexibility and we don't have to worry about name collisions.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Hmm, more flexibility sometimes means more complicated to use, I don't</div><div class="gmail_default" style="font-size:small">want these APIs to become difficult for newbies of LTP.</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">
It also avoids need to mount same controller multiple times<br>
(example 4 in your reply).<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">To be honest, most situations we have are examples 1 and 2, the</div><div class="gmail_default" style="font-size:small">others(ex3,4,5) might rarely happen in our test. So maybe leave too</div><div class="gmail_default" style="font-size:small">much work(i.e "new_path" creation) in testcase is not wise, and I don't</div><div class="gmail_default" style="font-size:small">want to make usage complicated just for ingratiating the rare situations.</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>
Let's assume this is API:<br>
<br>
#include "tst_cgroup.h"<br>
#define MEM_MNT "/tmp/cgroup1"<br>
#define CPUSET_MNT "/tmp/cgroup2"<br>
#define DIR1 "ltp_test_blah_dir1"<br>
#define DIR2 "ltp_test_blah_dir2"<br>
#define DIR3 "ltp_test_blah_dir3"<br>
<br>
static void run(void)<br>
{<br>
if (fork() == 0) {<br>
tst_cgroup_move_current(MEM_MNT, DIR2);<br>
// do your test<br>
exit(0);<br>
}<br>
tst_cgroup_move_current(MEM_MNT, DIR1);<br>
// do your test<br>
}<br>
<br>
static void setup(void)<br>
{<br>
tst_cgroup_mount(TST_CGROUP_MEMCG, MEM_MNT);<br>
tst_cgroup_mkdir(MEM_MNT, DIR1);<br>
tst_cgroup_mem_set_maxbytes(MEM_MNT, DIR1, 1*1024*1024);<br>
tst_cgroup_mkdir(MEM_MNT, DIR2);<br>
tst_cgroup_mem_set_maxbytes(MEM_MNT, DIR2, 2*1024*1024);<br>
<br>
tst_cgroup_mount(TST_CGROUP_CPUSET, CPUSET_MNT);<br>
tst_cgroup_mkdir(CPUSET_MNT, DIR3);<br>
tst_cgroup_move_current(CPUSET_MNT, DIR3);</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
}<br>
<br>
static void cleanup(void)<br>
{<br>
tst_cgroup_umount(MEM_MNT);<br>
tst_cgroup_umount(CPUSET_MNT);<br>
}<br>
<br>
static struct tst_test test = {<br>
...<br>
.test_all = run,<br>
};<br>
<br>
On library side we would have a list that tracks all mounts. And every mount<br>
record would have list that tracks all mkdir() operations, so we can<br>
cleanup anything that test creates. I think tracking per-process would be sufficient.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">To compare with my v2, this design leaves simple code in lib, and flexible</div><div class="gmail_default" style="font-size:small">in usage. The only disadvantage is that people need to define more macros</div><div class="gmail_default" style="font-size:small">and mkdir() by themself. </div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">I have no strong preference to shift from v2 to this method, or maybe we can try</div><div class="gmail_default" style="font-size:small">to combine together and do more optimize work in the lib side.</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>
(without spinning v3) What are your thoughts?<br>
<br>
Regards,<br>
Jan<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>