Re: [RFC 1/2] core: Add generic object registry implementation

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2860 bytes --]

On Tue, Nov 04, 2014 at 08:38:45AM -0800, Greg Kroah-Hartman wrote:
> On Tue, Nov 04, 2014 at 05:29:27PM +0100, Thierry Reding wrote:
[...]
> > diff --git a/drivers/base/registry.c b/drivers/base/registry.c
[...]
> > +/**
> > + * registry_record_ref - reference on the registry record
> > + * @record: record to reference
> > + *
> > + * Increases the reference count on the record and returns a pointer to it.
> > + *
> > + * Return: A pointer to the record on success or NULL on failure.
> > + */
> > +struct registry_record *registry_record_ref(struct registry_record *record)
> > +{
> > +	if (!record)
> > +		return NULL;
> > +
> > +	/*
> > +	 * Refuse to give out any more references if the module owning the
> > +	 * record is being removed.
> > +	 */
> > +	if (!try_module_get(record->owner))
> > +		return NULL;
> > +
> > +	kref_get(&record->kref);
> 
> You "protect" from a module being removed, but not from someone else
> releasing the kref at the same time.  Where is the lock that prevents
> this from happening?

You're right, we need a record lock to serialize ref/unref.

> And do we really care about module reference counts here?

We need this so that the code of the .release() callback stays in
memory as long as there are any references.

Also we need the module reference count for registries because they
would typically be statically allocated and go away along with a module
when it is unloaded.

> > +/**
> > + * registry_add - add a record to a registry
> > + * @registry: registry to add the record to
> > + * @record: record to add
> > + *
> > + * Tries to increase the reference count of the module owning the registry. If
> > + * successful adds the new record to the registry.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int registry_add(struct registry *registry, struct registry_record *record)
> > +{
> > +	if (!try_module_get(registry->owner))
> > +		return -ENODEV;
> > +
> > +	mutex_lock(&registry->lock);
> > +	list_add_tail(&record->list, &registry->list);
> > +	mutex_unlock(&registry->lock);
> 
> No incrementing of the reference of the record at all?

I'm not sure if we really need one. Drivers will have to remove the
record from the registry before dropping their reference. I guess we
could throw in another kref_get() here (and a kref_put() in
registry_remove()) for good measure to prevent breakage with buggy
drivers.

> You seem to be using 2 reference counts for the record / registry, a
> module count, and a kref count, which can cause confusion...

There is one reference count (kref actually) per registry record and one
module count for the record owner and the registry owner.

Can you elaborate what you think is confusing? Perhaps I can add more
kerneldoc to clarify.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]