Re: [PATCH 0/2] sg: fix races during device removal (v2)

[Tony Battersby <tonyb@cybernetics.com>]

Stefan Richter wrote:
> Tony Battersby wrote:
>   
>> Stefan Richter wrote:
>>     
>>> I believe your kref_get_not_zero() invention is because you want to
>>> count two unrelated numbers in the same counter.  This won't work, I'm
>>> afraid.
>>>  
>>>       
>> It does work actually, just in a way that people don't seem to like very
>> much.
>>     
>
> No, it doesn't work.  You can track how many transactions are pending,
> and you can track how many sites look at memory X, but you can't track
> both issues in the same counter.
>
> If you only count pending transactions, you know when to deregister the
> device from the idr.  But you don't know when it's OK to free the
> device's memory.
>
> If you cont only references to the memory, you know when it is OK to
> free it but you don't know when to deregister from the idr.
>
>   

It does work because kref_get_not_zero() must be called while holding a
lock that prevents the destructor from freeing the memory (this
requirement is in the comments that I put above the function).  If
kref_get_not_zero() returns false, then the caller forgets that it found
the object, drops the lock, and lets the destructor continue.  It is
safe from all races that I can see if used properly.  In my opinion, the
only legitimate objection that one could have is if the API is too hard
to understand and use correctly, thereby risking misuse or confusion by
people who don't understand it.

Let me give some examples:

* Example 1 *

CPU 1:
kref_put(): refcount 1 -> 0

CPU 2:
lock data structure
find object
kref_get_not_zero() returns false
forget object
unlock data structure

CPU 1:
lock data structure
remove object
unlock data structure
free object

* Example 2 *

CPU 1:
kref_put(): refcount 1 -> 0
lock data structure
remove object
unlock data structure

CPU 2:
lock data structure
object not found
unlock data structure

CPU 1:
free object

Can you point out an actual problem where this will fail to do the right
thing?

Tony