<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small">Hi,</div><div class="gmail_default" style="font-size:small"><br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>> -enum tst_cgroup_ctrl {<br>
>> -    TST_CGROUP_MEMCG = 1,<br>
>> +/* Controller sub-systems */<br>
>> +enum tst_cgroup_css {<br>
>> +    TST_CGROUP_MEMORY = 1,<br>
>>      TST_CGROUP_CPUSET = 2,<br>
>> -    /* add cgroup controller */<br>
>>  };<br>
><br>
> I spend a bit of time figuring out what css means, can't we just use<br>
> controler in the code instead? It's a bit longer but more obvious.<br>
><br>
> Or is this consitent with kernel naming?<br>
<br>
The kernel seems to refer to controllers as subsystems and css appears<br>
to mean "CGroup subsystem". I'm not sure if subsystems are necessarily<br>
controllers, but it seems that way. Also "controller" has too many<br>
letters in it. Indeed it makes some function names very long and I found<br>
that harder to read.</blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">I also feel css looks oddly, if "controller" is too long we can go back with "ctrl",</div><div class="gmail_default" style="font-size:small">then we might need a one-line comment for the name abbreviation.</div></div><div class="gmail_default" style="font-size:small"><br></div><div><div class="gmail_default" style="font-size:small">e.g.  previous "/* add cgroup controller */" is make no sense but a hint for people</div><div class="gmail_default" style="font-size:small">to know this is cgroup controller and could be extended in the future.</div></div><div><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>
I suppose to be totally consistent with the kernel we should randomly<br>
name some things css and others subsys. :-p<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Not very necessary, this test API is facing for userland, understanding easily</div><div class="gmail_default" style="font-size:small">for users is more important I think.</div></div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>> +struct tst_cgroup_opts {<br>
>> +    /* Only try to mount V1 CGroup controllers. This will not<br>
>> +     * prevent V2 from being used if it is already mounted, it<br>
>> +     * only indicates that we should mount V1 controllers if<br>
>> +     * nothing is present. By default we try to mount V2 first. */<br>
>> +    int only_mount_v1:1;<br>
>> +};<br>
><br>
> Do we need to pass the flags in a structure?<br>
><br>
> This is not an API that has to be future proof, when we find out we need<br>
> more than a few bitflags we can always change it.<br>
<br>
We may need to add xattr to this and there are many other<br>
options. However most tests will just have NULL or 0, 0... So we could<br>
move it into a vararg or have a tst_require2?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">My 2cent: </div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">I slightly think tst_cgroup_opts is good stuff, it gives a possibility</div><div class="gmail_default" style="font-size:small">to let users hook the cgoup mount way or, more options they</div><div class="gmail_default" style="font-size:small">needed (maybe for customized users). And, in most situations</div><div class="gmail_default" style="font-size:small">that test just has NULL, that's fine.</div><br></div><div><div class="gmail_default" style="font-size:small">@Richard,</div><div class="gmail_default" style="font-size:small">do you want to reserve the ‘no_cleanup‘ in this structure as well?</div><div class="gmail_default" style="font-size:small">I noticed you leave it in tst_cgroup02 test.</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">
>> +    struct tst_cgroup_tree *trees[TST_CGROUP_MAX_TREES + 1];<br>
>> +};<br>
><br>
> So this is an array of directories in different trees, can we please<br>
> name the strucuture better. What about tst_cgroup_nodes or<br>
> tst_cgroup_dirs?<br>
<br>
I guess tst_cgroup_tree always represents a directory. So I would go<br>
with tst_cgroup_dir.<br></blockquote><div><br></div><div class="gmail_default" style="font-size:small">+1</div><div class="gmail_default" style="font-size:small"></div><div class="gmail_default" style="font-size:small"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>> +struct tst_cgroup_tree {<br>
><br>
> Why isn't this called node or dir? Either of these would be more<br>
> fitting.<br>
><br>
> Also the tst_cgroup structure could use a better name, the tst_cgroup is<br>
> actually an array of pointers to directories.<br>
<br>
As far as the test author is concerned it represents an actual "Control<br>
Group". We keep arrays of directories to join together multiple V1<br>
controllers on different hierarchies, but that is an implementation<br>
detail.<br>
<br>
If anything it should be called tst_cgroup_group, but I don't like the<br>
repetition. :-p<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Personally, I think tst_cgroup_node may be better since we have described</div><div class="gmail_default" style="font-size:small">a tree for cgroup.  root, node, and leaves(files) should be more vivid.</div><br></div><div><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>
>> +    /* In general we avoid having sprintfs everywhere and use<br>
>> +     * openat, linkat, etc.<br>
>> +     */<br>
>> +    int dir;<br>
><br>
> Can we name this dir_fd so it's obvious what it is?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">+1</div><div class="gmail_default" style="font-size:small">I stand by Cyril at this point.</div></div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>> +/* Lookup tree for item names. */<br>
>> +typedef struct cgroup_item items_t[];<br>
>> +static items_t items = {<br>
>> +    [0] = { "cgroup", .inner = (items_t){<br>
>> +                    { "cgroup.procs", "tasks" },<br>
>> +                    { "cgroup.subtree_control" },<br>
>> +                    { "cgroup.clone_children", "clone_children" },<br>
>> +                    { NULL }<br>
>>              }<br>
>> -    }<br>
>> -    SAFE_FCLOSE(file);<br>
>> +    },<br>
>> +    [TST_CGROUP_MEMORY] = { "memory", .inner = (items_t){<br>
>> +                    { "memory.current", "memory.usage_in_bytes" },<br>
>> +                    { "memory.max", "memory.limit_in_bytes" },<br>
>> +                    { "memory.swappiness", "memory.swappiness" },<br>
>> +                    { "memory.swap.current", "memory.memsw.usage_in_bytes" },<br>
>> +                    { "memory.swap.max", "memory.memsw.limit_in_bytes" },<br>
>> +                    { "memory.kmem.usage_in_bytes", "memory.kmem.usage_in_bytes" },<br>
>> +                    { "memory.kmem.limit_in_bytes", "memory.kmem.usage_in_bytes" },<br>
>> +                    { NULL }<br>
>> +            },<br>
>> +      .css_indx = TST_CGROUP_MEMORY<br>
>> +    },<br>
>> +    [TST_CGROUP_CPUSET] = { "cpuset", .inner = (items_t){<br>
>> +                    { "cpuset.cpus", "cpuset.cpus" },<br>
>> +                    { "cpuset.mems", "cpuset.mems" },<br>
>> +                    { "cpuset.memory_migrate", "cpuset.memory_migrate" },<br>
>> +                    { NULL }<br>
>> +            },<br>
>> +      .css_indx = TST_CGROUP_CPUSET<br>
>> +    },<br>
>> +    { NULL }<br>
>> +};<br>
><br>
> Item is a very generic word, this is a list of known knobs and map<br>
> between v1 and v2. So maybe cgroup_knob_map or just cgroup_knobs ?<br>
<br>
I suppose that cgroup_item can be seperated into just two structs now<br>
that I removed controller sub items like "memory.swap". This might help<br>
solve the missing initializer warnings as well. </blockquote><div><br></div><div class="gmail_default" style="font-size:small">To separate into two structs sounds good, the 'cgroup_item' contains both</div><div class="gmail_default" style="font-size:small">trunk node and controller files make things mess up.</div><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">
<br>
I would not describe "memory.current" as a knob. It is a read only file<br>
and I think of knobs as something you can write to. So I would prefer<br>
cgroup_file and cgroup_subsys.</blockquote><div> </div><div><div class="gmail_default" style="font-size:small">+1</div><div class="gmail_default" style="font-size:small">cgroup_file and cgroup_subsys(or cgroup_controller) all very nice.</div></div><div> </div></div>-- <br><div dir="ltr"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>