On Tue, 2006-04-18 at 13:57 -0700, Matt Helsley wrote:
> > > > +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
not necessarily. In this function all you know is somebody provided you
with the controller data structure.
Looking at the code, realized that the single usage of this function can
be replaced with the _by_id counterpart, and it looks more readable too
(in the context).
> 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.
Yeah, i didn't answer this question. The object of having the reference
count outside the controller data structure is to make sure the
controller module exists when the reference to the controller is held.
Consider this situation with using kref:
processor 1 processor 2
user issues read stats
controller[i] reference acquired
controller i unregisters
module removed (all code removed)
controller data structure remains
since we hold a reference
function pointers in the data struture
point to random addresses
tries to access controller->show_stats --> results in page fault
--------------------
The way it is now, the controller_unregister function would 've failed
with -EBUSY (which would make the controller to retry the operation),
and the unregister would succeed only when there is no temporary
references held.
>
> <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).
This _is_ the unregister function, and i seriously doubt more than one
controller with the _same_ controller id race to unregister the
controller. Hence there is no lock around it.
>
>
> 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
