On Tue, 2006-04-18 at 13:39 -0700, Chandra Seetharaman wrote:
> 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>
> 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.
Hmm, ok. I can see a potential use for it, but it's not very compelling.
<snip>
> > > +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.
I asked around and discovered I was wrong :).
> Agree that this function warrants a header that tells that the lock need
> to be held on entry.
OK, sounds good.
> > <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.
The fact that we already have gotten the reference to it indicates that
it is in the array. The only other way anyone outside core could get a
pointer to a controller is if they registered it -- and in that case I
think calling get on it is a valid operation. Note that only if we're
given the id as a parameter do we really need to check if the controller
is in the array -- because then we're not guaranteed to have a reference
to the entry.
Also, I think moving the ref count into the controller structure makes
this even clearer.
<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 ?
Hmm. Well we still need to hold the lock when indexing the array. I'd
rather use the function and release the reference at the end than use
the lock in this function directly (this idea melds nicely with the
suggestion of add/remove_controller functions).
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
