On Tue, 2006-04-18 at 18:21 -0700, Chandra Seetharaman wrote:
> On Tue, 2006-04-18 at 16:40 -0700, Matt Helsley wrote:
> > 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.
>
> one more reason i implemented it this way is to have atomic_inc and dec
> at only one place and be consistent (atomic_inc() under lock) for
> readers.
This increases reviewer confusion about whether using the lock is even
necessary here. If they then pop their analysis stack and see a caller
that's not using the locks they might think the code is buggy. IMHO it's
better to add the 4-line function and be clear about the locks involved
and when they are needed.
<snip>
> > > > 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.
>
> I am talking w.r.t the lower level functionality ckrm_get_controller()
> itself, whereas i think you are referring to the higher level callers
> (like ckrm_alloc_class()).
I don't think either level is clearer by passing it back through
get_controller_by_id() when you already have 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.
>
> Only place where ckrm_get_controller() is called is from
> do_alloc_shares_struct(), which is not performance critical and the lock
> that is being acquired is also not contended.
Why do something complex -- grab locks -- when you don't have to? It
sends the wrong message to anyone analyzing the code.
> > > > 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.
>
> how is that ? how is a kref_t being non-zero will stop any module from
> being rmmoded ?
> If you are talking that the module should always try_module_get() in its
> kref(), then we are not talking about kref anymore.
Not kref -- the module use count (which may or may not be a kref, but is
still not the controller's ref count). There are *two* refcounts to be
concerned with here:
The module refcount (maintained by try_module_get() and friends)
The controller refcount (maintained by CKRM)
<snip>
> > > 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.
>
> hmm.. this might work. not kref (due to issues mentioned above), but
> atomic_t.
I think kref will work too, but try it with atomic_t first.
> > > > <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.
I still think the above argument is correct.
> What i meant was the same controller cannot race with _itself_ in
> unregister, as it won't be calling the unregister _more than once_ in
> the first place.
However, I'm out of energy for arguing this one.
Cheers,
-Matt
-------------------------------------------------------
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
