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.
> > >
> > > 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()).
>
> 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.
>
> > > 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.
If we are going to insist on try_module_get for each reference, then the
controller's exit_module() will never be called (reference will be added
for the default class which will never be removed).
The way it is implemented now, a controller can register/unregister
irrespective of the existence of classes.
>
> > 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.
All these logic follows the first step.
>
> > --------------------
> > 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.
>
> > > <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.
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.
Or do you mean that in different architectures we can expect unregister
to be called multiple times (and hence race with itself) ?
>
> 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
>
--
----------------------------------------------------------------------
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
