On Mon, 2006-01-16 at 18:13 -0800, Dave Hansen wrote:
> On Tue, 2006-01-17 at 10:57 +0900, KUROSAWA Takahiro wrote:
> > The pzone (pseudo zone) based memory resource controller is yet
> > another implementation of the CKRM memory resource controller.
> > The existing CKRM memory resource controller counts the number of
> > pages that are allocated for tasks in a class in order to guarantee
> > and limit memory resources. This requires changes to the existing
> > code for page allocation and page reclaim.
>
> This is an interesting approach, and it may have some potential. First
> of all, to get better reviews, I'd suggest that you post your patches
> LKML-style: one per email.
I agree with Dave. Preferrably inlined and not base64-encoded please. It
would have made the feedback below easier to understand.
That said I'm pleased to see an alternative approach! I've included my
comments below.
Cheers,
-Matt Helsley
In the second patch (pz01-memrc-pzone.patch) you have:
+struct ckrm_mem {
+ struct ckrm_class *class; /* the class I belong to */
+ struct ckrm_class *parent; /* parent of the class above. */
Couldn't you remove the parent field and replace the uses of res->parent
with res->class->parent?
I noticed in mem_res_alloc(), which appears to be the only caller of
mem_res_initcls_one(), that the initialization of the spinlock is
outside of the mem_res_initcls_one() function. Perhaps it should go
inside the init function.
In mem_res_initcls_one(), is there a function to initialize a struct
ckrm_shares to DONTCARE values? If not, perhaps there should be.
Why are you dealing with void* so often? Could some of these be
switched to something safer? Are the casts from void* to other types of
pointers necessary?
Why is the EXPORT_SYMBOL(mem_rc_get) necessary? I can't see where it's
used outside of mm/mem_rc_pzone.c.
In recalc_and_propagate() the very end of the function, specifically:
+ /* propagate to children */
+ ckrm_lock_hier(res->class);
+ while ((child = ckrm_get_next_child(res->class, child)) != NULL) {
+ childres =
+ ckrm_get_res_class(child, rcbs.resid, struct ckrm_mem);
+ if (childres) {
+ spin_lock(&childres->cnt_lock);
+ recalc_and_propagate(childres);
+ spin_unlock(&childres->cnt_lock);
+ }
+ }
+ ckrm_unlock_hier(res->class);
is exactly the same as the corresponding function in the numtasks
controller. Perhaps this could be factored out into a separate patch so
that multiple resource controllers could use it.
Also, I noticed that mem_rc_set_guar() is only called from within
mem_rc_set_guarantee() -- perhaps it could be directly substituted into
mem_rc_set_guarantee(). If not, I suggest more distinctive naming of
these two functions.
-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems? Stop! Download the new AJAX search engine that makes
searching your log files as easy as surfing the web. DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642
_______________________________________________
ckrm-tech mailing list
https://lists.sourceforge.net/lists/listinfo/ckrm-tech
