On Thu, 2005-09-15 at 17:42 +0900, MAEDA Naoaki wrote:
> Hi Chandra,
>
> I believe that simplifying CKRM is the right thing to do.
> At first glance, I have some questions and comments.
Hi Maeda-san,
Thanks for your review and comments. See below for my response
>
> > + * RESOURCE CONTROLLERS
> > + */
> > +/* resource controller callback structure */
> > +struct ckrm_res_ctlr {
> > + char res_name[CKRM_MAX_RES_NAME];
> > + int res_hdepth; /* maximum hierarchy */
>
> Nobody seems to care the res_hdepth member. Why?
I don't understand what do you mean here. By nobody, you mean resource
controllers ? If so, memory controller uses it.
> > +int max_resid; /* max resid used */
> > +atomic_t nr_resusers[CKRM_MAX_RES_CTLRS];
> > +struct ckrm_res_ctlr *res_ctlrs[CKRM_MAX_RES_CTLRS];
>
> Since these symbols, max_resid, nr_resusers and res_ctrlrs
> are globally exported in kernel, some prefix such as "ckrm_"
> should be added to these symbols to avoid future naming conflict.
Will fix.
> > +LIST_HEAD(classes);/* link all classes */
> > +int num_classes;
> > +rwlock_t ckrm_class_lock = RW_LOCK_UNLOCKED; /* protects classlists */
>
> These symbols, classes, num_classes and ckrm_class_lock should be
> declared as static, because only this file uses these symbols.
will fix appropriately.
> return;
> > +
> > + atomic_inc(&nr_resusers[resid]);
>
> I do not understand why nr_resusers[resid] is incremented here.
> Is it intentional?
Looking at it closely, realize that it is not needed(as we already are
holding a reference). will fix
>
> > + if (resid < CKRM_MAX_RES_CTLRS) {
> > + if (res_ctlrs[resid] == NULL) {
> > + res_ctlrs[resid] = rcbs;
> > + atomic_set(&nr_resusers[resid], 0);
> > + set_bit(resid, &bit_res_ctlrs);
> > + ret = resid;
> > + if (resid >= max_resid)
> > + max_resid = resid + 1;
> > + pr_debug("Registered resource: resid:%d name:%s\n",
> > + resid, res_ctlrs[resid]->res_name);
> > + } else
> > + ret = -EBUSY;
> > + }
> > + spin_unlock(&res_ctlrs_lock);
> > + return (-ENOMEM);
>
> Shouldn't be "return ret;" ? Otherwise calling this function
> is never success.
Yes. I moved the code around and missed to return ret. will fix.
>
> + */
> > +struct ckrm_class {
> > + void *res_class[CKRM_MAX_RES_CTLRS]; /* resource classes */
> > +
> > + struct list_head tasklist; /* this class's tasks */
> > + spinlock_t class_lock; /* protects list,array above */
> > +
> > + struct list_head clslist; /* peer classtype classes */
>
> What is the purpose of clslist member and peer classtype mean?
> Isn't it a former classtype specific member?
Yes, it is. will remove.
>
----------------------------------------------------------------------
Chandra Seetharaman | Be careful what you choose....
- [EMAIL PROTECTED] | .......you may get it.
----------------------------------------------------------------------
-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
ckrm-tech mailing list
https://lists.sourceforge.net/lists/listinfo/ckrm-tech
