Re: [PATCH] pidhash: Refactor the pid hash table.

[ebiederm@xmission.com (Eric W. Biederman)]

Andrew Morton <akpm@osdl.org> writes:

> ebiederm@xmission.com (Eric W. Biederman) wrote:
>>
>> +fastcall void put_pid(struct pid *pid)
>> +{
>> +	if (!pid)
>> +		return;
>> +	if ((atomic_read(&pid->count) == 1) ||
>> +	     atomic_dec_and_test(&pid->count))
>> +		kmem_cache_free(pid_cachep, pid);
>> +}
>
> This looks odd.  It's an RCU callback so it's asynchronous.  It doesn't
> take any locks, so if anyone else can have a ref on this thing then the
> refcount can change at any time.
>
> And both sides of the || are basically equivalent.  Perhaps you meant &&. 
> But I'm more worried by the apparent raciness?

The || was deliberate.

I expect the count to usually be 1.  So the atomic_read optimizes out
the atomic operation in the common case.  skb_put and kref_put do it
for the same reason.

As for the raciness, that is an interesting case.

Because the decrement is in the rcu callback the count of the structure
is guaranteed to be at least one the entire time the structure is reachable
in an rcu safe manner.  So during that interval the count can go up or down,
safely and atomically, and it won't reach zero.

After the structure stops being reachable in an rcu protected manner
through the hash table it devolves into a simple reference counted
structure.  Now unless something like /proc captured this pid it will
have a count of exactly one, and I expect this to be relatively
common.

So if pid->count == 1 it means I own the only reference, and can do
what I please with this structure.

If pid->count > 1 then I don't own the only reference and someone
may be racing with me.  In that case I do need to do the full
atomic_dec_and_test to see if I have the very last reference to
this structure.

I wasn't certain which part of this you were confused about so I tried
to cover all of my bases.  Hopefully I have cleared up your confusion.

Eric