On Tue, 2006-04-18 at 14:39 -0700, Chandra Seetharaman wrote:
> 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).
If that's the one I replaced with ckrm_get_controller() then on the
contrary, I think the opposite is the case.
Passing a controller pointer to a function indicates that the
controller will not be unregistered because the caller holds a reference
to the controller.
Passing an id of a controller indicates that the caller will have to
lookup the entry in the controller array, test if it is registered
(non-NULL), etc.
So passing an id when you already have a reference to the controller
simply results in additional work (including use of the res_ctlr_lock)
and impies uncertainty about the reference to the controller when in
fact no such uncertainty exists.
> > 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.
Having the reference count outside the controller doesn't ensure that
the controller module is not removed while the reference to the
controller is held. It only makes sure that if there's a bug that we
won't oops (or worse) when accessing the reference count.
> Consider this situation with using kref:
>
> processor 1 processor 2
> user issues read stats
> controller[i] reference acquired
> controller i unregisters
It can't unregister because it's reference count is non-zero.
> module removed (all code removed)
It can't be removed because it can't unregister -- can't module removebe
made to fail?
> controller data structure remains
> since we hold a reference
Yes, as it should.
> function pointers in the data struture
> point to random addresses
Not random addresses -- addresses in the module that still hasn't been
removed.
> tries to access controller->show_stats --> results in page fault
Nope.
> --------------------
> 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.
You can fail with -EBUSY when the reference count is in the controller
structure too. Only when it drops to 0 (i.e. release is called) do you
drop the reference to the module containing the code/controller struct,
and only then can it be safely removed.
> > <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.
Memory models and code reordering vary enough between archs that you
can't assume consistency just because it looks like it will be compiled
into an "atomic" instruction. You need to explicitly tell the compiler
that consistency is needed.
Incidentally, this is one reason I don't like passing ids around
instead of controller pointers -- it encourages anyone reading the code
to think they can/should index the array directly. By focusing it to
ckrm_get_controller* functions and inside ckrm_get_controller_shares()
we emphasize that the id should rarely, if ever, be used outside the
core CKRM code (and even then sparingly).
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
