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.
Also, ckrm_get_controller() is missing. I could see providing that to
controllers along with ckrm_put_controller().
I think the plan ckrm_get|put_controller() pair belongs in ckrm_rc.h and
not ckrm.h.
<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.
<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.
> +/* 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 ;)
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.
> +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.
<snip>
Cheers,
-Matt Helsley
-------------------------------------------------------
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
