On Mon, 2006-04-17 at 18:11 -0700, Matt Helsley wrote:
> On Fri, 2006-04-14 at 20:23 -0700, [EMAIL PROTECTED] wrote:
> <snip>
>
> > +extern struct ckrm_controller *ckrm_get_controller_by_name(const char *);
> > +extern struct ckrm_controller *ckrm_get_controller_by_id(unsigned int);
> > +extern void ckrm_put_controller(struct ckrm_controller *);
>
> Hmm, perhaps extern declarations of ckrm_get_controller_by_foo() belongs
> in rcfs.c and nowhere else. I'm not completely sure, but in general they
> shouldn't be exposed to controllers themselves.
I think the usual practice is to put in the header file that belong to
the module that exports the function (and not the one that uses it).
>
> Also, ckrm_get_controller() is missing. I could see providing that to
> controllers along with ckrm_put_controller().
ckrm_get_controller() is not needed outside of ckrm.c. so, i do not
think it needs to be exported. I don't want somebody outside of core to
be holding a controller data structure without a reference(providing
ckrm_get_controller(struct ckrm_controller *) does imply that).
ckrm_get_controller_by_foo() returns a controller data structure, so we
just have a common ckrm_put_controller() function. Not necessarily,
>
> I think the plan ckrm_get|put_controller() pair belongs in ckrm_rc.h and
> not ckrm.h.
I do not see why resource controllers need to use get/put. These
interface are available mainly to make sure that the controller doesn't
disappear when _somebody else_ is accessing the controller data
structure.
>
> <snip>
>
> > +extern int ckrm_register_controller(struct ckrm_controller *);
> > +extern int ckrm_unregister_controller(struct ckrm_controller *);
> > +#endif /* _LINUX_CKRM_RC_H */
> > Index: linux-2.6.16/kernel/ckrm/ckrm.c
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6.16/kernel/ckrm/ckrm.c
> <snip>
>
> > +static struct ckrm_controller *ckrm_controllers[CKRM_MAX_RES_CTLRS];
> > +static atomic_t ckrm_ctlr_refcount[CKRM_MAX_RES_CTLRS];
>
> It would be nice if we could move the refcount into the controller
> structure as a struct kref (along with a reference to the module that
> owns the controller..).
>
> > +/* res_ctlrs_lock protects ckrm_controllers and ckrm_ctlr_refcount arrays
> > */
> > +static spinlock_t res_ctlrs_lock = SPIN_LOCK_UNLOCKED;
> > +
> > +static LIST_HEAD(ckrm_classes);/* link all classes */
> > +static rwlock_t ckrm_class_lock = RW_LOCK_UNLOCKED; /* protects
> > ckrm_classes */
> > +
> > +static inline int is_ctlr_id_valid(unsigned int ctlr_id)
> > +{
> > + return ((ctlr_id < CKRM_MAX_RES_CTLRS) &&
> > + (ckrm_controllers[ctlr_id] != NULL));
> > +}
>
> Perhaps __ as a prefix would indicate that callers must hold the
> res_ctlrs_lock.
I think __ is mainly used for showing it is an internal function and not
to be used externally, nothing w.r.t lock.
Agree that this function warrants a header that tells that the lock need
to be held on entry.
>
> <snip>
>
> > +void ckrm_get_controller(struct ckrm_controller *ctlr)
> > +{
> > + struct ckrm_controller *tmp = ckrm_get_controller_by_id(ctlr->ctlr_id);
> > + BUG_ON(tmp != ctlr);
> > +}
>
> Why isn't this just an atomic_inc()? If you've already got the
> controller pointer then you have at least one reference to it already --
> no need to grab the controllers lock to get another reference.
we want to make sure that the controller _is_ in our array.
>
> > +/* Allocate resource specific information for a class */
> > +static void do_alloc_shares_struct(struct ckrm_class *class,
> > + struct ckrm_controller *ctlr)
> > +{
> > + if (class->shares[ctlr->ctlr_id]) /* already allocated */
> > + return;
> > +
> > + if (class->depth > ctlr->depth_supported)
> > + return;
> > +
> > + class->shares[ctlr->ctlr_id] = ctlr->alloc_shares_struct(class);
> > + if (class->shares[ctlr->ctlr_id] != NULL)
> > + ckrm_get_controller(ctlr);
>
> Harping on the ckrm_get_controller() again :) -- we don't need the lock
> for the ckrm_get_controller() call since we already have one reference
> to the controller.
>
> <snip>
>
> > +/*
> > + * Interfaces for resource controllers
> > + */
>
> This comment doesn't really tell us much. Why not drop it? Besides,
> ckrm_register_controller_internal() is static ;)
agreed.
>
> Incidentally ckrm_register_controller_internal() could use a better name
> -- sure it's used in the register path, but what does it do that makes
> it logical to have as a separate function? I'd suggest add_controller().
> It may also make sense to have a corresponding remove_controller()
> called by unregister since folks are used to seeing pairs like this.
sounds valid. can do
>
> > +static int ckrm_register_controller_internal(struct ckrm_controller *ctlr)
> > +{
> > + int ctlr_id, ret = -ENOSPC;
> > +
> > + spin_lock(&res_ctlrs_lock);
> > + for (ctlr_id = 0; ctlr_id < CKRM_MAX_RES_CTLRS; ctlr_id++)
> > + if (ckrm_controllers[ctlr_id] == NULL) {
> > + ckrm_controllers[ctlr_id] = ctlr;
> > + ret = ctlr_id;
> > + break;
> > + }
> > + spin_unlock(&res_ctlrs_lock);
> > + return ret;
> > +}
>
> <snip>
>
> > +/*
> > + * Unregistering resource controller.
> > + *
> > + * Returns 0 on success -errno for failure.
> > + */
> > +int ckrm_unregister_controller(struct ckrm_controller *ctlr)
> > +{
> > + struct ckrm_class *class;
> > + unsigned int ctlr_id;
> > +
> > + if (!ctlr)
> > + return -EINVAL;
> > + ctlr_id = ctlr->ctlr_id;
> > +
> > + if (!is_ctlr_id_valid(ctlr_id) || (ckrm_controllers[ctlr_id] != ctlr))
> > + return -EINVAL;
>
> It occurs to me that if (ckrm_get_controller_by_id(ctlr_id) != ctlr)
> could replace this entire check since it checks for a valid id and
> correctly synchronizes access to the array.
I did think about it... here is the reason i didn't make it that way: If
we have a get, then we have to have a put. if we have it in unregister,
then to be consistent, we have to add a get/put in register, when all
four of these are not really necessary (in this context). convincing ?
>
> <snip>
>
> Cheers,
> -Matt Helsley
>
--
----------------------------------------------------------------------
Chandra Seetharaman | Be careful what you choose....
- [EMAIL PROTECTED] | .......you may get it.
----------------------------------------------------------------------
-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
ckrm-tech mailing list
https://lists.sourceforge.net/lists/listinfo/ckrm-tech
