Re: [PATCH] driver core: remove unneeded klist methods

[Alan Stern <stern@rowland.harvard.edu>]

On Fri, 20 Jan 2006, James Bottomley wrote:

> On Fri, 2006-01-20 at 11:39 -0500, Alan Stern wrote:
> > This patch (as641) removes unneeded klist methods from the driver core and
> > changes a klist_del call to klist_remove in device_del.
> > 
> > The _get and _put methods have no effect, because the klist nodes are
> > deleted by calling klist_remove, which waits until they are unreferenced
> > by any klist iterators.  Furthermore, the _puts cause problems because
> > they occur while the iterator is holding a spinlock.
> 
> Could you just elaborate on the actual problem that you're trying to
> solve here?

The problem is that put_device must not be called while holding a
spinlock.  This has always been true, but we only started noticing it
recently when Greg added a might_sleep.  Your klist method would call
put_device while holding the klist's spinlock.

> Iterators of volatile lists are ipso facto just "best guess", so moving
> the location of the iterator piece is OK.

Good, I thought so.

>  However, your assumption that
> only the routine called by the iterator is always going to do the final
> put on the object is fundamentally flawed.

That was not my assumption.  My assumption was that the routine called by
the iterator will _sometimes_ do the final put on the object.  I don't
know if that actually ever happens, but the possibility certainly exists.  
For example, there are places where a device_for_each_child iteration is
used for calling device_unregister on all the children.

>  The list is volatile because
> references to the object are being acquired and released all the time.
> Additionally, the list nodes are embedded in the object, so by removing
> the object get and put calls, you remove the ability for the object
> refcounting to see the fact that the list is using the object.

Agreed.  Note, however, that the patch doesn't do this for all objects -- 
only for struct device.

>  This
> will lead to the situation where the object could be freed while the
> iterator is acting on it.  You cannot remove the object get/put calls
> from the klist references otherwise the list refcounting will be totally
> divorced from the object refcounting.

Not so.  A device structure can't be freed before device_del returns, and
the patch makes device_del call klist_remove instead of klist_del.  The
difference between the two is that klist_remove blocks until all iterators
have finished using the klist node.  New iterators can't start using it
because the routine removes the node from the klist.

So by the time device_del returns, we are guaranteed that the embedded 
klist node is not in use and will never be used.  Thus releasing the 
device structure is safe.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/